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

jufab · 2023-10-26T00:49:27Z

Summary

I made a counter resource and used it for display counter on the team sidebar.

Ticket Link

Fixes #202

mickmister

Thanks for this improvement @jufab 👍

I have a few comments and one request to minimize the fetching of lists from the frontend

webapp/src/actions.js

webapp/src/components/sidebar_buttons/sidebar_buttons.jsx

webapp/src/index.js

mickmister · 2023-10-31T04:28:25Z

server/plugin.go

+		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)


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)

I see now that this is copied from the function above. I'm still thinking we can make this change here

I just modified all the error messages the same way

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

@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

mickmister · 2023-10-31T04:31:15Z

server/plugin.go

+	}
+
+}


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

mickmister · 2023-10-31T04:31:39Z

server/store.go

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


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

server/plugin.go

webapp/src/actions.js

raghavaggarwal2308 · 2023-10-31T18:50:03Z

@jufab Just added a few minor comments, other than that LGTM. You still need to fix the comments provided by @mickmister

jufab · 2023-11-02T00:23:35Z

Sorry for that but I revised my MR proposal with this single request version for counters and display

mickmister

Great work on this @jufab. Thanks for addressing the changes 👍

I have a few more requests on this. There are some conventions that have changed on the frontend to cater towards the new data being transferred, though I think previous component props had a nice separation of concerns. Please let me know your thoughts @jufab

mickmister · 2023-11-07T06:22:16Z

server/issue.go

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


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"`

}

mickmister · 2023-11-07T06:27:24Z

server/plugin.go

@@ -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)


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

No need so i delete it but getIssueList is used by command

mickmister · 2023-11-07T06:28:33Z

server/plugin.go

+		msg := "Unable to get telemetry payload from JSON"
+		p.API.LogError(msg, "err", err.Error())
+		p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)


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

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

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

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

mickmister · 2023-11-07T06:29:44Z

webapp/src/action_types.js

@@ -15,6 +15,8 @@ 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';


This will also be removed

mickmister · 2023-11-07T06:29:53Z

webapp/src/actions.js

@@ -174,6 +175,25 @@ export const list = (reminder = false, listName = 'my') => async (dispatch, getS
    return {data};
 };

