[GH-47] [FIX] Agenda item number should get updated if the items of the agenda are deleted #88

sanjaydemansol · 2021-08-31T04:58:47Z

Summary

Fixes #47
Now, Whenever we add new agenda item. Plugin searches for old messages for that hashtag.
If some item is missing(deleted), it fixes numbering for all items.

Testing

Ensure that the agenda plugin is installed and enabled.
Go to a channel header and edit the agenda settings to update the prefix and
Use command /agenda queue to queue at least 3 items as following:
/agenda queue item 1
/agenda queue item 2
/agenda queue item 3

Click on the agenda hashtag to open up the agenda list on the RHS.
On the RHS, click the post menu for item 1 (or the first item that was queued) and delete the agenda item.
Ensure that now there are only 2 items for that agenda hashtag: item 2 and item 3 on the RHS.
Add a new agenda item: /agenda queue new item.
Click on the hashtag to refresh the RHS.

Outcome: The RHS has 3 agenda items numbered 1), 2) and 3)

Ticket Link

#47

codecov · 2021-08-31T07:23:23Z

Codecov Report

Merging #88 (dd95544) into master (2f22484) will decrease coverage by 4.83%.
The diff coverage is 5.83%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   26.03%   21.19%   -4.84%     
==========================================
  Files           7        7              
  Lines         315      453     +138     
==========================================
+ Hits           82       96      +14     
- Misses        216      339     +123     
- Partials       17       18       +1

Impacted Files	Coverage Δ
server/command.go	`0.00% <0.00%> (ø)`
server/utils.go	`62.74% <0.00%> (-31.20%)`	⬇️
server/meeting.go	`62.06% <50.00%> (-0.98%)`	⬇️
server/main.go	`0.00% <0.00%> (ø)`
server/manifest.go	`100.00% <0.00%> (ø)`
server/configuration.go	`0.00% <0.00%> (ø)`
server/plugin.go	`25.74% <0.00%> (+3.16%)`	⬆️

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 2f22484...dd95544. Read the comment docs.

maisnamrajusingh · 2021-09-03T02:18:55Z

server/command.go


