Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 89 additions & 11 deletions server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"fmt"
"regexp"
"strings"
"time"

Expand Down Expand Up @@ -49,6 +50,9 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*mo
case "queue":
return p.executeCommandQueue(args), nil

case "requeue":
return p.executeCommandReQueue(args), nil

case "setting":
return p.executeCommandSetting(args), nil

Expand All @@ -69,7 +73,7 @@ func (p *Plugin) executeCommandList(args *model.CommandArgs) *model.CommandRespo
weekday = int(parsedWeekday)
}

hashtag, err := p.GenerateHashtag(args.ChannelId, nextWeek, weekday)
hashtag, err := p.GenerateHashtag(args.ChannelId, nextWeek, weekday, false, time.Now().Weekday())
if err != nil {
return responsef("Error calculating hashtags")
}
Expand Down Expand Up @@ -147,25 +151,21 @@ func (p *Plugin) executeCommandQueue(args *model.CommandArgs) *model.CommandResp
message = strings.Join(split[3:], " ")
}

hashtag, error := p.GenerateHashtag(args.ChannelId, nextWeek, weekday)
hashtag, error := p.GenerateHashtag(args.ChannelId, nextWeek, weekday, false, time.Now().Weekday())
if error != nil {
return responsef("Error calculating hashtags. Check the meeting settings for this channel.")
}

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

if appErr != nil {
return responsef("Error calculating list number")
itemErr, numQueueItems := calculateQueItemNumber(args, p, hashtag)
if itemErr != nil {
return itemErr
}

postList := *searchResults.PostList
numQueueItems := len(postList.Posts)

_, appErr = p.API.CreatePost(&model.Post{
_, appErr := p.API.CreatePost(&model.Post{
UserId: args.UserId,
ChannelId: args.ChannelId,
RootId: args.RootId,
Message: fmt.Sprintf("#### %v %v) %v", hashtag, numQueueItems+1, message),
Message: fmt.Sprintf("#### %v %v) %v", hashtag, numQueueItems, message),
})
if appErr != nil {
return responsef("Error creating post: %s", appErr.Message)
Expand All @@ -174,6 +174,84 @@ func (p *Plugin) executeCommandQueue(args *model.CommandArgs) *model.CommandResp
return &model.CommandResponse{}
}

func calculateQueItemNumber(args *model.CommandArgs, p *Plugin, hashtag string) (*model.CommandResponse, int) {
searchResults, appErr := p.API.SearchPostsInTeamForUser(args.TeamId, args.UserId, model.SearchParameter{Terms: &hashtag})
if appErr != nil {
return responsef("Error calculating list number"), 0
}
postList := *searchResults.PostList
numQueueItems := len(postList.Posts)
return nil, numQueueItems + 1
}

func (p *Plugin) executeCommandReQueue(args *model.CommandArgs) *model.CommandResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot going on in this function. Can we pull some of the logic out to be unit tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah absolutely, will refactor it.

Copy link
Contributor

@mickmister mickmister Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this function was refactored, but we agreed it should be refactored and tested in this conversation

split := strings.Fields(args.Command)

if len(split) <= 2 {
return responsef("Missing parameters for requeue command")
}

meeting, err := p.GetMeeting(args.ChannelId)
if err != nil {
return responsef("Can't find the meeting")
mickmister marked this conversation as resolved.
Show resolved Hide resolved
}

oldPostID := split[2]
postToBeReQueued, _ := p.API.GetPost(oldPostID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check for an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var (
prefix string
hashtagDateFormat string
)
if matchGroups := meetingDateFormatRegex.FindStringSubmatch(meeting.HashtagFormat); len(matchGroups) == 4 {
prefix = matchGroups[1]
hashtagDateFormat = strings.TrimSpace(matchGroups[2])
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define this regex at the top of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5, not optimal since we substitute another value in regex here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use regexp.Compile instead of MustCompile so we can handle the error and avoid a panic. As well as anywhere else we are calling MustCompile inside of a function.

)

if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid this large indented block here

Suggested change
if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {
matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message)
if len(matchGroups) != 3 {

Then return an error in that if block, and continue on otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that this is not done

originalPostDate := strings.ReplaceAll(strings.TrimSpace(matchGroups[1]), "_", " ") // reverse what we do to make it a valid hashtag
Copy link
Contributor

@mickmister mickmister Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the "mutation" operations like this in its own function? Same with the operation that is the reverse of this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

originalPostMessage := strings.TrimSpace(matchGroups[2])

today := time.Now()
local, _ := time.LoadLocation("Local")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Local what's used elsewhere in this plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is usually used instead, or is the only instance of LoadLocation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanjaydemansol Please see my question above:

What is usually used instead, or is the only instance of LoadLocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No localized methods are used anywhere. removing LoadLocation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time.LoadLocation is stored in local and is used here at.
formattedDate, _ := time.ParseInLocation(hashtagDateFormat, originalPostDate, local)

formattedDate, _ := time.ParseInLocation(hashtagDateFormat, originalPostDate, local)
if formattedDate.Year() == 0 {
thisYear := today.Year()
formattedDate = formattedDate.AddDate(thisYear, 0, 0)
}

if today.Year() <= formattedDate.Year() && today.YearDay() < formattedDate.YearDay() {
return responsef("Re-queing future items are not supported.")
mickmister marked this conversation as resolved.
Show resolved Hide resolved
}

hashtag, err := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())
if err != nil {
return responsef("Error calculating hashtags. Check the meeting settings for this channel.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should be logged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

itemErr, numQueueItems := calculateQueItemNumber(args, p, hashtag)
if itemErr != nil {
return itemErr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return itemErr
return errors.Wrap(itemErr, "failed to calculate queue item number")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our plugin doesn't support returning this as HTTP response.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that itemErr seems like it should be an error but it's a *model.CommandResponse. Maybe we can rename the variable commandResponse?

}

_, appErr := p.API.UpdatePost(&model.Post{
Id: oldPostID,
UserId: args.UserId,
ChannelId: args.ChannelId,
RootId: args.RootId,
Message: fmt.Sprintf("#### %v %v) %v", hashtag, numQueueItems, originalPostMessage),
})
if appErr != nil {
return responsef("Error creating post: %s", appErr.Message)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should be logged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
return responsef(fmt.Sprintf("Item has been Re-queued to %v", hashtag))
}
return responsef("Make sure, message is in required format!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances would this occur? We definitely don't want to yell at the user 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced ! with . .It occurs if Hashtag is not correctly formatted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5 it should be worded as

Please make sure the agenda hashtag is formatted properly.

}

func (p *Plugin) executeCommandHelp(args *model.CommandArgs) *model.CommandResponse {
return responsef(helpCommandText)
}
Expand Down
17 changes: 13 additions & 4 deletions server/meeting.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (p *Plugin) SaveMeeting(meeting *Meeting) error {
}

// GenerateHashtag returns a meeting hashtag
func (p *Plugin) GenerateHashtag(channelID string, nextWeek bool, weekday int) (string, error) {
func (p *Plugin) GenerateHashtag(channelID string, nextWeek bool, weekday int, requeue bool, assignedDay time.Weekday) (string, error) {
meeting, err := p.GetMeeting(channelID)
if err != nil {
return "", err
Expand All @@ -75,9 +75,18 @@ func (p *Plugin) GenerateHashtag(channelID string, nextWeek bool, weekday int) (
return "", err
}
} else {
// Get date for the list of days of the week
if meetingDate, err = nextWeekdayDateInWeek(meeting.Schedule, nextWeek); err != nil {
return "", err
// user didn't provide any specific date, Get date for the list of days of the week
mickmister marked this conversation as resolved.
Show resolved Hide resolved
if !requeue {
if meetingDate, err = nextWeekdayDateInWeek(meeting.Schedule, nextWeek); err != nil {
return "", err
}
} else {
if len(meeting.Schedule) == 1 && meeting.Schedule[0] == assignedDay { // if this day is the only day selected in settings
mickmister marked this conversation as resolved.
Show resolved Hide resolved
nextWeek = true
}
if meetingDate, err = nextWeekdayDateInWeekSkippingDay(meeting.Schedule, nextWeek, assignedDay); err != nil {
return "", err
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion server/meeting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, 1)
mickmister marked this conversation as resolved.
Show resolved Hide resolved
if (err != nil) != tt.wantErr {
t.Errorf("GenerateHashtag() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
19 changes: 19 additions & 0 deletions server/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,25 @@ func nextWeekdayDateInWeek(meetingDays []time.Weekday, nextWeek bool) (*time.Tim
return nextWeekdayDate(meetingDay, nextWeek)
}

func nextWeekdayDateInWeekSkippingDay(meetingDays []time.Weekday, nextWeek bool, dayToSkip time.Weekday) (*time.Time, error) {
if len(meetingDays) == 0 {
return nil, errors.New("missing weekdays to calculate date")
}

todayWeekday := time.Now().Weekday()

// Find which meeting weekday to calculate the date for
meetingDay := meetingDays[0]
for _, day := range meetingDays {
if todayWeekday <= day && day != dayToSkip {
meetingDay = day
break
}
}
Comment on lines +83 to +89
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write unit tests for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure


return nextWeekdayDate(meetingDay, nextWeek)
}

// nextWeekdayDate calculates the date of the next given weekday
// from today's date.
// If nextWeek is true, it will be based on the next calendar week.
Expand Down
28 changes: 26 additions & 2 deletions webapp/src/actions/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {searchPostsWithParams} from 'mattermost-redux/actions/search';

import {getCurrentChannel} from 'mattermost-redux/selectors/entities/channels';
import {getCurrentTeamId} from 'mattermost-redux/selectors/entities/teams';
import {getConfig} from 'mattermost-redux/selectors/entities/general';
import {Client4} from 'mattermost-redux/client';

import Client from '../client';

Expand Down Expand Up @@ -80,4 +81,27 @@ export function performSearch(terms) {

return dispatch(searchPostsWithParams(teamId, {terms, is_or_search: false, include_deleted_channels: viewArchivedChannels, page: 0, per_page: 20}, true));
};
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is itemId a post id? If so, it seems like having this requeue slash command doesn't make much sense. 1/5 we should implement this as an HTTP endpoint instead, using the server-side ServeHTTP hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister we had a discussion for same at #88 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the slash command makes sense then. Maybe we change this to /agenda requeue post ${postId}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied over from #88 (comment):

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

await clientExecuteCommand(dispatch, getState, command);
return {data: true};
};
}

export async function clientExecuteCommand(dispatch, getState, command) {
const currentChannel = getCurrentChannel(getState());
const currentTeamId = getCurrentTeamId(getState());
mickmister marked this conversation as resolved.
Show resolved Hide resolved
const args = {
channel_id: currentChannel?.id,
team_id: currentTeamId,
};

try {
return Client4.executeCommand(command, args);
} catch (error) {
return error;
}
}
6 changes: 5 additions & 1 deletion webapp/src/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal} from './actions';
import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal, requeueItem} from './actions';

import reducer from './reducer';

Expand All @@ -19,6 +19,10 @@ export default class Plugin {
(channelId) => {
store.dispatch(openMeetingSettingsModal(channelId));
});

registry.registerPostDropdownMenuAction('Re-queue', (itemId) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registerPostDropdownMenuAction supports a third argument, a callback that returns boolean representing if we should show this action in the post menu. We should use this callback as an opportunity to check if the post is an agenda item post. If we determine that it is not an agenda item post, we should return false in the callback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We possibly want to use an identifier in post.props to signal that it's an agenda post. But that wouldn't work with manually created agenda items.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5, for now we could ensure we only show Re-queue option if a post starts with an #. Your thoughts? @mickmister thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5, for now we could ensure we only show Re-queue option if a post starts with an #

I think this is a little too broad of a claim. Two reasons come to mind:

  1. Posts can start with # because they are using a header, and not a hash tag. For example, # I'm a header renders as:

I'm a header

  1. Hashtags don't imply that it's an agenda hashtag.

I think we should go with the post.props strategy. If someone wants to use the post menu to re-queue an item they created manually, too bad. cc @hanzei

Another option going along with this same strategy is for the server-side of the plugin to apply the post.props mutation to the manually created agenda items during the MessageWillBePosted and MessageWillBeUpdated hooks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for using post.props

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mickmister @hanzei , nice approach.
Should I implement it in this PR or create a separate issue?

Copy link
Contributor

@mickmister mickmister Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1/5 I'm thinking we should add it to this PR. @hanzei thoughts? I would want the change to block the release anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanjaydemansol Note that the PR should be using post.props to determine if the post is created by the plugin

store.dispatch(requeueItem(itemId));
mickmister marked this conversation as resolved.
Show resolved Hide resolved
});
}
}

Expand Down