Add discard option #42
Conversation
|
Thanks for the contribution! Before we can merge this, we need @NickChokas to sign the Salesforce Inc. Contributor License Agreement. |
|
👋 @NickChokas Thanks so much for this contribution and for this great addition. Would you mind signing the CLA agreement? You should see it linked above on this page. |
👋 srajiang - Absolutely! I think it should be good actually: |
There was a problem hiding this comment.
➕ This is super slick! And the code looks great! Left a few comments with some thoughts, but I think all of it can be ignored without problem. Giving an approval now and can merge soon, but will wait a bit for adjustments if you want to make those.
Thanks a ton for sending this in! ❤️
| // If the user selects to discard the draft message | ||
| if (action.selected_option.value == "discard_message_overflow") { |
There was a problem hiding this comment.
IMO it makes sense to break both the edit_message_overflow and discard_message_overflow into separate functions since, once the action.selected_option.value is decided, these branches end with exit or error.
It'd make openDraftEditView act more as a handler which might make parsing a bit easier, but perhaps not... This is personal preference for sure, so your call on this change!
| // Delete the draft from the 'drafts' Datstore | ||
| const deleteResp = await client.apps.datastore.delete({ | ||
| datastore: DraftDatastore.name, | ||
| id: id, | ||
| }); |
There was a problem hiding this comment.
Without getting too deep in thought, I might opt to delete from the datastore before deleting the message in case something goes wrong with the datastore. In either case an error from either of these gets us into a weird state, so I don't think this has to be changed, but would rather the datastore act as a source of drafting truths if possible!
There was a problem hiding this comment.
Ahh yeah that makes sense to me. I'll move the Delete Datastore operation before the Slack Message Deletion 👌
There was a problem hiding this comment.
Thanks for the update! Realizing now that moving the datastore check first might also let us return from the function early and avoid deleting the chat message after an error 😳
I'm thinking a try / catch wrapping this function could be interesting, then throwing errors instead of returning early 🤔 But an early return works well too!
There was a problem hiding this comment.
All awesome changes, thank you! 👏 This is looking great and about ready to merge but some small things popped out after those changes for me.
Left a few comments around handling more error cases that I think are worth considering, mostly around guarding against strange function behavior and all. Once that seems alright, I believe this'll be a sure way to delete drafts 💪
| // Delete the draft from the 'drafts' Datstore | ||
| const deleteResp = await client.apps.datastore.delete({ | ||
| datastore: DraftDatastore.name, | ||
| id: id, | ||
| }); |
There was a problem hiding this comment.
Thanks for the update! Realizing now that moving the datastore check first might also let us return from the function early and avoid deleting the chat message after an error 😳
I'm thinking a try / catch wrapping this function could be interesting, then throwing errors instead of returning early 🤔 But an early return works well too!
Thanks again! This was fun! |
Type of change (place an x in the [ ] that applies)
Summary
The goal of this PR is to add a "Discard" option to the Draft Message overflow menu, giving the user the option to quickly Delete the Draft if it's not needed. The added logic in
create_draft/interactivity_handler.tswill Delete the message from the Draft Slack Channel and also delete the respective record from thedrafts.tsDatastore.Requirements (place an x in each [ ] that applies)