-	_, appErr = p.API.CreatePost(&model.Post{
+	if today.Year() <= formattedDate.Year() && today.YearDay() < formattedDate.YearDay() {
+		return responsef("We don't support re-queuing future items, only available for present and past items.")


I think you should make it more short. Something like Re-queing future items are not supported.

Sure, thanks.

maisnamrajusingh · 2021-09-03T02:24:12Z

webapp/src/actions/index.js

+    // Default to town square if there is no current channel (i.e., if Mattermost has not yet loaded)
+    if (!currentChannel) {
+        currentChannel = await Client4.getChannelByName(currentTeamId, 'town-square');


Might be best to move the town-square into a variable. I remember this pattern being followed in some of the other plugins. @mickmister what do you suggest ?

If we are using a post dropdown item to requeue this agenda item, Mattermost has indeed loaded and we can assume currentChannel is already a valid channel. We don't need this if block

server/command.go

mickmister

Great work @sanjaydemansol 👍

I have some requests regarding the implementation, and removing code related to other PRs

mickmister · 2021-09-07T20:54:21Z

webapp/src/components/meeting_settings/meeting_settings.jsx

+                                <select
+                                    name='format'
+                                    value={this.state.dateFormat}
+                                    onChange={this.handleDateFormat}
+                                    style={{height: '35px', border: '1px solid #ced4da'}}
+                                    className='form-select'
+                                >
+                                    <option value='Jan 2'>{'Month_day'}</option>
+                                    <option value='2 Jan'>{'day_Month'}</option>
+                                    <option value='1 2'>{'month_day'}</option>
+                                    <option value='2 1'>{'day_month'}</option>
+                                    <option value='2006 1 2'>{'year_month_day'}</option>
+
+                                </select>


Is this code supposed to be for a different PR?

Please follow sequence for reviewing these three PRs #66->#12->#47. There is one commit for each issue.

@sanjaydemansol, I'd prefer them as separate PR's in the future so we can view the code in isolation to the requested issue.

mickmister · 2021-09-07T21:00:11Z

webapp/src/index.js

+        registry.registerPostDropdownMenuAction('Re-queue', (params) => {
+            store.dispatch(requeueItem(params));
+        });


We should only show this if the post has an agenda item right? We can use the third argument to registerPostDropdownMenuAction here.

Is it feasible to read the post's message and determine that way? We may not have the channel's agenda settings readily available. We may need to start putting something in the post's props to signal that this post is an agenda item if this isn't possible.

we could make channel's agenda settings readily available but 2/5 in my opinion we should go with your other suggestion i.e props in message. It would solve multiple issues like in current implementation we can't locate agenda messages if user change hashtag format multiple times. Also, backwards compatibility could be added after this.
I am not sure if that's possible, would be great if we could store something like this in props:
{ isAgendaItem:true, hastagDateFormat: "dd/mm/yy", hashtagPrefix:"myprefix", eventDate:"isoDate" }
@mickmister thoughts?

@sanjaydemansol Yeah in the props we can do something like:

{ "agendaSettings": { "hasftagDateFormat": "dd/mm/yy", "hashtagPrefix": "myprefix", "eventDate": "isoDate" } }

Then we just need to check post.props.agendaSettings. If that's not in the props, don't show the requeue button.

What is the eventDate you're thinking of? Is that something we'll need to keep up to date in the post's props? When will we reference the props instead of the channel's agenda settings and vise versa?

It will be the date the even is supposed to happen on, since hashtag format may be changed any time. Also user may change format after scheduling a meeting rendering old items unparsable. Ex, #1_2_2021 could mean any of two dates. So, either we must have old hashtag format(which was used at the time of creation of this hashtag) or better eventDate.

mickmister · 2021-09-07T21:01:31Z

webapp/src/actions/index.js

+    // Default to town square if there is no current channel (i.e., if Mattermost has not yet loaded)
+    if (!currentChannel) {
+        currentChannel = await Client4.getChannelByName(currentTeamId, 'town-square');


If we are using a post dropdown item to requeue this agenda item, Mattermost has indeed loaded and we can assume currentChannel is already a valid channel. We don't need this if block

mickmister · 2021-09-07T21:04:10Z

webapp/src/actions/index.js

+export function requeueItem(itemId) {
+    return async (dispatch, getState) => {
+        const command = `/agenda requeue ${itemId}`;


What is itemId? It seems requeueItem is called like this:

registry.registerPostDropdownMenuAction('Re-queue', (params) => { store.dispatch(requeueItem(params)); });

which would mean params is an itemId?

Will the user ever run /agenda requeue manually? If not, we should use an http endpoint for this feature and create a slash command. If the user would use this command manually, what would they supply for the itemId?

Sure will refactor params to itemId.
Made /agenda requeue to support future options like
/agenda requeue all # meeting canceled, I want to requeue all items to next meeting.
/agenda requeue last # requeue last items
/agenda requeue last n # requeue last n items

Okay if this is the case, then maybe we should have a /agenda requeue item (itemId) command, and use that for the current programmatic case. Also what is itemId? Is it the post ID? Let's use postId as the identifier instead of itemId if this is the case. And so we would also change the command to /agenda requeue post (postId)

Right, will change it to postId and change command as well.

mickmister · 2021-09-07T21:08:28Z

server/meeting_test.go

@@ -138,7 +138,7 @@ func TestPlugin_GenerateHashtag(t *testing.T) {
 			jsonMeeting, err := json.Marshal(tt.args.meeting)
 			tAssert.Nil(err)
 			api.On("KVGet", tt.args.meeting.ChannelID).Return(jsonMeeting, nil)
-			got, err := mPlugin.GenerateHashtag(tt.args.meeting.ChannelID, tt.args.nextWeek, -1)
+			got, err := mPlugin.GenerateHashtag(tt.args.meeting.ChannelID, tt.args.nextWeek, -1, false, time.Now().Weekday()) // last parameter doesn't matter unless requeue


Can we change the last parameter to not rely on time.Now(). Tests should have completely static data if possible. The comment here can be removed too.

Not sure about the best way to do same, since go lang doesn't support default values for parameters.
any ideas?

Just use some random hardcoded day as the test data, and assert expected output based on that static input

yes, did same in new PR.

mickmister · 2021-09-07T21:32:36Z

server/command.go

+		return responsef("We don't support re-queuing future items, only available for present and past items.")
+	}
+
+	hashtag, error := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())


Suggested change

hashtag, error := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())

hashtag, err := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())

mickmister · 2021-09-07T21:33:24Z

server/command.go

 	}
+	return &model.CommandResponse{Text: fmt.Sprintf("Item has been Re-queued to %v", hashtag), ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL}


We should use responsef here instead of &model.CommandResponse

sure, will update.

mickmister · 2021-09-07T21:36:52Z

server/command.go

+		prefix = matchGroups[1]
+		hashtagDateFormat = strings.TrimSpace(matchGroups[2])
+	} else {
+		return "", ParsedMeetingMessage{}, responsef("Error 267")


What is Error 267?

Will make it friendly @maisnamrajusingh @mickmister .

mickmister · 2021-09-07T21:37:25Z

server/command.go

+	}
+
+	var (
+		messageRegexFormat = regexp.MustCompile(fmt.Sprintf(`(?m)^#### #%s(?P<date>.*) ([0-9]+)\) (?P<message>.*)?$`, prefix))


Can this string be defined as a constant at the top of the file?

server/command.go

mickmister · 2021-09-07T21:42:31Z

I accidentally reviewed the code for requeueing items because that is the majority of the PR. Please remove any code not related to changing the existing meeting items.

hanzei

Blocking until the re-queue changes have been removed

sanjaydemansol · 2021-09-08T06:11:00Z

Blocking until the re-queue changes have been removed

Please follow sequence for reviewing these three PRs #66->#12->#47. There is one commit for each issue.

jfrerich · 2021-09-09T22:57:32Z

@sanjaydemansol, can you pull the code into 3 separate PRs, one for each issue? #66, #12, and #47.

jfrerich · 2021-09-09T23:00:49Z

Actually, I see the PRs have been separated per issue. The remaining work for this PR is to remove the code relative to PR #86 and #87

sanjaydemansol · 2021-09-10T02:51:46Z

Actually, I see the PRs have been separated per issue. The remaining work for this PR is to remove the code relative to PR #86 and #87

Actually #86 is the base for #87 and #87 is base for this PR(#88).

Co-authored-by: Michael Kochell <[email protected]>

dipak-demansol · 2021-09-28T05:15:57Z

@hanzei hii, you added the invalid label but i don't understand the label is for Dev or Qa, should i need to test this PR?

hanzei · 2021-09-28T13:20:01Z

@dipak-demansol No, there is nothing to test here

Sanjay added 3 commits July 13, 2021 16:47

Improve date format selection UI mattermost-community#66

3395a30

Fixes mattermost-community#12

752d468

fix mattermost-community#47

8f14f07

sanjaydemansol requested a review from jfrerich as a code owner August 31, 2021 04:58

Delete webapp/src/components/modals/channel_subscriptions directory

1cd0db5

hanzei added the 2: Dev Review Requires review by a core committer label Aug 31, 2021

jfrerich requested a review from mickmister August 31, 2021 14:36

jfrerich added the 3: QA Review Requires review by a QA tester label Aug 31, 2021

sanjaydemansol requested review from sibasankarnayak and maisnamrajusingh September 2, 2021 04:27

maisnamrajusingh reviewed Sep 3, 2021

View reviewed changes

maisnamrajusingh assigned sanjaydemansol Sep 3, 2021

sibasankarnayak reviewed Sep 3, 2021

View reviewed changes

server/command.go Show resolved Hide resolved

sibasankarnayak approved these changes Sep 3, 2021

View reviewed changes

mickmister requested changes Sep 7, 2021

View reviewed changes

mickmister requested a review from hanzei September 7, 2021 21:40

hanzei requested changes Sep 8, 2021

View reviewed changes

Update server/command.go

dd95544

Co-authored-by: Michael Kochell <[email protected]>

sanjaydemansol requested a review from mickmister September 13, 2021 02:11

sanjaydemansol mentioned this pull request Sep 14, 2021

[GH-66] Improve date format selection UI. Fixes #66 #86

Closed

sanjaydemansol closed this Sep 14, 2021

hanzei removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Sep 21, 2021

hanzei added the Invalid This doesn't seem right label Sep 21, 2021

sanjaydemansol mentioned this pull request Oct 25, 2021

[GH-12] Added Ability to re-queue agenda Items. #89

Closed

	hashtag, error := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())
	hashtag, err := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())

		}
		return &model.CommandResponse{Text: fmt.Sprintf("Item has been Re-queued to %v", hashtag), ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL}

[GH-47] [FIX] Agenda item number should get updated if the items of the agenda are deleted #88

[GH-47] [FIX] Agenda item number should get updated if the items of the agenda are deleted #88

Conversation

sanjaydemansol commented Aug 31, 2021

Summary

Testing

Ticket Link

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

maisnamrajusingh Sep 3, 2021 • 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

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

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

Choose a reason for hiding this comment

mickmister Oct 1, 2021 • 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

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

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 commented Sep 7, 2021

hanzei left a comment

Choose a reason for hiding this comment

sanjaydemansol commented Sep 8, 2021

jfrerich commented Sep 9, 2021

jfrerich commented Sep 9, 2021

sanjaydemansol commented Sep 10, 2021

dipak-demansol commented Sep 28, 2021

hanzei commented Sep 28, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading

maisnamrajusingh Sep 3, 2021 •

edited

Loading

mickmister Oct 1, 2021 •

edited

Loading