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

Jump to bottom

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+160 −197
Open

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

Show file tree
Hide file tree
Changes from 2 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: 0 additions & 7 deletions server/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@ type ListsIssue struct {
Out []*ExtendedIssue `json:"out"`
}

// CountIssue for all counter issues
type CountIssue struct {
In int `json:"in"`
My int `json:"my"`
Out int `json:"out"`
}

func newIssue(message string, description, postID string) *Issue {
return &Issue{
ID: model.NewId(),
Expand Down
43 changes: 4 additions & 39 deletions server/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@
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)
Expand Down Expand Up @@ -208,7 +207,7 @@
if addRequest.SendTo == "" {
_, err = p.listManager.AddIssue(userID, addRequest.Message, addRequest.Description, addRequest.PostID)
if err != nil {
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 @@ -227,7 +226,7 @@
receiver, appErr := p.API.GetUserByUsername(addRequest.SendTo)
if appErr != nil {
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
}
Expand Down Expand Up @@ -290,27 +289,18 @@
}
}

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 {
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 {
Expand All @@ -329,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 @@ -338,31 +328,6 @@
}
}

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

_, err = w.Write(issuesJSON)
if err != nil {
p.API.LogError("Unable to write json response err=" + err.Error())
}
}

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

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

allListIssueJSON, err := json.Marshal(allListIssue)
if err != nil {
msg := "Unable marhsal all lists issues to json"
Expand Down Expand Up @@ -438,7 +403,7 @@
receiver, appErr := p.API.GetUserByUsername(changeRequest.SendTo)
if appErr != nil {
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
}
Expand Down
4 changes: 0 additions & 4 deletions webapp/src/action_types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ 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_COUNT_ISSUES = pluginId + '_get_count_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';
Expand Down
40 changes: 2 additions & 38 deletions webapp/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ import {
OPEN_TODO_TOAST,
CLOSE_TODO_TOAST,
RECEIVED_SHOW_RHS_ACTION,
GET_ISSUES,
GET_IN_ISSUES,
GET_OUT_ISSUES,
UPDATE_RHS_STATE,
SET_RHS_VISIBLE,
SET_HIDE_TEAM_SIDEBAR_BUTTONS,
Expand Down Expand Up @@ -142,43 +139,10 @@ export const changeAssignee = (id, assignee) => async (dispatch, getState) => {
}));
};

