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

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
7 changes: 7 additions & 0 deletions server/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ type ExtendedIssue struct {
ForeignPosition int `json:"position"`
}

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

Choose a reason for hiding this comment

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

This isn't used anywhere anymore so we should remove this type definition

Suggested change
// 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
7 changes: 7 additions & 0 deletions server/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ type ListStore interface {

// GetList returns the list of IssueRef in listID for userID
GetList(userID, listID string) ([]*IssueRef, error)

// CountIssues returns all counter issues
CountIssues(userID string) (*CountIssue, error)
}

type listManager struct {
Expand Down Expand Up @@ -135,6 +138,10 @@ func (l *listManager) GetIssueList(userID, listID string) ([]*ExtendedIssue, err
return extendedIssues, nil
}

func (l *listManager) CountIssues(userID string) (countIssues *CountIssue, err error) {
return l.store.CountIssues(userID)
}

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
27 changes: 27 additions & 0 deletions server/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ type ListManager interface {
SendIssue(senderID, receiverID, message, description, postID string) (string, error)
// GetIssueList gets the todos on listID for userID
GetIssueList(userID, listID string) ([]*ExtendedIssue, error)
// CountIssues get counter all issues
raghavaggarwal2308 marked this conversation as resolved.
Show resolved Hide resolved
CountIssues(userID string) (*CountIssue, 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 @@ -116,6 +118,7 @@ func (p *Plugin) initializeAPI() {

p.router.HandleFunc("/add", p.checkAuth(p.handleAdd)).Methods(http.MethodPost)
p.router.HandleFunc("/list", p.checkAuth(p.handleList)).Methods(http.MethodGet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this endpoint anymore? Or are we still calling this in some cases?

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 need so i delete it but getIssueList is used by command

p.router.HandleFunc("/count", p.checkAuth(p.handleCount)).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 @@ -340,6 +343,30 @@ func (p *Plugin) handleList(w http.ResponseWriter, r *http.Request) {
}
}

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

countIssues, err := p.listManager.CountIssues(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)
return
}

countIssuesJSON, err := json.Marshal(countIssues)
if err != nil {
p.API.LogError("Unable marhsal count issue list to json err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable marhsal count issue list to json", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: to de-duplicate some string creation here. Same with the block above this one

Suggested change
p.API.LogError("Unable marhsal count issue list to json err=" + err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable marhsal count issue list to json", err)
msg := "Unable marshal count issue list to json"
p.API.LogError(msg, "err", err.Error())
p.handleErrorWithCode(w, http.StatusInternalServerError, msg, err)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that this is copied from the function above. I'm still thinking we can make this change 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.

I just modified all the error messages the same way

Copy link
Contributor

Choose a reason for hiding this comment

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

@mickmister What are your opinions on moving the log statement inside the "handleErrorWithCode" function instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

@raghavaggarwal2308 That sounds fine but we would want to change every call to handleErrorWithCode as well to avoid double logging. And that's definitely out of scope of this PR

return
}

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

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra newline at the end of this function and listStore.CountIssues. This is something we should have the linter configured to enforce, but I'm not sure if we have this particular convention enforced anywhere


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

Expand Down
22 changes: 22 additions & 0 deletions server/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,28 @@ func (l *listStore) getList(userID, listID string) ([]*IssueRef, []byte, error)
return list, originalJSONList, nil
}

func (l *listStore) CountIssues(userID string) (*CountIssue, error) {
myList, err := l.GetList(userID, MyListKey)
if err != nil {
return nil, err
}
inList, err := l.GetList(userID, InListKey)
if err != nil {
return nil, err
}
outList, err := l.GetList(userID, OutListKey)
if err != nil {
return nil, err
}

return &CountIssue{
In: len(inList),
My: len(myList),
Out: len(outList),
}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well just return the actual lists right? Then the frontend doesn't need to do any other requests


}

func (l *listStore) saveList(userID, listID string, list []*IssueRef, originalJSONList []byte) (bool, error) {
newJSONList, jsonErr := json.Marshal(list)
if jsonErr != nil {
Expand Down
1 change: 1 addition & 0 deletions webapp/src/action_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also be removed

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

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
21 changes: 21 additions & 0 deletions webapp/src/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
CLOSE_ADD_CARD,
SET_EDITING_TODO,
REMOVE_EDITING_TODO,
GET_COUNT_ISSUES,
} from './action_types';

import {getPluginServerRoute} from './selectors';
Expand Down Expand Up @@ -174,6 +175,26 @@ export const list = (reminder = false, listName = 'my') => async (dispatch, getS
return {data};
};

export const count = () => async (dispatch, getState) => {
mickmister marked this conversation as resolved.
Show resolved Hide resolved
let resp;
let data;
try {
resp = await fetch(getPluginServerRoute(getState()) + '/count', Client4.getOptions({
method: 'get',
}));
data = await resp.json();
raghavaggarwal2308 marked this conversation as resolved.
Show resolved Hide resolved
} catch (error) {
return {error};
}

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

return {data};
};

export const remove = (id) => async (dispatch, getState) => {
await fetch(getPluginServerRoute(getState()) + '/remove', Client4.getOptions({
method: 'post',
Expand Down
10 changes: 4 additions & 6 deletions webapp/src/components/sidebar_buttons/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,25 @@
import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';

import {list, updateRhsState, telemetry} from '../../actions';
import {count, updateRhsState, telemetry} from '../../actions';

import SidebarButtons from './sidebar_buttons.jsx';

function mapStateToProps(state) {
return {
issues: state['plugins-com.mattermost.plugin-todo'].issues,
inIssues: state['plugins-com.mattermost.plugin-todo'].inIssues,
outIssues: state['plugins-com.mattermost.plugin-todo'].outIssues,
countIssues: state['plugins-com.mattermost.plugin-todo'].countIssues,
showRHSPlugin: state['plugins-com.mattermost.plugin-todo'].rhsPluginAction,
};
}

function mapDispatchToProps(dispatch) {
return {
actions: bindActionCreators({
list,
count,
updateRhsState,
telemetry,
}, dispatch),
};
}

export default connect(mapStateToProps, mapDispatchToProps)(SidebarButtons);
export default connect(mapStateToProps, mapDispatchToProps)(SidebarButtons);
16 changes: 6 additions & 10 deletions webapp/src/components/sidebar_buttons/sidebar_buttons.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@ export default class SidebarButtons extends React.PureComponent {
theme: PropTypes.object.isRequired,
isTeamSidebar: PropTypes.bool,
showRHSPlugin: PropTypes.func.isRequired,
issues: PropTypes.arrayOf(PropTypes.object),
inIssues: PropTypes.arrayOf(PropTypes.object),
outIssues: PropTypes.arrayOf(PropTypes.object),
countIssues: PropTypes.object,
actions: PropTypes.shape({
list: PropTypes.func.isRequired,
count: PropTypes.func.isRequired,
mickmister marked this conversation as resolved.
Show resolved Hide resolved
updateRhsState: PropTypes.func.isRequired,
telemetry: PropTypes.func.isRequired,
}).isRequired,
Expand Down Expand Up @@ -49,9 +47,7 @@ export default class SidebarButtons extends React.PureComponent {
container = style.containerTeam;
}

const issues = this.props.issues || [];
const inIssues = this.props.inIssues || [];
const outIssues = this.props.outIssues || [];
const countIssues = this.props.countIssues;

return (
<div style={container}>
Expand All @@ -68,7 +64,7 @@ export default class SidebarButtons extends React.PureComponent {
}}
>
<i className='icon icon-check'/>
{' ' + issues.length}
{' ' + countIssues.my}
</a>
</OverlayTrigger>
<OverlayTrigger
Expand All @@ -84,7 +80,7 @@ export default class SidebarButtons extends React.PureComponent {
style={button}
>
<i className='icon icon-arrow-down'/>
{' ' + inIssues.length}
{' ' + countIssues.in}
</a>
</OverlayTrigger>
<OverlayTrigger
Expand All @@ -100,7 +96,7 @@ export default class SidebarButtons extends React.PureComponent {
style={button}
>
<i className='icon icon-arrow-up'/>
{' ' + outIssues.length}
{' ' + countIssues.out}
</a>
</OverlayTrigger>
</div>
Expand Down
12 changes: 7 additions & 5 deletions webapp/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Root from './components/root';
import AssigneeModal from './components/assignee_modal';
import SidebarRight from './components/sidebar_right';

import {openAddCard, list, setShowRHSAction, telemetry, updateConfig, setHideTeamSidebar} from './actions';
import {openAddCard, list, setShowRHSAction, telemetry, updateConfig, setHideTeamSidebar, count} from './actions';
import reducer from './reducer';
import PostTypeTodo from './components/post_type_todo';
import TeamSidebar from './components/team_sidebar';
Expand Down Expand Up @@ -66,11 +66,15 @@ export default class Plugin {
return frontendListName;
};

const refresh = ({data: {lists}}) => lists.forEach((listName) => store.dispatch(list(false, getFrontendListName(listName))));
const refresh = ({data: {lists}}) => {
mickmister marked this conversation as resolved.
Show resolved Hide resolved
lists.forEach((listName) => store.dispatch(list(false, getFrontendListName(listName))));
store.dispatch(count());
mickmister marked this conversation as resolved.
Show resolved Hide resolved
};
const refreshAll = () => {
store.dispatch(list(false));
store.dispatch(list(false, 'in'));
store.dispatch(list(false, 'out'));
store.dispatch(count());
mickmister marked this conversation as resolved.
Show resolved Hide resolved
};

const iconURL = getPluginServerRoute(store.getState()) + '/public/app-bar-icon.png';
Expand All @@ -83,9 +87,7 @@ export default class Plugin {
registry.registerWebSocketEventHandler(`custom_${pluginId}_refresh`, refresh);
registry.registerReconnectHandler(refreshAll);

store.dispatch(list(true));
store.dispatch(list(false, 'in'));
store.dispatch(list(false, 'out'));
store.dispatch(count());

// register websocket event to track config changes
const configUpdate = ({data}) => {
Expand Down
11 changes: 11 additions & 0 deletions webapp/src/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
GET_ISSUES,
SET_EDITING_TODO,
REMOVE_EDITING_TODO,
GET_COUNT_ISSUES,
GET_IN_ISSUES,
GET_OUT_ISSUES,
RECEIVED_SHOW_RHS_ACTION,
Expand Down Expand Up @@ -113,6 +114,15 @@ const outIssues = (state = [], action) => {
}
};

const countIssues = (state = {}, action) => {
switch (action.type) {
case GET_COUNT_ISSUES:
return action.data;
default:
return state;
}
};

function rhsPluginAction(state = null, action) {
switch (action.type) {
case RECEIVED_SHOW_RHS_ACTION:
Expand Down Expand Up @@ -159,6 +169,7 @@ export default combineReducers({
issues,
inIssues,
outIssues,
countIssues,
rhsState,
rhsPluginAction,
isRhsVisible,
Expand Down
1 change: 1 addition & 0 deletions webapp/src/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const getMessage = (state) => {
export const getIssues = (state) => getPluginState(state).issues;
export const getInIssues = (state) => getPluginState(state).inIssues;
export const getOutIssues = (state) => getPluginState(state).outIssues;
Copy link
Contributor

Choose a reason for hiding this comment

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

These selectors should probably be doing this now:

Suggested change
export const getIssues = (state) => getPluginState(state).issues;
export const getInIssues = (state) => getPluginState(state).inIssues;
export const getOutIssues = (state) => getPluginState(state).outIssues;
export const getIssues = (state) => getAllIssues(state).my;
export const getInIssues = (state) => getAllIssues(state).in;
export const getOutIssues = (state) => getAllIssues(state).out;

export const getCountIssues = (state) => getPluginState(state).countIssues;
export const getCurrentTeamRoute = (state) => {
const basePath = getSiteURL(state);
const teamName = state.entities.teams.teams[state.entities.teams.currentTeamId].name;
Expand Down