Ft adding message field optional #132
Conversation
|
Good to speak with you the other day @alpeshvas I've thought of a few places that will need updating with this change:
I'm hoping to take a proper look at this PR sometime this week 🤞 |
Will make those changes. |
Done. Also, we can use something like this for auto generation of swagger , wdyt @connorads ?https://www.serverless.com/plugins/serverless-openapi-documentation |
|
Nice one @alpeshvas! Unfortunately we got some lint and type check errors in CI Try running
If you want to see what is being run on CI you can have a look at the build script |
Nice idea. I'm open to autogeneration ideas but if I understand correctly this plugin doesn't appear to offer much automation.
|
alpeshvas
force-pushed
the
ft-adding-message-field-optional
branch
from
15e2f9e to
6b6c683
Compare
alpeshvas
force-pushed
the
ft-adding-message-field-optional
branch
from
6b6c683 to
a66b229
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| name: nonEmptyWhitespaceFreeString, | ||
| owner: nonEmptyWhitespaceFreeString, | ||
| }), | ||
| // Making message field optional for backward compatibility |
There was a problem hiding this comment.
We can remove this comment
It's not just for backwards compatibility, people also might not chose to add a description - so it's always optional
| getFirstParam(command.text), | ||
| handleCommand((command) => { | ||
| const resourceName = getFirstParam(command.text); | ||
| const descriptionMessage = getSecondParam(command.text); |
There was a problem hiding this comment.
I was thinking instead of message or descriptionMessage we could consistently call this description everywhere in the types/code/spec/docs etc.
| @@ -25,6 +25,7 @@ export type Destination = "user" | "channel"; | |||
| export interface Response { | |||
| message: string; | |||
| destination: Destination; | |||
| metaData?: Record<string, string>; | |||
There was a problem hiding this comment.
This metaData was never meant to be read out of the DynamoDB via code.
I don't want us to read the metaData out of the DB ever and maybe we should just pass an optional description?: string back in the Response.
That wasn't for you to know, it's not obvious, I thought you added it at first 😅
The metaData is just to be able to store human-readable data in the database for the rare occasion I need to look in DynamoDB via AWS Console and see how people are using it - to understand current use-cases. Otherwise it's a lot of unreadable team/channel/user slack codes.
| @@ -25,6 +25,7 @@ export type Destination = "user" | "channel"; | |||
| export interface Response { | |||
There was a problem hiding this comment.
If you scroll up a bit you can see the LockRepo interface.
I think we'll have to return an additional description?: string from getAll so that the description can be seen in /locks. There will also need to be some additional tests and implementation etc. such has displaying locks with and without a description.
Then for setOwner I think it would make sense to pass description?: string after team and before metadata
| }, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
If you try to get the description after storing it will not work because there's no code to retrieve it.
It only shows the description when you lock it but not when you type /locks or try and get the description from the API.
If you try this test it will fail
test("Create lock with description, get lock", async () => {
await server
.post("/dev/api/teams/T012345WXYZ/channels/C012345ABCD/locks")
.set("Authorization", `Basic ${credentials1}`)
.send('{ "name": "dev", "owner": "U012345MNOP", "message": "Hey man" }');
const res = await server
.get("/dev/api/teams/T012345WXYZ/channels/C012345ABCD/locks/dev")
.set("Authorization", `Basic ${credentials1}`);
expect(res.status).toBe(200);
expect(res.text).toBe(
JSON.stringify({
name: "dev",
owner: "U012345MNOP",
message: "Hey man",
})
);
});
| text: message, | ||
| response_type: getResponseType(destination), | ||
| }; | ||
| if (metaData && metaData.Message) { | ||
| respondArguments.attachments = [ |
There was a problem hiding this comment.
I don't think attachments is the right thing here.
I think we could use Block quote instead and just incoporate it into the message. Although I'm not sure how we'd also make the description look nice when using /locks, perhaps a newline and italics? Would have to have a think/experiment.
In slack attachments looks very similar to a Block quote
which is what we use with /locks
Except you can remove the attachment, which is probably undesirable
Sneak peak:
