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 all commits
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
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ require (
github.com/mattermost/mattermost-server/v5 v5.3.2-0.20200723144633-ed34468996e6
github.com/pkg/errors v0.9.1
github.com/stretchr/testify v1.6.1
github.com/undefinedlabs/go-mpatch v1.0.6
)
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,8 @@ github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGr
github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY=
github.com/undefinedlabs/go-mpatch v1.0.6 h1:h8q5ORH/GaOE1Se1DMhrOyljXZEhRcROO7agMqWXCOY=
github.com/undefinedlabs/go-mpatch v1.0.6/go.mod h1:TyJZDQ/5AgyN7FSLiBJ8RO9u2c6wbtRvK827b6AVqY4=
github.com/urfave/negroni v1.0.0/go.mod h1:Meg73S6kFm/4PpbYdq35yYWoCZ9mS/YSx+lKnmiohz4=
github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc=
github.com/valyala/fasthttp v1.6.0/go.mod h1:FstJa9V+Pj9vQ7OJie2qMHdwemEDaDiSdBnvPM1Su9w=
Expand Down
100 changes: 98 additions & 2 deletions server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,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 @@ -77,7 +80,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 @@ -160,7 +163,7 @@ func (p *Plugin) executeCommandQueue(args *model.CommandArgs) *model.CommandResp
message = strings.Join(split[3:], " ")
}

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. Check the meeting settings for this channel.")
}
Expand All @@ -183,6 +186,99 @@ 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("There was no meeting found for this channel.")
}

oldPostID := split[2]
postToBeReQueued, appErr := p.API.GetPost(oldPostID)
if appErr != nil {
p.API.LogWarn("Error fetching post: %s", "error", appErr.Message)
return responsef("Error fetching post.")
}
var (
prefix string
hashtagDateFormat string
)
matchGroups := meetingDateFormatRegex.FindStringSubmatch(meeting.HashtagFormat)
if len(matchGroups) != 3 {
responsef("Error parsing hashtag format.")
}
prefix = matchGroups[1]
hashtagDateFormat = strings.TrimSpace(matchGroups[2])

var (
messageRegexFormat, er = regexp.Compile(fmt.Sprintf(`(?m)^#### #%s(?P<date>.*) [0-9]+\) (?P<message>.*)?$`, prefix))
)
if er != nil {
return responsef(er.Error())
}

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 := p.replaceUnderscoreWithSpace(strings.TrimSpace(matchGroups[1])) // reverse what we do to make it a valid hashtag
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-queuing future items is not supported.")
}

hashtag, err := p.GenerateHashtag(args.ChannelId, false, -1, true, formattedDate.Weekday())
if err != nil {
p.API.LogWarn("Error calculating hashtags. Check the meeting settings for this channel.", "error", err.Error())
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

}

