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

[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.

• [x] Add /agenda setting schedule help text - list possible values 0..6 for Sun..Sat
• [x] 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]

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.

[marianunez]

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

@jfrerich Currently, the date format to include must be one of the supported layouts by Go time.Format. In that example it should be junk-Feb-02-2006. The unexpected behavior of an unknown format is coming from that implementation.

To try to avoid those scenarios, I've included a link in the UI to explain which type of formats are currently supported.

Not sure how we could make this less "techy" and more user friendly.

[jfrerich]

With the latest commits, from @marianunez, I believe we are good to move forward. @levb, do you want to have QA review first, or can we merge first? I believe we need to change the PR pointer from starter_template to master and then we can merge?

[marianunez]

I've created some initial HW tickets to cover the comments here and other useful features:

https://github.com/mattermost/mattermost-plugin-agenda/issues/9 https://github.com/mattermost/mattermost-plugin-agenda/issues/10 https://github.com/mattermost/mattermost-plugin-agenda/issues/11 https://github.com/mattermost/mattermost-plugin-agenda/issues/12

[levb]

Apologies - just noticed that @aaronrothschild and @DHaussermann were not assigned to this PR to review; added now. Also; @jfrerich FYI I added QA the team to the repo.

[jfrerich]

@marianunez, Thanks for the HW tickets!

@levb, thanks for the QA and PM additions. I wasn't sure if you preferred them now or post merge.

[levb]

/update-branch

[jfrerich]

@levb, @marianunez - I propose we change the base branch from starter_template to master now that coding changes are completed. Do y'all agree? There is no need to merge to starter_template and then merge again to master

[marianunez]

@jfrerich agreed! Changing now.

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