-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix message actions #202
Fix message actions #202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
=======================================
Coverage 63.82% 63.82%
=======================================
Files 7 7
Lines 445 445
Branches 122 122
=======================================
Hits 284 284
Misses 153 153
Partials 8 8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! The only piece of feedback I have is that the fake bodies that you are using throughout the test cases are not truly minimal, and that leaves some room for confusion regarding what properties are truly necessary for a body to be interpreted as a specific kind of event. I also see value in having bodies that are recognizable as something that might actually come from Slack, and therefore have more properties defined that strictly necessary, but I think that belongs in an integration test (which we should also have... at some point).
I'm happy for this to land as-is, so I'm giving it approval. I just wanted to capture that feedback in case you also thought now is the right time to go as minimal as possible for the bodies.
src/helpers.spec.ts
Outdated
event: { | ||
type: 'app_home_opened', | ||
channel: conversationId, | ||
event_ts: 'EVENT_TS', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think type
or event_ts
are technically needed for the tests to succeed. right?
const dummyCommandBody = { | ||
command: 'COMMAND_NAME', | ||
channel_id: conversationId, | ||
response_url: 'https://hooks.slack.com/commands/RESPONSE_URL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think response_url
is needed for the test to succeed.
src/helpers.spec.ts
Outdated
{ | ||
type: 'message_action', | ||
channel: { id: conversationId }, | ||
callback_id: 'CALLBACK_ID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think a callback_id
is necessary.
src/helpers.spec.ts
Outdated
type: 'dialog_submission', | ||
channel: { id: conversationId }, | ||
callback_id: 'CALLBACK_ID', | ||
submission: { KEY: 'VALUE' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think a callback_id
or a submission
are necessary.
type: 'button', | ||
name: 'NAME', | ||
value: 'VALUE', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think a callback_id
or a real value for the first action (as long as there is something in the array) are necessary.
}, | ||
value: 'VALUE', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think a real value for the first action (as long as there is something in the array) is necessary.
src/helpers.spec.ts
Outdated
{ | ||
type: 'block_suggestion', | ||
channel: { id: conversationId }, | ||
action_id: 'ACTION_ID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think the action_id
is necessary.
Summary
Fixes #201 and adds tests
Requirements (place an
x
in each[ ]
)