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.

Sign up for GitHub

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

Fix(Issue#202): counts in one request #225

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix(Issue#202): counts in one request #225

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bdb189c
feat: add count ressource for counter
jufab Oct 26, 2023
80d96b7
chore: add count to update or add list
jufab Oct 26, 2023
49f7ffc
fix: review
jufab Oct 31, 2023
e822f51
fix: count to fetchIssueCounts
jufab Oct 31, 2023
6624565
chore: review count in one request and lists
jufab Nov 2, 2023
cd53a4b
fix: review
jufab Nov 7, 2023
2d27842
fix: remove useless refreshAll
jufab Nov 7, 2023
99b959c
fix: new review
jufab Nov 9, 2023
046d508
Merge branch 'master' into issue-202
raghavaggarwal2308 Jul 18, 2024
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
7 changes: 7 additions & 0 deletions server/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ type ExtendedIssue struct {
ForeignPosition int `json:"position"`
}

// ListsIssue for all list issues
type ListsIssue struct {
In []*ExtendedIssue `json:"in"`
My []*ExtendedIssue `json:"my"`
Out []*ExtendedIssue `json:"out"`
}

func newIssue(message string, description, postID string) *Issue {
return &Issue{
ID: model.NewId(),
Expand Down
22 changes: 20 additions & 2 deletions server/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ type ListStore interface {
PopReference(userID, listID string) (*IssueRef, error)
// BumpReference moves the Issue reference for issueID in listID for userID to the beginning of the list
BumpReference(userID, issueID, listID string) error

// GetIssueReference gets the IssueRef and position of the issue issueID on user userID's list listID
GetIssueReference(userID, issueID, listID string) (*IssueRef, int, error)
// GetIssueListAndReference gets the issue list, IssueRef and position for user userID
GetIssueListAndReference(userID, issueID string) (string, *IssueRef, int)

// GetList returns the list of IssueRef in listID for userID
GetList(userID, listID string) ([]*IssueRef, error)
}
Expand Down Expand Up @@ -135,6 +133,26 @@ func (l *listManager) GetIssueList(userID, listID string) ([]*ExtendedIssue, err
return extendedIssues, nil
}

func (l *listManager) GetAllList(userID string) (listsIssue *ListsIssue, err error) {
inListIssue, err := l.GetIssueList(userID, InListKey)
if err != nil {
return nil, err
}
myListIssue, err := l.GetIssueList(userID, MyListKey)
if err != nil {
return nil, err
}
outListIssue, err := l.GetIssueList(userID, OutListKey)
if err != nil {
return nil, err
}
return &ListsIssue{
In: inListIssue,
My: myListIssue,
Out: outListIssue,
}, nil
}

func (l *listManager) CompleteIssue(userID, issueID string) (issue *Issue, foreignID string, listToUpdate string, err error) {
issueList, ir, _ := l.store.GetIssueListAndReference(userID, issueID)
if ir == nil {
Expand Down
140 changes: 78 additions & 62 deletions server/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
SendIssue(senderID, receiverID, message, description, postID string) (string, error)
// GetIssueList gets the todos on listID for userID
GetIssueList(userID, listID string) ([]*ExtendedIssue, error)
// GetAllList get all issues
GetAllList(userID string) (*ListsIssue, error)
// CompleteIssue completes the todo issueID for userID, and returns the issue and the foreign ID if any
CompleteIssue(userID, issueID string) (issue *Issue, foreignID string, listToUpdate string, err error)
// AcceptIssue moves one the todo issueID of userID from inbox to myList, and returns the message and the foreignUserID if any
Expand Down Expand Up @@ -115,7 +117,7 @@
p.router.Use(p.withRecovery)

p.router.HandleFunc("/add", p.checkAuth(p.handleAdd)).Methods(http.MethodPost)
p.router.HandleFunc("/list", p.checkAuth(p.handleList)).Methods(http.MethodGet)
p.router.HandleFunc("/lists", p.checkAuth(p.handleLists)).Methods(http.MethodGet)
p.router.HandleFunc("/remove", p.checkAuth(p.handleRemove)).Methods(http.MethodPost)
p.router.HandleFunc("/complete", p.checkAuth(p.handleComplete)).Methods(http.MethodPost)
p.router.HandleFunc("/accept", p.checkAuth(p.handleAccept)).Methods(http.MethodPost)
Expand Down Expand Up @@ -168,8 +170,9 @@

telemetryRequest, err := GetTelemetryPayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get telemetry payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get telemetry payload from JSON.", err)
msg := "Unable to get telemetry payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
Comment on lines +181 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I was unclear about my request before. We shouldn't edit the log statements in functions unrelated to this PR's purpose. We can clean this up in a separate effort/PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry too. should I put everything back the way it was before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes everything that is not related to the main feature/effort of the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we do change the logging back the way it was for places unrelated the purpose of the PR, but I don't want to drag this PR on for that reason

return
}

Expand All @@ -188,8 +191,9 @@

addRequest, err := GetAddIssuePayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get add issue payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get add issue payload from JSON.", err)
msg := "Unable to get add issue payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -203,8 +207,9 @@
if addRequest.SendTo == "" {
_, err = p.listManager.AddIssue(userID, addRequest.Message, addRequest.Description, addRequest.PostID)
if err != nil {
p.API.LogError("Unable to add issue err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to add issue", err)
msg := "Unable to add issue"

Check failure on line 210 in server/plugin.go

View workflow job for this annotation

GitHub Actions / plugin-ci / lint 

string `Unable to add issue` has 2 occurrences, make it a constant (goconst)
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -220,16 +225,18 @@

receiver, appErr := p.API.GetUserByUsername(addRequest.SendTo)
if appErr != nil {
p.API.LogError("username not valid, err=" + appErr.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to find user", appErr)
msg := "Unable to find user"
p.API.LogError(msg, "err", err.Error())

Check failure on line 229 in server/plugin.go

View workflow job for this annotation

GitHub Actions / plugin-ci / lint 

nilness: nil dereference in dynamic method call (govet)
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

if receiver.Id == userID {
_, err = p.listManager.AddIssue(userID, addRequest.Message, addRequest.Description, addRequest.PostID)
if err != nil {
p.API.LogError("Unable to add issue err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to add issue", err)
msg := "Unable to add issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -255,8 +262,9 @@

issueID, err := p.listManager.SendIssue(userID, receiver.Id, addRequest.Message, addRequest.Description, addRequest.PostID)
if err != nil {
p.API.LogError("Unable to send issue err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to send issue", err)
msg := "Unable to send issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -281,31 +289,24 @@
}
}

func (p *Plugin) handleList(w http.ResponseWriter, r *http.Request) {
func (p *Plugin) handleLists(w http.ResponseWriter, r *http.Request) {
userID := r.Header.Get("Mattermost-User-ID")

listInput := r.URL.Query().Get("list")
listID := MyListKey
switch listInput {
case OutFlag:
listID = OutListKey
case InFlag:
listID = InListKey
}

issues, err := p.listManager.GetIssueList(userID, listID)
allListIssue, err := p.listManager.GetAllList(userID)
if err != nil {
p.API.LogError("Unable to get issues for user err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to get issues for user", err)
msg := "Unable to get issues for user"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

if len(issues) > 0 && r.URL.Query().Get("reminder") == "true" && p.getReminderPreference(userID) {
if allListIssue != nil && len(allListIssue.My) > 0 && r.URL.Query().Get("reminder") == "true" && p.getReminderPreference(userID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I think we are changing the logic here, but I think we are actually fixing a bug. LGTM 👍

var lastReminderAt int64
lastReminderAt, err = p.getLastReminderTimeForUser(userID)
if err != nil {
p.API.LogError("Unable to send reminder err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to send reminder", err)
msg := "Unable to send reminder"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -318,7 +319,7 @@
nt := time.Unix(now/1000, 0).In(timezone)
lt := time.Unix(lastReminderAt/1000, 0).In(timezone)
if nt.Sub(lt).Hours() >= 1 && (nt.Day() != lt.Day() || nt.Month() != lt.Month() || nt.Year() != lt.Year()) {
p.PostBotDM(userID, "Daily Reminder:\n\n"+issuesListToString(issues))
p.PostBotDM(userID, "Daily Reminder:\n\n"+issuesListToString(allListIssue.My))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is a good bug fix here too. Nice work 👍

p.trackDailySummary(userID)
err = p.saveLastReminderTimeForUser(userID)
if err != nil {
Expand All @@ -327,14 +328,15 @@
}
}

issuesJSON, err := json.Marshal(issues)
allListIssueJSON, err := json.Marshal(allListIssue)
if err != nil {
p.API.LogError("Unable marhsal issues list to json err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable marhsal issues list to json", err)
msg := "Unable marhsal all lists issues to json"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

_, err = w.Write(issuesJSON)
_, err = w.Write(allListIssueJSON)
if err != nil {
p.API.LogError("Unable to write json response err=" + err.Error())
}
Expand All @@ -345,8 +347,9 @@

editRequest, err := GetEditIssuePayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get edit issue payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get edit issue payload from JSON.", err)
msg := "Unable to get edit issue payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -357,8 +360,9 @@

foreignUserID, list, oldMessage, err := p.listManager.EditIssue(userID, editRequest.ID, editRequest.Message, editRequest.Description)
if err != nil {
p.API.LogError("Unable to edit message: err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to edit issue", err)
msg := "Unable to edit message"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -385,8 +389,9 @@

changeRequest, err := GetChangeAssignmentPayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get change request payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get change request from JSON.", err)
msg := "Unable to get change request payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -397,15 +402,17 @@

receiver, appErr := p.API.GetUserByUsername(changeRequest.SendTo)
if appErr != nil {
p.API.LogError("username not valid, err=" + appErr.Error())
p.handleErrorWithCode(w, http.StatusNotFound, "Unable to find user", appErr)
msg := "username not valid"
p.API.LogError(msg, "err", err.Error())

Check failure on line 406 in server/plugin.go

View workflow job for this annotation

GitHub Actions / plugin-ci / lint 

nilness: nil dereference in dynamic method call (govet)
p.handleErrorWithCode(w, http.StatusNotFound, msg, err)
return
}

issueMessage, oldOwner, err := p.listManager.ChangeAssignment(changeRequest.ID, userID, receiver.Id)
if err != nil {
p.API.LogError("Unable to change the assignment of an issue: err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to change the assignment", err)
msg := "Unable to change the assignment of an issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -431,8 +438,9 @@

acceptRequest, err := GetAcceptRequestPayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get accept request payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get accept request from JSON.", err)
msg := "Unable to get accept request payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -443,8 +451,9 @@

todoMessage, sender, err := p.listManager.AcceptIssue(userID, acceptRequest.ID)
if err != nil {
p.API.LogError("Unable to accept issue err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to accept issue", err)
msg := "Unable to accept issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -463,8 +472,9 @@

completeRequest, err := GetCompleteIssuePayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get complete issue request payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get complete issue request from JSON.", err)
msg := "Unable to get complete issue request payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -475,8 +485,9 @@

issue, foreignID, listToUpdate, err := p.listManager.CompleteIssue(userID, completeRequest.ID)
if err != nil {
p.API.LogError("Unable to complete issue err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to complete issue", err)
msg := "Unable to complete issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand All @@ -503,8 +514,9 @@

removeRequest, err := GetRemoveIssuePayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get remove issue request payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get remove issue request from JSON.", err)
msg := "Unable to get remove issue request payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -515,8 +527,9 @@

issue, foreignID, isSender, listToUpdate, err := p.listManager.RemoveIssue(userID, removeRequest.ID)
if err != nil {
p.API.LogError("Unable to remove issue, err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to remove issue", err)
msg := "Unable to remove issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}
p.sendRefreshEvent(userID, []string{listToUpdate})
Expand Down Expand Up @@ -549,8 +562,9 @@

bumpRequest, err := GetBumpIssuePayloadFromJSON(r.Body)
if err != nil {
p.API.LogError("Unable to get bump issue request payload from JSON err=" + err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to get bump issue request from JSON.", err)
msg := "Unable to get bump issue request payload from JSON"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)
return
}

Expand All @@ -561,8 +575,9 @@

todoMessage, foreignUser, foreignIssueID, err := p.listManager.BumpIssue(userID, bumpRequest.ID)
if err != nil {
p.API.LogError("Unable to bump issue, err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to bump issue", err)
msg := "Unable to bump issue"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand Down Expand Up @@ -596,8 +611,9 @@

configJSON, err := json.Marshal(clientConfig)
if err != nil {
p.API.LogError("Unable to marshal plugin configuration to json err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to marshal plugin configuration to json", err)
msg := "Unable to marshal plugin configuration to json"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)
return
}

Expand Down
4 changes: 1 addition & 3 deletions webapp/src/action_types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ export const GET_ASSIGNEE = pluginId + '_get_assignee';
export const SET_EDITING_TODO = pluginId + '_set_editing_todo';
export const REMOVE_EDITING_TODO = pluginId + '_remove_editing_todo';
export const REMOVE_ASSIGNEE = pluginId + '_remove_assignee';
export const GET_ISSUES = pluginId + '_get_issues';
export const GET_OUT_ISSUES = pluginId + '_get_out_issues';
export const GET_IN_ISSUES = pluginId + '_get_in_issues';
export const GET_ALL_ISSUES = pluginId + '_get_all_issues';
export const RECEIVED_SHOW_RHS_ACTION = pluginId + '_show_rhs';
export const UPDATE_RHS_STATE = pluginId + '_update_rhs_state';
export const SET_RHS_VISIBLE = pluginId + '_set_rhs_visible';
Expand Down
Loading
Loading