+export const fetchAllIssue = () => async (dispatch, getState) => {


Suggested change

export const fetchAllIssue = () => async (dispatch, getState) => {

export const fetchAllIssueLists = () => async (dispatch, getState) => {

mickmister · 2023-11-07T06:34:51Z

webapp/src/components/sidebar_buttons/index.js

-        issues: state['plugins-com.mattermost.plugin-todo'].issues,
-        inIssues: state['plugins-com.mattermost.plugin-todo'].inIssues,
-        outIssues: state['plugins-com.mattermost.plugin-todo'].outIssues,
+        allIssues: state['plugins-com.mattermost.plugin-todo'].allIssues,


I think it makes sense to keep the convention of feeding the component the issues the way we were doing before:

function mapStateToProps(state) { const allIssues = getAllIssues(state); // from selectors.js return { myIssues: allIssues.my, // note that I changed the prop to be `myIssues` inIssues: allIssues.in, outIssues: allIssues.out,

Though as mentioned elsewhere, we can use individual selectors for the in issues. The main reason I'm thinking this, is it simplifies what the component needs to worry about

mickmister · 2023-11-07T06:36:10Z

webapp/src/reducer.js

@@ -113,6 +114,15 @@ const outIssues = (state = [], action) => {
    }
 };

+const allIssues = (state = {}, action) => {


Let's handle the undefined state issue here instead of elsewhere

Suggested change

const allIssues = (state = {}, action) => {

const allIssuesInitialState = {

my: [],

in: [],

out: [],

};

const allIssues = (state = allIssuesInitialState, action) => {

mickmister · 2023-11-07T06:38:59Z

webapp/src/components/sidebar_right/index.js


 import SidebarRight from './sidebar_right.jsx';

 function mapStateToProps(state) {
    return {
-        todos: getIssues(state),
+        allIssues: state['plugins-com.mattermost.plugin-todo'].allIssues,


We should use the selectors whenever possible. Reading the previous code, I think we can continue using the getInIssues etc. selectors to make the consumer code less complicated. The selector would call the getAllIssues selector and return allIssues.in

I also like the convention of myTodos rather than just todos

mickmister · 2023-11-07T06:41:03Z

webapp/src/index.js

+        const refresh = () => {
+            store.dispatch(fetchAllIssue());


Can we have the websocket message that gets sent from the server contain the lists, rather than fetching them here?

It's after

registry.registerWebSocketEventHandler(`custom_${pluginId}_refresh`, refresh);

I'm not sure what this means. Are you saying it's not possible to deliver the data in the websocket message?

This line is the definition of the function that is then executed in the websocket.

mickmister · 2023-11-07T06:42:45Z

webapp/src/selectors.js

 export const getIssues = (state) => getPluginState(state).issues;
 export const getInIssues = (state) => getPluginState(state).inIssues;
 export const getOutIssues = (state) => getPluginState(state).outIssues;


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;

codecov-commenter · 2023-11-07T06:52:13Z

Codecov Report

Attention: 94 lines in your changes are missing coverage. Please review.

Comparison is base (d1f1d09) 6.42% compared to head (2d27842) 6.30%.

❗ Current head 2d27842 differs from pull request most recent head 99b959c. Consider uploading reports for the commit 99b959c to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #225      +/-   ##
=========================================
- Coverage    6.42%   6.30%   -0.13%     
=========================================
  Files          11      11              
  Lines        1712    1746      +34     
=========================================
  Hits          110     110              
- Misses       1594    1628      +34     
  Partials        8       8

Files	Coverage Δ
server/issue.go	`0.00% <ø> (ø)`
server/list.go	`0.00% <0.00%> (ø)`
server/plugin.go	`0.00% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jufab · 2023-11-07T23:59:49Z

I think I resolved all comment 😄

mickmister

Thanks @jufab! I have just a few more suggestions

mickmister · 2023-11-09T05:00:36Z

webapp/src/selectors.js

-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;


Can we rename this to getMyIssues?

Suggested change

export const getIssues = (state) => getAllIssues(state).my;

export const getMyIssues = (state) => getAllIssues(state).my;

mickmister · 2023-11-09T05:03:48Z

server/plugin.go

 	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) {


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

mickmister · 2023-11-09T05:04:31Z

server/plugin.go

@@ -329,7 +319,7 @@ func (p *Plugin) handleList(w http.ResponseWriter, r *http.Request) {
 		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))


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

mickmister · 2023-11-09T05:09:09Z

webapp/src/components/sidebar_right/sidebar_right.jsx

@@ -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 || [];


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

mickmister · 2023-11-09T05:11:20Z

webapp/src/components/sidebar_right/sidebar_right.jsx

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

    getInIssues() {


Suggested change

getInIssues() {

getInIssuesCount() {

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

mickmister · 2023-11-09T05:12:19Z

webapp/src/components/sidebar_right/sidebar_right.jsx

@@ -51,7 +51,9 @@ const InListName = 'in';

 export default class SidebarRight extends React.PureComponent {
    static propTypes = {
-        allIssues: PropTypes.arrayOf(PropTypes.object),
+        myIssues: PropTypes.array,


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)

mickmister · 2023-11-09T05:12:41Z

webapp/src/components/sidebar_buttons/sidebar_buttons.jsx

@@ -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,


Same here

Suggested change

myIssues: PropTypes.array,

myIssues: PropTypes.array.isRequired,

mickmister · 2023-11-09T05:13:01Z

webapp/src/components/sidebar_buttons/sidebar_buttons.jsx

@@ -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}


I think myIssues will always be defined right?

mickmister · 2023-11-09T23:19:48Z

@jufab Is this ready for another review? Just wondering

jufab · 2023-11-10T00:37:53Z

yes ready 😊

jufab · 2023-11-16T22:57:14Z

Do I have something else to do?

mickmister

LGTM, thanks @jufab!

mickmister · 2023-11-17T17:05:56Z

server/plugin.go

+		msg := "Unable to get telemetry payload from JSON"
+		p.API.LogError(msg, "err", err.Error())
+		p.handleErrorWithCode(w, http.StatusBadRequest, msg, err)


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

jufab added 2 commits October 26, 2023 02:26

feat: add count ressource for counter

bdb189c

chore: add count to update or add list

80d96b7

jufab requested a review from larkox as a code owner October 26, 2023 00:49

jufab changed the title ~~Issue 202~~ Fix(Issue#202): counts in one request Oct 26, 2023

larkox requested a review from mickmister October 26, 2023 13:29

mickmister requested review from raghavaggarwal2308 and Kshitij-Katiyar October 27, 2023 20:22

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 27, 2023

mickmister reviewed Oct 31, 2023

View reviewed changes

raghavaggarwal2308 approved these changes Oct 31, 2023

View reviewed changes

server/plugin.go Outdated Show resolved Hide resolved

webapp/src/actions.js Outdated Show resolved Hide resolved

jufab added 3 commits October 31, 2023 23:04

fix: review

49f7ffc

fix: count to fetchIssueCounts

e822f51

chore: review count in one request and lists

6624565

mickmister reviewed Nov 7, 2023

View reviewed changes

jufab added 2 commits November 8, 2023 00:34

fix: review

cd53a4b

fix: remove useless refreshAll

2d27842

mickmister self-requested a review November 9, 2023 04:58

mickmister reviewed Nov 9, 2023

View reviewed changes

fix: new review

99b959c

larkox requested a review from mickmister November 17, 2023 10:45

mickmister approved these changes Nov 17, 2023

View reviewed changes

mickmister requested a review from AayushChaudhary0001 November 17, 2023 17:08

mickmister removed the 2: Dev Review Requires review by a core committer label Nov 17, 2023

larkox approved these changes Nov 20, 2023

View reviewed changes

avas27JTG added this to the v1.0.0 milestone Dec 22, 2023

Merge branch 'master' into issue-202

046d508

		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)

		@@ -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)

	export const fetchAllIssue = () => async (dispatch, getState) => {
	export const fetchAllIssueLists = () => async (dispatch, getState) => {

-const allIssues = (state = {}, action) => {
+const allIssuesInitialState = {
+    my: [],
+    in: [],
+    out: [],
+};
+const allIssues = (state = allIssuesInitialState, action) => {

	export const getIssues = (state) => getAllIssues(state).my;
	export const getMyIssues = (state) => getAllIssues(state).my;

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

		getInIssues() {

	myIssues: PropTypes.array,
	myIssues: PropTypes.array.isRequired,

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

Are you sure you want to change the base?

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

Conversation

jufab commented Oct 26, 2023

Summary

Ticket Link

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Oct 31, 2023

jufab commented Nov 2, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 7, 2023 • edited Loading

Codecov Report

jufab commented Nov 7, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented Nov 9, 2023

jufab commented Nov 10, 2023

jufab commented Nov 16, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Nov 7, 2023 •

edited

Loading

mickmister Nov 9, 2023 •

edited

Loading

codecov-commenter commented Nov 7, 2023 •

edited

Loading