Added command for removing any todo #207

thoratvinod · 2023-05-22T17:20:14Z

Summary

Added command for removing any todo

Ticket Link

Fixes #195

Screenshots

Command: `/todo remove --todo <todoID>`

mattermost-build · 2023-05-22T17:20:23Z

Hello @thoratvinod,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

mickmister

@thoratvinod Thanks for this great contribution 👍

I have one suggestion to de-deplicate/share some existing code handling issue deletion. Please let me know your thoughts on this.

server/command.go

larkox

LGTM, but with a few non blocking concerns.

.gitignore

server/command.go

larkox · 2023-05-23T08:56:02Z

server/command.go

@@ -327,6 +329,61 @@ func (p *Plugin) runPopCommand(args []string, extra *model.CommandArgs) (bool, e
 	return false, nil
 }

+func (p *Plugin) runRemoveCommand(args []string, extra *model.CommandArgs) (bool, error) {
+	message := strings.Join(args, " ")


Having to write down the whole message seems bad UX, but 0/5.

True. Maybe we can only take the number of Todo in the remove cmd and delete that specific todo. In that case, I think we should change the list of todos from a bullet list to a numbered list. What do you think about this?

Numbered list is "dangerous". I get my list: 1. todo A, 2. todo B, 3. todo C. Now I say... delete number 2 (todo B). But while I am writing that, another user deletes the first one todo A. And I end up deleting the "new second one", todo C.

Something I did for the badges plugin was to use the autocomplete to add the badge id (in this case the todo id). May be an option, but 0/5 on how good. I don't like much the way it is done in the badges plugin neither 😅

A more elaborate option (and maybe not good neither) is that /todo delete opens an interactive dialog with all the todos, and you mark with a checkmark all of the todos you want to delete.

@asaadmahmood Since you were doing some changes on the todo plugin some time ago, not sure if you have any thoughts on the UX for this particular part (slash command for todo deletion).

@asaadmahmood any suggestion here?

@thoratvinod @larkox I think we should have an autocomplete with the todo list.
So when the user types /todo remove an autocomplete opens up, with the todo items (name+id+description), and then the user can select the one he wants to remove.

When he selects it, the selection can turn into an id for simplicity.

See screenshot for autocomplete.

Thanks for the suggestion @asaadmahmood. However, I'm not sure how we can implement this in the code.

After reading the code, I've noticed a few things. Please correct me if I'm wrong:

The OnActivate method will run only once when the plugin is activated.

The ExecuteCommand method will run every time a command is entered.

The OnDeactivate method will run only once when the plugin is deactivated.

We register all plugin commands in the OnActivate method, which is executed only once after the plugin is activated. If we want to add autocomplete for the delete command, we would need to fetch the latest todo list, which I believe is not possible.

@mickmister @larkox please let me know if I am missing something or if you have any way to enable autocomplete for delete.

Take a look to https://github.com/larkox/mattermost-plugin-badges

The idea is to register the autocomplete as something dynamic. Hopefully reading the code in there is enough, but if you need more guidance let me know 😄

Have made changes, also updated the screenshots.

@thoratvinod The current screenshot in the PR description shows the todo ids. Is this intentional?

server/command.go

mickmister

Just one comment to avoid index out of range issue. Otherwise LGTM 👍

FYI I'm going on PTO for a few weeks and won't be able to re-review this. Please take this review as my approval other than the index out of range issue.

server/command.go

server/plugin.go

thoratvinod · 2023-06-08T16:38:56Z

@larkox @asaadmahmood have resolved the comments, pls have a look.

larkox

LGTM, I will leave @asaadmahmood to consider the UX implications, but that part also looks good to me.

codecov-commenter · 2023-06-12T08:20:10Z

Codecov Report

Attention: Patch coverage is 0% with 85 lines in your changes are missing coverage. Please review.

Project coverage is 6.53%. Comparing base (1afeeb0) to head (04b917f).
Report is 5 commits behind head on master.

❗ Current head 04b917f differs from pull request most recent head 9a43981. Consider uploading reports for the commit 9a43981 to get more accurate results

Files	Patch %	Lines
server/command.go	0.00%	49 Missing ⚠️
server/plugin.go	0.00%	36 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #207      +/-   ##
=========================================
+ Coverage    6.42%   6.53%   +0.11%     
=========================================
  Files          11      10       -1     
  Lines        1712    1682      -30     
=========================================
  Hits          110     110              
+ Misses       1594    1564      -30     
  Partials        8       8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattermost-build · 2023-06-23T01:03:13Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

thoratvinod · 2023-07-05T14:27:56Z

@asaadmahmood please have a look.

mattermost-build · 2024-02-17T01:02:37Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mattermost-build · 2024-03-25T01:02:19Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Added support for removing any todo

85324b5

thoratvinod requested a review from larkox as a code owner May 22, 2023 17:20

larkox requested a review from mickmister May 22, 2023 17:23

mickmister reviewed May 22, 2023

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

larkox approved these changes May 23, 2023

View reviewed changes

hanzei added the 2: Dev Review Requires review by a core committer label May 30, 2023

hanzei requested a review from asaadmahmood May 30, 2023 07:57

hanzei added the 1: UX Review Requires review by a UX Designer label May 30, 2023

Vinod Thorat added 3 commits June 1, 2023 11:31

Added autocomplete for delete command

eb70898

rename delete to remove

96b5240

removed unnecessary code from getRemoveTodoSuggestions

1e612b7

thoratvinod changed the title ~~Added command for removing any todo~~ Added command for deleting any todo Jun 3, 2023

thoratvinod changed the title ~~Added command for deleting any todo~~ Added command for removingany todo Jun 3, 2023

thoratvinod changed the title ~~Added command for removingany todo~~ Added command for removing any todo Jun 3, 2023

thoratvinod requested review from mickmister and larkox June 3, 2023 08:54

mickmister approved these changes Jun 6, 2023

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

server/plugin.go Outdated Show resolved Hide resolved

Addressed comments

04b917f

larkox approved these changes Jun 12, 2023

View reviewed changes

mattermost-build added the Lifecycle/1:stale label Jun 23, 2023

Merge branch 'master' into remove-cmd

9a43981

asaadmahmood approved these changes Feb 6, 2024

View reviewed changes

mickmister requested a review from AayushChaudhary0001 February 6, 2024 20:22

mickmister removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale labels Feb 6, 2024

mattermost-build added the Lifecycle/1:stale label Feb 17, 2024

mickmister added 3: QA Review Requires review by a QA tester and removed 1: UX Review Requires review by a UX Designer Lifecycle/1:stale labels Mar 14, 2024

mattermost-build added the Lifecycle/1:stale label Mar 25, 2024

Merge branch 'master' into remove-cmd

ec01631

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added command for removing any todo #207

Added command for removing any todo #207

thoratvinod commented May 22, 2023 •

edited

Loading

mattermost-build commented May 22, 2023

mickmister left a comment

larkox left a comment

larkox May 23, 2023

thoratvinod May 23, 2023

larkox May 24, 2023

thoratvinod May 29, 2023

asaadmahmood May 30, 2023 •

edited

Loading

thoratvinod Jun 2, 2023

larkox Jun 2, 2023

thoratvinod Jun 3, 2023

mickmister Dec 4, 2023

mickmister left a comment

thoratvinod commented Jun 8, 2023

larkox left a comment

codecov-commenter commented Jun 12, 2023 •

edited

Loading

mattermost-build commented Jun 23, 2023

thoratvinod commented Jul 5, 2023

mattermost-build commented Feb 17, 2024

mattermost-build commented Mar 25, 2024

Added command for removing any todo #207

Are you sure you want to change the base?

Added command for removing any todo #207

Conversation

thoratvinod commented May 22, 2023 • edited Loading

Summary

Ticket Link

Screenshots

Command: /todo remove --todo <todoID>

mattermost-build commented May 22, 2023

mickmister left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asaadmahmood May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

thoratvinod commented Jun 8, 2023

larkox left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 12, 2023 • edited Loading

Codecov Report

mattermost-build commented Jun 23, 2023

thoratvinod commented Jul 5, 2023

mattermost-build commented Feb 17, 2024

mattermost-build commented Mar 25, 2024

thoratvinod commented May 22, 2023 •

edited

Loading

Command: `/todo remove --todo <todoID>`

asaadmahmood May 30, 2023 •

edited

Loading

codecov-commenter commented Jun 12, 2023 •

edited

Loading