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

sanjaydemansol · 2021-09-17T06:03:08Z

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-09-17T06:06:00Z

Codecov Report

Merging #90 (c5abeee) into master (2f22484) will decrease coverage by 1.46%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   26.03%   24.56%   -1.47%     
==========================================
  Files           7        7              
  Lines         315      403      +88     
==========================================
+ Hits           82       99      +17     
- Misses        216      287      +71     
  Partials       17       17

Impacted Files	Coverage Δ
server/command.go	`0.00% <0.00%> (ø)`
server/meeting.go	`40.00% <0.00%> (-23.05%)`	⬇️
server/main.go	`0.00% <0.00%> (ø)`
server/manifest.go	`100.00% <0.00%> (ø)`
server/configuration.go	`0.00% <0.00%> (ø)`
server/utils.go	`95.12% <0.00%> (+1.18%)`	⬆️
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...c5abeee. Read the comment docs.

dipak-demansol · 2021-09-17T10:15:08Z

@sanjaydemansol LGTM

dipak-demansol · 2021-10-13T10:18:42Z

@sanjaydemansol i forgot to add the review so i will check it again and will add the review.

dipak-demansol

LGTM, Its Working as Expected.

dipak-demansol · 2021-10-18T18:27:26Z

@sanjaydemansol @hanzei This PR is Tested From my side but i don't have pull review request for this PR, can someone send me that.

mickmister

Great work @sanjaydemansol! I have some requests regarding error handling and writing unit tests

server/command.go

mickmister · 2021-10-20T02:50:14Z

server/command.go

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


I'm not verse enough in regex to gauge the stability of this. @hanzei @levb Can you take a look at this? This regex is meant to parse an agenda post's header.

@sanjaydemansol Can you please provide unit tests to assert that the code works under different circumstances?

Note that MustCompile will panic if the regex doesn't compile. Can we instead use Compile and check for the error? Probably a good idea to make sure we're doing this elsewhere as well

Added for my part, the other case is in the global scope.

server/command.go

mickmister

Thanks for implementing the changes @sanjaydemansol 👍 I have a few more comments

mickmister · 2021-12-02T04:33:21Z

server/command.go

-		return responsef("Error calculating list number")
+	numQueueItems, itemErr := calculateQueueItemNumberAndUpdateOldItems(meeting, args, p, hashtag)
+	if itemErr != nil {
+		return responsef(err.Error())


Suggested change

return responsef(err.Error())

return responsef(itemErr.Error())

Can we log this error as well?

Sure, it is done.

mickmister · 2021-12-02T04:36:17Z

server/command.go

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


Note that MustCompile will panic if the regex doesn't compile. Can we instead use Compile and check for the error? Probably a good idea to make sure we're doing this elsewhere as well

mickmister · 2021-12-02T04:37:39Z

server/meeting.go

@@ -61,6 +65,45 @@ func (p *Plugin) SaveMeeting(meeting *Meeting) error {
 	return nil
 }

+func calculateQueueItemNumberAndUpdateOldItems(meeting *Meeting, args *model.CommandArgs, p *Plugin, hashtag string) (int, error) {
+	searchResults, appErr := p.API.SearchPostsInTeamForUser(args.TeamId, args.UserId, model.SearchParameter{Terms: &hashtag})


We want to scope this post search to the current channel as well, not just the current team

Suggested change

searchResults, appErr := p.API.SearchPostsInTeamForUser(args.TeamId, args.UserId, model.SearchParameter{Terms: &hashtag})

c, appErr := p.API.GetChannel(args.ChannelId)

if appErr != nil {...}

terms := fmt.Sprintf("in:%s %s", c.Name, hashtag)

searchResults, appErr := p.API.SearchPostsInTeamForUser(args.TeamId, args.UserId, model.SearchParameter{Terms: &terms})

Also I think this method should be defined on Plugin instead of passing it in as a parameter

mickmister · 2021-12-02T04:49:20Z

server/meeting.go

+		counter++
+		if updateErr != nil {
+			return 0, errors.Wrap(updateErr, "Error updating post")
+		}


For readability, can we increment counter after the if block?

mickmister · 2021-12-02T04:51:21Z

server/meeting.go

+			Id:        post.Id,
+			UserId:    args.UserId,
+			ChannelId: args.ChannelId,
+			RootId:    args.RootId,


I don't think we should change the post author or root id here

Suggested change

Id: post.Id,

UserId: args.UserId,

ChannelId: args.ChannelId,

RootId: args.RootId,

Id: post.Id,

UserId: post.UserId,

ChannelId: post.ChannelId,

RootId: post.RootId,

Yeah, right. Done.

mickmister · 2021-12-02T04:52:06Z

server/meeting.go

+			return 0, errors.Wrap(updateErr, "Error updating post")
+		}
+	}
+	return counter, nil


Can we add a newline before this for readability? We basically want to always add a newline after the end of a loop or if block

mickmister · 2021-12-02T04:55:49Z

server/meeting.go

+			p.API.LogDebug(err.Error())
+			return 0, errors.New(err.Error())
+		}
+		_, updateErr := p.API.UpdatePost(&model.Post{


Please add a newline after the if block above

mickmister · 2021-12-02T04:56:55Z

server/command.go

+		}
+		return hashtagDateFormat, parsedMeetingMessage, nil
+	}
+	return hashtagDateFormat, ParsedMeetingMessage{}, errors.New("failed to parse meeting post's header")


Please add newlines here

mickmister

LGTM, just a few non-blocking suggestions

mickmister · 2021-12-13T20:52:22Z

server/command.go