commandResponse, numQueueItems := calculateQueItemNumber(args, p, hashtag)
if commandResponse != nil {
return 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 {
p.API.LogWarn("Error creating post: %s", "error", appErr.Message)
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.")
}

func (p *Plugin) replaceUnderscoreWithSpace(hashtag string) string {
return strings.ReplaceAll(hashtag, "_", " ")
}

func parseMeetingPost(meeting *Meeting, post *model.Post) (string, ParsedMeetingMessage, error) {
var (
prefix string
Expand Down
14 changes: 10 additions & 4 deletions server/meeting.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (p *Plugin) calculateQueueItemNumberAndUpdateOldItems(meeting *Meeting, arg
}

// 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 @@ -126,9 +126,15 @@ 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 specify any specific date/day in command, Get date for the list of days of the week
if !requeue {
if meetingDate, err = nextWeekdayDateInWeek(meeting.Schedule, nextWeek); err != nil {
return "", err
}
} else {
if meetingDate, err = nextWeekdayDateInWeekSkippingDay(meeting.Schedule, nextWeek, assignedDay); err != nil {
return "", err
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion server/meeting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,10 @@ 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)
weekday := -1
requeue := false
assignedDay := time.Weekday(1)
got, err := mPlugin.GenerateHashtag(tt.args.meeting.ChannelID, tt.args.nextWeek, weekday, requeue, assignedDay)
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
42 changes: 42 additions & 0 deletions server/utils_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package main

import (
"reflect"
"testing"
"time"

"github.com/undefinedlabs/go-mpatch"
)

func Test_parseSchedule(t *testing.T) {
Expand Down Expand Up @@ -41,3 +44,42 @@ func Test_parseSchedule(t *testing.T) {
})
}
}

func Test_nextWeekdayDateInWeekSkippingDay(t *testing.T) {
var patch, err = mpatch.PatchMethod(time.Now, func() time.Time {
return time.Date(2021, 11, 15, 00, 00, 00, 0, time.UTC)
})
Comment on lines +49 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we would pass in the value for now in the function we're testing this with, rather than implicitly patching the value here. @sanjaydemansol What do you think?

We would modify nextWeekdayDateInWeekSkippingDay to accept now as a parameter:

now, _ := time.Date(2021, 11, 15, 00, 00, 00, 0, time.UTC)

got, err := nextWeekdayDateInWeekSkippingDay(now, tt.args.meetingDays, tt.args.nextWeek, tt.args.dayToSkip)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use now for past and future cases hence, we should be fine with patching.

if patch == nil || err != nil {
t.Errorf("error creating patch")
}
type args struct {
meetingDays []time.Weekday
nextWeek bool
dayToSkip time.Weekday
}
tests := []struct {
name string
args args
want time.Time
wantErr bool
}{

{name: "test skip tuesday in week, today", args: args{[]time.Weekday{1, 2, 3, 4, 5, 6, 7}, false, time.Weekday(2)}, want: time.Now().AddDate(0, 0, 0), wantErr: false},
{name: "test skip tuesday in few days", args: args{[]time.Weekday{2, 3, 4, 5, 6, 7}, false, time.Weekday(2)}, want: time.Now().AddDate(0, 0, 2), wantErr: false},
{name: "test skip monday with nextWeek true", args: args{[]time.Weekday{1, 2, 3, 4}, true, time.Weekday(1)}, want: time.Now().AddDate(0, 0, 8), wantErr: false},
{name: "test only meeting day is skipped", args: args{[]time.Weekday{3}, false, time.Weekday(3)}, want: time.Now().AddDate(0, 0, 2), wantErr: false},
{name: "test only meeting day is skipped with nextWeek true", args: args{[]time.Weekday{3}, true, time.Weekday(3)}, want: time.Now().AddDate(0, 0, 9), wantErr: false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := nextWeekdayDateInWeekSkippingDay(tt.args.meetingDays, tt.args.nextWeek, tt.args.dayToSkip)
if (err != nil) != tt.wantErr {
t.Errorf("nextWeekdayDateInWeekSkippingDay() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, &tt.want) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also work?

Suggested change
if !reflect.DeepEqual(got, &tt.want) {
require.Equal(t, got, &tt.want) {

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 tested code added require.Equal(t, got, &tt.want) {, not working. test cases failed.

t.Errorf("nextWeekdayDateInWeekSkippingDay() got = %v, want %v", got, tt.want)
}
})
}
}
29 changes: 27 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,28 @@ 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(postId) {
return async (dispatch, getState) => {
const command = `/agenda requeue post ${postId}`;
await clientExecuteCommand(dispatch, getState, command);
return {data: true};
};
}

export async function clientExecuteCommand(dispatch, getState, command) {
const state = getState();
const currentChannel = getCurrentChannel(state);
const currentTeamId = getCurrentTeamId(state);
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', (postId) => {
store.dispatch(requeueItem(postId));
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use post.props to:

  • declare a given post was made with the agenda plugin
  • and check here to see if we should render the Re-queue item in the post dropdown menu

});
}
}

Expand Down