export const list = (reminder = false, listName = 'my') => async (dispatch, getState) => {
let resp;
export const fetchAllIssueLists = (reminder = false) => async (dispatch, getState) => {
let data;
try {
resp = await fetch(getPluginServerRoute(getState()) + '/list?reminder=' + reminder + '&list=' + listName, Client4.getOptions({
method: 'get',
}));
data = await resp.json();
} catch (error) {
return {error};
}

let actionType = GET_ISSUES;
switch (listName) {
case 'my':
actionType = GET_ISSUES;
break;
case 'in':
actionType = GET_IN_ISSUES;
break;
case 'out':
actionType = GET_OUT_ISSUES;
break;
}

dispatch({
type: actionType,
data,
});

return {data};
};

export const fetchAllIssue = () => async (dispatch, getState) => {
let data;
try {
const resp = await fetch(getPluginServerRoute(getState()) + '/lists', Client4.getOptions({
const resp = await fetch(getPluginServerRoute(getState()) + '/lists?reminder=' + reminder, Client4.getOptions({
method: 'get',
}));
data = await resp.json();
Expand Down
4 changes: 2 additions & 2 deletions webapp/src/components/post_type_todo/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';

import {remove, complete, accept, telemetry} from '../../actions';
import {getSiteURL} from '../../selectors';
import {getInIssues, getSiteURL} from '../../selectors';

import PostTypeTodo from './post_type_todo';

function mapStateToProps(state, ownProps) {
return {
...ownProps,
siteURL: getSiteURL(state),
pendingAnswer: state['plugins-com.mattermost.plugin-todo'].inIssues.some((issue) => issue.id === ownProps.post.props.issueId),
pendingAnswer: getInIssues(state).some((issue) => issue.id === ownProps.post.props.issueId),
};
}

Expand Down
10 changes: 7 additions & 3 deletions webapp/src/components/sidebar_buttons/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,25 @@
import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';

import {fetchAllIssue, updateRhsState, telemetry} from '../../actions';
import {fetchAllIssueLists, updateRhsState, telemetry} from '../../actions';

import {getIssues, getInIssues, getOutIssues} from '../../selectors';

import SidebarButtons from './sidebar_buttons.jsx';

function mapStateToProps(state) {
return {
allIssues: state['plugins-com.mattermost.plugin-todo'].allIssues,
myIssues: getIssues(state),
inIssues: getInIssues(state),
outIssues: getOutIssues(state),
showRHSPlugin: state['plugins-com.mattermost.plugin-todo'].rhsPluginAction,
};
}

function mapDispatchToProps(dispatch) {
return {
actions: bindActionCreators({
fetchAllIssue,
fetchAllIssueLists,
updateRhsState,
telemetry,
}, dispatch),
Expand Down
14 changes: 9 additions & 5 deletions webapp/src/components/sidebar_buttons/sidebar_buttons.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ export default class SidebarButtons extends React.PureComponent {
theme: PropTypes.object.isRequired,
isTeamSidebar: PropTypes.bool,
showRHSPlugin: PropTypes.func.isRequired,
allIssues: PropTypes.object,
myIssues: PropTypes.array,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Suggested change
myIssues: PropTypes.array,
myIssues: PropTypes.array.isRequired,

inIssues: PropTypes.array,
outIssues: PropTypes.array,
actions: PropTypes.shape({
updateRhsState: PropTypes.func.isRequired,
telemetry: PropTypes.func.isRequired,
Expand Down Expand Up @@ -46,7 +48,9 @@ export default class SidebarButtons extends React.PureComponent {
container = style.containerTeam;
}

const allIssues = this.props.allIssues;
const myIssues = this.props.myIssues;
const inIssues = this.props.inIssues;
const outIssues = this.props.outIssues;

return (
<div style={container}>
Expand All @@ -63,7 +67,7 @@ export default class SidebarButtons extends React.PureComponent {
}}
>
<i className='icon icon-check'/>
{' ' + allIssues && allIssues.my ? allIssues.my.length : 0}
{' ' + myIssues ? myIssues.length : 0}
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 myIssues will always be defined right?

</a>
</OverlayTrigger>
<OverlayTrigger
Expand All @@ -79,7 +83,7 @@ export default class SidebarButtons extends React.PureComponent {
style={button}
>
<i className='icon icon-arrow-down'/>
{' ' + allIssues && allIssues.in ? allIssues.in.length : 0}
{' ' + inIssues ? inIssues.length : 0}
</a>
</OverlayTrigger>
<OverlayTrigger
Expand All @@ -95,7 +99,7 @@ export default class SidebarButtons extends React.PureComponent {
style={button}
>
<i className='icon icon-arrow-up'/>
{' ' + allIssues && allIssues.out ? allIssues.out.length : 0}
{' ' + outIssues ? outIssues.length : 0}
</a>
</OverlayTrigger>
</div>
Expand Down
10 changes: 6 additions & 4 deletions webapp/src/components/sidebar_right/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';

import {getSiteURL, getTodoToast} from '../../selectors';
import {remove, fetchAllIssue, openAssigneeModal, openAddCard, closeAddCard, complete, bump, accept, telemetry, setRhsVisible} from '../../actions';
import {getSiteURL, getTodoToast, getIssues, getInIssues, getOutIssues} from '../../selectors';
import {remove, fetchAllIssueLists, openAssigneeModal, openAddCard, closeAddCard, complete, bump, accept, telemetry, setRhsVisible} from '../../actions';

import SidebarRight from './sidebar_right.jsx';

function mapStateToProps(state) {
return {
allIssues: state['plugins-com.mattermost.plugin-todo'].allIssues,
myIssues: getIssues(state),
inIssues: getInIssues(state),
outIssues: getOutIssues(state),
todoToast: getTodoToast(state),
siteURL: getSiteURL(state),
rhsState: state['plugins-com.mattermost.plugin-todo'].rhsState,
Expand All @@ -25,7 +27,7 @@ function mapDispatchToProps(dispatch) {
complete,
accept,
bump,
fetchAllIssue,
fetchAllIssueLists,
openAddCard,
closeAddCard,
openAssigneeModal,
Expand Down
20 changes: 11 additions & 9 deletions webapp/src/components/sidebar_right/sidebar_right.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ const InListName = 'in';

export default class SidebarRight extends React.PureComponent {
static propTypes = {
allIssues: PropTypes.arrayOf(PropTypes.object),
myIssues: PropTypes.array,
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
myIssues: PropTypes.array,
myIssues: PropTypes.array.isRequired,

Same for the two lines below this one. (GitHub isn't letting me comment on multiple lines for some reason)

image

inIssues: PropTypes.array,
outIssues: PropTypes.array,
todoToast: PropTypes.object,
theme: PropTypes.object.isRequired,
siteURL: PropTypes.string.isRequired,
Expand All @@ -61,7 +63,7 @@ export default class SidebarRight extends React.PureComponent {
complete: PropTypes.func.isRequired,
accept: PropTypes.func.isRequired,
bump: PropTypes.func.isRequired,
fetchAllIssue: PropTypes.func.isRequired,
fetchAllIssueLists: PropTypes.func.isRequired,
openAddCard: PropTypes.func.isRequired,
closeAddCard: PropTypes.func.isRequired,
openAssigneeModal: PropTypes.func.isRequired,
Expand Down Expand Up @@ -99,7 +101,7 @@ export default class SidebarRight extends React.PureComponent {

componentDidMount() {
document.addEventListener('keydown', this.handleKeypress);
this.props.actions.fetchAllIssue();
this.props.actions.fetchAllIssueLists();
this.props.actions.setVisible(true);
}

Expand All @@ -122,15 +124,15 @@ export default class SidebarRight extends React.PureComponent {
}

getInIssues() {
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
getInIssues() {
getInIssuesCount() {

It looks like these 3 functions aren't actually called though. I think we can just remove them

return this.props.allIssues.in.length;
return this.props.inIssues.length;
}

getOutIssues() {
return this.props.allIssues.out.length;
return this.props.outIssues.length;
}

getMyIssues() {
return this.props.allIssues.my.length;
return this.props.myIssues.length;
}

addTodoItem() {
Expand All @@ -150,12 +152,12 @@ export default class SidebarRight extends React.PureComponent {

switch (this.state.list) {
case MyListName:
todos = this.props.allIssues.my || [];
todos = this.props.myIssues || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

These empty arrays are going to cause re-renders. But this.props.myIssues should always be defined anyway right? So these actually don't have an effect. Either way I think that this component shouldn't be responsible for this check

addButton = 'Add Todo';
inboxList = this.props.allIssues.in || [];
inboxList = this.props.inIssues || [];
break;
case OutListName:
todos = this.props.allIssues.out || [];
todos = this.props.outIssues || [];
listHeading = 'Sent Todos';
addButton = 'Request a Todo from someone';
break;
Expand Down
Loading
Loading