search

mattermost-community / mattermost-plugin-agenda

Mattermost plugin to handle meeting agendas
Apache License 2.0
35 stars 21 forks source link

GH-9 - Added support for multiple meetings per week #46

Closed marianunez closed 4 years ago

marianunez commented 4 years ago

Summary

Added support for multiple meetings per week.

Screen Shot 2020-06-14 at 10 02 01 AM

Included support for specifying the week day to queue or list an item given the week day (short name, long name or number):

/agenda queue Mon This will be an item for Mondays meeting /agenda list Thursday /agenda queue This will be posted for the next day in the list of configured days

Also includes autocomplete dynamic values:

Screen Shot 2020-07-10 at 4 31 06 PM Screen Shot 2020-07-10 at 4 09 16 PM

Ticket Link

Fixes https://github.com/mattermost/mattermost-plugin-agenda/issues/9

codecov[bot] commented 4 years ago

Codecov Report

Merging #46 into master will decrease coverage by 3.17%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   19.63%   16.46%   -3.18%     
==========================================
  Files           6        6              
  Lines         275      328      +53     
==========================================
  Hits           54       54              
- Misses        209      262      +53     
  Partials       12       12
Impacted Files Coverage Δ
server/command.go 0.00% <0.00%> (ø)
server/meeting.go 23.52% <0.00%> (-5.05%) :arrow_down:
server/plugin.go 26.26% <0.00%> (-12.55%) :arrow_down:
server/utils.go 48.78% <0.00%> (-17.89%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 31c1a48...2b0a83c. Read the comment docs.

marianunez commented 4 years ago

@lieut-data @jfrerich I've requested your review again given that I included support for specifying the week day to queue or list an item given the week day (short name/long name or number):

/agenda queue Mon This will be an item for Mondays meeting /agenda list Thursday /agenda queue This will be posted for the next day in the list of configured days

Apologies for the churn on this PR.

marianunez commented 4 years ago

What happens to configurations already in the key value store? Is it possible to "migrate" them into the new array format?

@lieut-data The meetings that were already configured would be "lost" and they would get a message like this when they try to queue a meeting:

Screen Shot 2020-06-18 at 5 17 34 PM

(Wording open to suggestions)

At this point they would go back to the channel setting menu and reconfigure their meeting. Given that this plugin is still in very early stage (not even the first v0.1 is released) it didn't seem to be worth the effort to implement a migration.

hanzei commented 4 years ago

@DHaussermann Given that this plugin is not in the Marketplace yet, would you be fine skipping QA review?

marianunez commented 4 years ago

@hanzei I need to make an update here to add some values to the autocomplete from changes in this PR.

marianunez commented 4 years ago

Added autocomplete values for meeting days for queue and list commands:

Screen Shot 2020-07-10 at 4 31 06 PM Screen Shot 2020-07-10 at 4 09 16 PM

Apologies again for the churn on this PR.

hanzei commented 4 years ago

@marianunez @DHaussermann Given this plugin isn't in the Marketplace yet, I would opt for skipping QA review here and trying to push for a new release instead. Thoughts?

marianunez commented 4 years ago

@marianunez @DHaussermann Given this plugin isn't in the Marketplace yet, I would opt for skipping QA review here and trying to push for a new release instead. Thoughts?

@hanzei I would be ok with that. However, before we deploy to community, we probably want to include this PR https://github.com/mattermost/mattermost-plugin-agenda/pull/30 as well, given that both are a "breaking" change in the sense that any existing meeting settings need to be reconfigured. We might as well do this only once.

DHaussermann commented 4 years ago

Hi @marianunez I've done some testing on this and I'm seeing some strange issues.

  1. Queuing items for current week always schedules for Thursday the 23 and Queuing next-week always goes to the 26th I have tried:
    • Changing hashtag to ensure it's unique
    • Changes meeting days
    • Changing from 2 days a week to 1 day a week all with this behavior still.

Steps:

A. Observed: The auto-complete only shows Thursday and next week Expected: If both days are still in the future this week, I expected to see both days as an option as well as next week B. Observed: Queuing items for current week always schedules for Thursday the 23 and Queuing next-week always goes to the 26th. Expected: Queue should go to the earliest scheduled day this week that has not ye passed. (on Tuesday if meeting days are Monday and Friday, it should go to Monday) Queue next week should go to 1st meeting taking place next week

  1. I see intermittent errors like this Image Pasted at 2020-7-20 17-15 They go away when I move the meeting to a different day and then move it back. Maybe upgrade related? Any thoughts?

Also just to confirm, there is no way to preserve the meeting day on upgrade?

marianunez commented 4 years ago
  1. I see intermittent errors like this Image Pasted at 2020-7-20 17-15

After you upgrade, any channel that had a meeting defined before will see this error message until they update the settings again (redefine their day and hashtag). This should not happen in channels that have never setup the meeting before the upgrade. Given that this plugin is still in beta we decided that doing a migration to preserve the meeting settings on upgrade was not worth the effort.

  1. Queuing items for current week always schedules for Thursday the 23 and Queuing next-week always goes to the 26th I have tried:
  • Changing hashtag to ensure it's unique
  • Changes meeting days
  • Changing from 2 days a week to 1 day a week all with this behavior still.

Steps:

  • Installed v0.0.2. confirmed queue and queue-next-week were working normally
  • Upgrade to this branch
  • Configure a new channel or pre-existing channel to only have a meeting on 2 days (That does not include Thursday and are passed the current day of the week)

@DHaussermann Regarding this issue, can you share what is the hashtag and meeting days you have configured when you see this problem?

DHaussermann commented 4 years ago

@marianunez I seem to be seeing this on all channels regardless of which days are configured or if I change them.

For Example: 1 of the channels has a Meeting set for Tuesday only and a hashtag of Pineapple-Jan02

If it helps, the cloud server where I tested the upgrade is: https://dkh-esr2-test.test.mattermost.cloud All channels seem to be affected. Not sure if seeing the issue from the UI alone would be beneficial?

marianunez commented 4 years ago

@DHaussermann I took a look at the server and I noticed that it's a v5.24. Apologies, I should've included that the dynamic autocomplete is using a feature available in v5.26. That is why you do not see the meeting days correctly in the autocomplete. I've updated the branch and now it should not be able to install less than v5.26.

Regarding the error message, I was able to correctly queue on the channels and didn't get any error messages. DM me if you are still having issues with this and I can take a look at your example in the server.

marianunez commented 4 years ago

/update-branch

DHaussermann commented 4 years ago

Thanks for changes @marianunez . I see some of the issues seem to be resolved. However, it seems like the queuing for correct days of the week is still behaving strangely. The days available do not seem to update in real time. By playing around with a few channels, I was able to see the correct days of the week available to queue on 1 channel. But, this updated the auto-complete option in every channel. Is it possible there's a bug where this is not properly being set in a way that is channel specific?

See current server https://dkh-agenda-test.test.mattermost.cloud

Steps:

Please let me know if you can repro this.

marianunez commented 4 years ago

Is it possible there's a bug where this is not properly being set in a way that is channel specific?

@DHaussermann investigating this more, I did find a bug on the webapp side that is not updating the autocomplete options at real time. I've created this bug MM-27182 which also includes steps on how to reproduce the issue in the Incident Response as well.

In order to get past this bug and test the agenda plugin is working properly, you can refresh the page each time before executing the slash command again.

DHaussermann commented 4 years ago

@marianunez thank you so much for figuring out what was happening there and writing up a bug!

Using refresh as needed, I have tested this further. I have 1 more question / concern... What is the desired behavior of next-week " If next-week is provided, it will list the agenda for the next calendar week." My expectation is that the item would be queued for the earliest occurrence of the meeting that takes place on the following week. (I believe it worked this way before)

What I'm seeing is that it get's scheduled for the Sunday of the following week instead of the day the earliest meeting falls on. This occurs even if you only have one event day per week. (I tried using refresh to see if it would resolve the issue) list has this same behavior where it shows me the queue for Sunday but that may not be a bug since there are events to show me already.

Steps:

Expected Item scheduled for Monday the 27th - the earliest meeting eve next week Observed Item is scheduled for Sunday the 26th

hanzei commented 4 years ago

@marianunez Heads up that there is a merge conflict.

marianunez commented 4 years ago

What I'm seeing is that it get's scheduled for the Sunday of the following week instead of the day the earliest meeting falls on.

Great catch @DHaussermann! I finally got a chance to take a look and have pushed a fix for this. Thanks for the thorough testing on this. 👌

DHaussermann commented 4 years ago

/update-branch

hanzei commented 4 years ago

@marianunez Is this PR ready to get merged?

marianunez commented 4 years ago

Yes, thanks @hanzei :)