+// ParsedMeetingMessage is meeting message after being parsed
+type ParsedMeetingMessage struct {
+	date        string
+	number      string // TODO we don't need it right now


For what feature will we remove this TODO comment?

I can't recall, I removed the comment.

mickmister · 2021-12-13T20:52:40Z

server/command.go

@@ -132,6 +140,11 @@ func (p *Plugin) executeCommandQueue(args *model.CommandArgs) *model.CommandResp
 		return responsef("Missing parameters for queue command")
 	}

+	meeting, err := p.GetMeeting(args.ChannelId)
+	if err != nil {
+		p.API.LogError("failed to get meeting for channel", "err", err.Error(), "channel_id:", args.ChannelId)


Suggested change

p.API.LogError("failed to get meeting for channel", "err", err.Error(), "channel_id:", args.ChannelId)

p.API.LogError("failed to get meeting for channel", "err", err.Error(), "channel_id", args.ChannelId)

mickmister · 2021-12-13T21:00:49Z

server/meeting.go

+	terms := fmt.Sprintf("in:%s %s", c.Name, hashtag)
+	searchResults, appErr := p.API.SearchPostsInTeamForUser(args.TeamId, args.UserId, model.SearchParameter{Terms: &terms})
+	if appErr != nil {
+		return 0, errors.Wrap(appErr, "Error calculating list number")


Suggested change

return 0, errors.Wrap(appErr, "Error calculating list number")

return 0, errors.Wrap(appErr, "Error searching posts to find hashtags")

mickmister · 2021-12-13T21:05:33Z

server/meeting.go

+		})
+
+		if updateErr != nil {
+			return 0, errors.Wrap(updateErr, "Error updating post")
+		}
+		counter++


Don't feel obligated to implement this suggestion, but the newlines are meant to separate different operations. Since this updateErr is logically coupled with the "block" above, we don't need the newline between them. The block above actually not even a block, it's a function call with a struct. If it was an actual loop/if statement then it would make since to insert a newline after it.

Suggestion: We can remove the newline above this if statement. But we should always have a newline after if statements and loops. This is because after the block is finished, we are done both checking the value and processing its conditional logic. We then move on to something unrelated, thus the newline helps signal this intention.

Suggested change

})

if updateErr != nil {

return 0, errors.Wrap(updateErr, "Error updating post")

}

counter++

})

if updateErr != nil {

return 0, errors.Wrap(updateErr, "Error updating post")

}

counter++

@mickmister Thank you so much for highlighting that; I understood. Newlines are finally demystified for me.😊

hanzei · 2021-12-14T15:57:57Z

@dipak-demansol This is ready for review

dipak-demansol

LGTM Tested it in the Chat and Thread Section.

fix mattermost-community#47

9cab630

sanjaydemansol requested a review from jfrerich as a code owner September 17, 2021 06:03

sanjaydemansol requested review from maisnamrajusingh, sibasankarnayak and dipak-demansol September 17, 2021 06:07

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

dipak-demansol added the QA Review Done PR has been approved by QA label Sep 27, 2021

dipak-demansol approved these changes Oct 14, 2021

View reviewed changes

mickmister added this to the v0.3.0 milestone Oct 19, 2021

mickmister self-requested a review October 19, 2021 05:31

mickmister requested changes Oct 20, 2021

View reviewed changes

sibasankarnayak approved these changes Nov 2, 2021

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Nov 30, 2021

sanjaydemansol added 2 commits December 1, 2021 15:24

changes based on feedback

26e94ba

changes based on feedback

caa0604

sanjaydemansol removed the Awaiting Submitter Action Blocked on the author label Dec 1, 2021

hanzei requested a review from mickmister December 1, 2021 10:11

hanzei removed the QA Review Done PR has been approved by QA label Dec 1, 2021

mickmister requested changes Dec 2, 2021

View reviewed changes

hanzei added the 3: QA Review Requires review by a QA tester label Dec 7, 2021

feedback updates

7ac5c26

hanzei requested a review from mickmister December 13, 2021 19:28

mickmister approved these changes Dec 13, 2021

View reviewed changes

more feedback updates

c5abeee

hanzei requested a review from dipak-demansol December 14, 2021 15:57

hanzei removed the 2: Dev Review Requires review by a core committer label Dec 14, 2021

dipak-demansol approved these changes Dec 15, 2021

View reviewed changes

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 15, 2021

hanzei removed request for jfrerich and maisnamrajusingh December 15, 2021 10:27

hanzei approved these changes Dec 15, 2021

View reviewed changes

hanzei merged commit bc0e65d into mattermost-community:master Dec 15, 2021

	return responsef(err.Error())
	return responsef(itemErr.Error())

	p.API.LogError("failed to get meeting for channel", "err", err.Error(), "channel_id:", args.ChannelId)
	p.API.LogError("failed to get meeting for channel", "err", err.Error(), "channel_id", args.ChannelId)

	return 0, errors.Wrap(appErr, "Error calculating list number")
	return 0, errors.Wrap(appErr, "Error searching posts to find hashtags")

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

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

Conversation

sanjaydemansol commented Sep 17, 2021

Summary

Testing

Ticket Link

codecov bot commented Sep 17, 2021 • edited Loading

Codecov Report

dipak-demansol commented Sep 17, 2021

dipak-demansol commented Oct 13, 2021

dipak-demansol left a comment

Choose a reason for hiding this comment

dipak-demansol commented Oct 18, 2021

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

mickmister left a comment • edited by hanzei 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

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

sanjaydemansol Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

hanzei commented Dec 14, 2021

dipak-demansol left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2021 •

edited

Loading

mickmister left a comment •

edited by hanzei

Loading

sanjaydemansol Dec 14, 2021 •

edited

Loading