Added command for removing any todo by thoratvinod · Pull Request #207 · mattermost-community/mattermost-plugin-todo

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.

larkox

LGTM, but with a few non blocking concerns.

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

 }

+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?

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.

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!

hanzei · 2025-01-06T08:38:33Z

@thoratvinod Can you please fix the CI issues?

Error: ./command.go:364:50: not enough arguments in call to p.postReplyIfNeeded
	have (string, string, string)
	want (string, string, string, string)

Thanks!

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

Comment thread server/command.go Outdated

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 larkox and mickmister June 3, 2023 08:54

mickmister approved these changes Jun 6, 2023

View reviewed changes

Comment thread server/command.go Outdated

Comment thread server/plugin.go Outdated

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

hanzei added 2 commits December 3, 2024 09:53

Merge branch 'master' into remove-cmd

ec01631

Merge branch 'master' into remove-cmd

6d3bebb

hanzei added Awaiting Submitter Action Blocked on the author and removed 3: QA Review Requires review by a QA tester Lifecycle/1:stale labels Jan 6, 2025

Uh oh!

Conversation

thoratvinod commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Screenshots

Command: /todo remove --todo <todoID>

Uh oh!

mattermost-build commented May 22, 2023

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larkox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asaadmahmood May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thoratvinod commented Jun 8, 2023

Uh oh!

larkox left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattermost-build commented Jun 23, 2023

Uh oh!

thoratvinod commented Jul 5, 2023

Uh oh!

mattermost-build commented Feb 17, 2024

Uh oh!

mattermost-build commented Mar 25, 2024

Uh oh!

hanzei commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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