MM-21154 - Agenda Plugin Review

[marianunez]

Summary

This PR is to kick start the dev review for the Agenda Plugin to be included in community server. It includes all the changes since the plugin starter template.

See Readme for details on how to use it.

Ticket Link

MM-21154

[jfrerich]

removed

[jfrerich]

Great work, @marianunez! It was a pleasure to review this plugin and I think it will be valuable for us!

I left a couple comments. The only use case that might need to be addressed (that I found) was handling Sun and Saturday. If you could just comment on the items below, that would be great. If you agree with the proposed changes, please let me know and I can make those changes if you'd like.

If you could comment on the following, that would also be helpful.

• Add /agenda help with examples
• Add /agenda setting schedule help text - list possible values 0..6 for Sun..Sat
• Add Sun and Sat to UI (slash comand works 0, 6)
• Currently user can use /agenda setting schedule 0 and will update, but not show in UI.
• Any Int works /agenda setting schedule command. Perhaps limit to 0..6 and next-week only?
• /agenda setting hashtag junk-Feb-01-2000 responds setting updated, but queueing gives weird return result

Possible Enhancements - Can create as HW tickets if not a simple task to add.

• /agenda list shows RHS (or updates). Have /agenda queue also refresh the RHS. (This isn't actually a good idea, but will leave the comment for reference). queue should not hijack a users RHS because list is available. queue shows the result in a post.

[marianunez]

I left a couple comments. The only use case that might need to be addressed (that I found) was handling Sun and Saturday. If you could just comment on the items below, that would be great. If you agree with the proposed changes, please let me know and I can make those changes if you'd like.

I was being optimistic to not having meeting on weekends 😂 Agreed this should support the whole week.

[ ] Add /agenda help with examples

Yes, I agree this should be default for all plugins.

• Add /agenda setting schedule help text - list possible values 0..6 for Sun..Sat
• Add Sun and Sat to UI (slash comand works 0, 6)

Agreed, which you extend support for the whole week. Should be a simple fix.

• Currently user can use /agenda setting schedule 0 and will update, but not show in UI.

This should be fixed with the full week support from above.

Any Int works /agenda setting schedule command. Perhaps limit to 0..6 and next-week only?

Yes, this would make the slash command more robust.

/agenda setting hashtag junk-Feb-01-2000 responds setting updated, but queueing gives weird return result

What is the weird return result?

[levb]

0/5 maybe add (last dozen characters?) from the channel name? Not sure if the use of the default is typical.

[marianunez]

I will create a ticket to add this as a future enhancement.

[crspeller]

Couple of comments. One big optional thing would be to split into packages rather than lumping everything in main. You can use the references of workflow: https://github.com/mattermost/mattermost-plugin-workflow/ and the solar lottery PR: mattermost-community/mattermost-plugin-solar-lottery#6 The exact way we recommend splitting out packages is in flux. Some conversation here: https://community-daily.mattermost.com/core/pl/9sf5x5j7wfyjxjrdoys5prgfdw

[levb]

Fix https://github.com/mattermost/mattermost-plugin-agenda/blob/pluginReview/go.mod#L1 though

[marianunez]

Fix /go.mod@pluginReview#L1 though

@lev will do! I will go through all the review comments. Apologies for the delay on this.

[jfrerich]

@levb, @aaronrothschild, @crspeller, @DHaussermann - This PR was approved by all before we swapped the base branch from starter_template to master. Can everyone re-approve once more for merging?

@DHaussermann, FYI, we still need you review for this PR.

[levb]

I'd prefer that the CI changes were merged here and the build was green. If for some reason it's difficult - ok with doing it later.

[DHaussermann]

@marianunez or @jfrerich I can't build this from master (or the template branch). I get a couple dozen errors that look like this...

Resolved Thanks Jason!

ERROR in ./node_modules/mattermost-redux/actions/preferences.js
Module not found: Error: Can't resolve 'core-js/modules/es.promise' in '/Users/dylanhaussermann/go/src/github.com/mattermost/mattermost-plugin-agenda/webapp/node_modules/mattermost-redux/actions'
 @ ./node_modules/mattermost-redux/actions/preferences.js 3:0-37
 @ ./node_modules/mattermost-redux/actions/posts.js
 @ ./node_modules/mattermost-redux/actions/search.js
 @ ./src/actions/index.js
 @ ./src/index.js
 @ multi ./src/index.js

Am I missing something locally? Do I need specific branch of the redux repo maybe?

[DHaussermann]

First Round of testing completed

• Channel menu item follows demo plugin convention correctly
• If Tag and day of week match across channels - Items are pushed to the same meeting queue
• If tag matches but day of the week does not - Items are pushed to different queues
• Forecasting date based on next day of the week seem to work fine (except whatever is happening on Issue 4.)
• Works across all channel types
• Explored various tags and date formats
  Still To Do
• next-week query and list functionality

@marianunez The biggest issue I'm hitting with this is that setting the date format is too brittle. 0/5 this should maybe be a drop down. It seems like if I don't use Jan02 it does unexpected things. Why does the date format string do more than just define the format? Maybe it is also supposed to set to the next meeting date otherwise it causes issues? For example: QATJan03 on Thursaday and QATJan04 on Thursday do not push to the same list. This is confusing to me as my expectation was that the day of the month is exemplifying the format. Instead because the string does not match things are going to another meeting? Ot is there a different reason? The UI should contain contextual help if all the data must be verbatim.
Another example would be that using PicklesApr02 instead of PicklesJan02 will then create meeting items for April instead of the next upcoming meeting #PicklesApr13 but, I don't get why.

Other Issues

1. I'm not understanding how to use next week. Please help. It keeps posting it as post content. Based on read-me /agenda queue [next-week] message should work but [next-week] remains part of the post
2. Increment is broken if prefix has a "-" as in 'QA-Jan02' Also broken when using "QAT Jan-02-06" or "01/02/06"
   
3. Agenda item can sometimes be in the past? (don't know how to repro with other data)
   1. Open a channel where the agenda data was never modified
   2. Change the stock date from "Jan02" to "Jan03" and save (don't touch the day of the week.)
   3. Queue an item
      Item Queued for March 4th. In the past and not a Thursday
      
4. The word the is duplicated in the modal text as it is also part of the link. Text reads Date format should follow the one of the predefined layouts Please remove the first the
5. No LHS item listed under System Console >> Plugin Management

Could wait for further enhancement:

• Validate the date format on save or make UI to select it from.
  Things can go wrong if the user does not see the warning and uses a format they've seen before ..
  
• Validate on save that the plugin will produce a valid hashtag to avoid things like "QA" which is too short to be a hashtag
• Changing the day of the week for a meeting could be difficult as I can't see what other channels are using the same tag-day combination and meeting queue- Show me a list maybe?
• Should agenda have just place holder text in the settings box instead of a default value? this could avoid un-intentional meeting collision? (would also require and ephemeral post if you use the plugin when no data is saved)

[marianunez]

The biggest issue I'm hitting with this is that setting the date format is too brittle. 0/5 this should maybe be a drop down. It seems like if I don't use Jan02 it does unexpected things.

Agreed. The date formatting is NOT user friendly at all. The format you do needs to strictly be based on the actual date of January 2, 2006 🤦‍♀️ I added a link of examples of date formats on the dialog but agreed is not clear.

Why does the date format string do more than just define the format? For example: QATJan03 on Thursaday and QATJan04 on Thursday do not push to the same list. This is confusing to me as my expectation was that the day of the month is exemplifying the format. Instead because the string does not match things are going to another meeting? Ot is there a different reason?

Exactly, if you don't use January 2nd as the date example, then it has unexpected behavior. If you want a format of QATMonDay then it MUST be set to QATJan02. This is because of the way Go formats date and time.

The idea of leaving it open text was to give flexibility to creating your own hashtag format but I'm 2/5 this needs to be improved in some way before release because this issue will be coming up constantly.

Some options:

• Improve the help text in the dialog to warn that the exact date must match January 2nd 2006 for the format to work.
• Limit it to a pre-set list of date formats like you suggest but not sure how this would work on placing the rest of your hashtag text (just before the date?).
• Design a different way/UI to define your date format all together.

[marianunez]

1. m not understanding how to use next week. Please help. It keeps posting it as post content. Based on read-me /agenda queue [next-week] message should work but [next-week] remains part of the post

The flag is a literal of next-week without the brackets. The idea of the brackets was to indicate optional. So for example:

/agenda queue next-week This is the item for next week

should queue This is the item for next week with the hashtag of the meeting in the next calendar week.

2. Increment is broken if prefix has a "-" as in 'QA-Jan02' Also broken when using "QAT Jan-02-06" or "01/02/06"

This is related to the search in the server. If you search in the RHS (which is what the plugin does to calculate the list numbers) for QA-Jan02 it does not find it in a server without Elasticsearch. I reported this in MM-20984 and was closed as working as expected.

Agenda item can sometimes be in the past? (don't know how to repro with other data)

1. Open a channel where the agenda data was never modified
2. Change the stock date from "Jan02" to "Jan03" and save (don't touch the day of the week.)

This is the unexpected behavior mentioned in the comment before of not using January 2nd as the literal date for the format. 😖

5. No LHS item listed under System Console >> Plugin Management

Not sure how the System Console behaves in this case but it may be because the plugin has no settings available for configuration.

Thanks for the thorough testing @DHaussermann! Great feedback!

[DHaussermann]

@marianunez thanks for clarifying. Having no background on the plugin - I did click the link and view the page with examples. For me personally, I took that to mean "Use only these formats" but did not understand that I had to actually use that date for my example. I only figured that out when I realized other dates were causing issues :)

I still think validating on save or using a drop down would be better But, I think a small change that would add alot of value here would be to add a bit more explanation in the modal that you're not just copying the format but,the date as well. For example...
Using the example date of January 2nd, set the date format in the box below ... then the rest of the text and link you already have there.

edit: 0/5 There should also be an explanation that matching meetings tags would be applied across all channels. Depending how some people use Mattermost they may not be obvious.

[DHaussermann]

Tested for queuing and listing using next week Thanks @marianunez the [next-week] is working.

Couple questions..

1. Is there any way that the plugin can know if the meeting happening that same day has already occurred.

I ask because what I'm seeing is that if the current day is Friday and the meeting is set for Fridays, On the day of - Queuing items will go to the next week's meeting. And pulling the Queue list will pull next weeks meeting. Both of these make sense depending on the time of day IF the meeting has already taken place. But in cases where your checking the list of items before the meeting it causes an issue. Any thoughts?

2. Is it low effort to add a bit more info to the modal text to avoid people using dates other than Jan2nd to set the format?

[marianunez]

1. Is there any way that the plugin can know if the meeting happening that same day has already occurred.

Yes, for this case we need to support meeting time. This way we can check at the time of any request if the meeting has happened already. I think this would be a good HW candidate.

I ask because what I'm seeing is that if the current day is Friday and the meeting is set for Fridays, On the day of - Queuing items will go to the next week's meeting. And pulling the Queue list will pull next weeks meeting.

@DHaussermann Was this using the next-week flag? The behavior I'm seeing is that when I do /agenda queue This item is for today the same day of the meeting it will queue it for that day not for next week.

2. Is it low effort to add a bit more info to the modal text to avoid people using dates other than Jan2nd to set the format?

Yes it is a simple change. How about:

Suggestions on improving the help text are welcomed. cc @jfrerich @aaronrothschild

[DHaussermann]

@aaronrothschild should we improve the help text on this before the initial release?

[DHaussermann]

Thanks for the work on this @marianunez

• The core functionality is working.
• Overview of testing posted in comment above
• Added test case to release testing
  LGTM!

Created #17 and #18
Will follow-up with @aaronrothschild to see what must be resolved before this is suitable for deployment to community server.