[MM-350] Fix issue in create issue modal and attach issue modal #761

ayusht2810 · 2024-04-04T12:27:57Z

Summary

Fixed issue of repository list not updating when there was incorrect ogranisation in the system console setting for Create GitHub Issue modal
Fixed issue of getting error of unexpected json when opening Attach to GitHub Issue modal for the above-mentioned case

Ticket Link

Fixes #557

Checklist

Completed dev testing
make test Ran test cases and ensured they are passing
make check-style Ran style check and ensured both webapp and server pass the checks

What to test?

Check the dropdown values for repository present in the Create GitHub Issue modal
Check the dropdown values for github issue present in the Attach to GitHub Issue modal

How to test?

Add GitHub Organisation in the system console settings
Check the dropdowns mentioned above
Update the GitHub Organisation again in the system console settings by adding some incorrect value
Check the dropdown values again

codecov-commenter · 2024-04-04T12:31:15Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 16.15%. Comparing base (da4c4df) to head (40626e3).
Report is 8 commits behind head on master.

❗ Current head 40626e3 differs from pull request most recent head 1b57a12

Please upload reports for the commit 1b57a12 to get more accurate results.

Files	Patch %	Lines
server/plugin/api.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
- Coverage   16.16%   16.15%   -0.01%     
==========================================
  Files          17       17              
  Lines        6021     6022       +1     
==========================================
  Hits          973      973              
- Misses       5003     5004       +1     
  Partials       45       45

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

hanzei · 2024-04-04T12:32:46Z

server/plugin/api.go

@@ -806,6 +806,7 @@ func (p *Plugin) searchIssues(c *UserContext, w http.ResponseWriter, r *http.Req
 	result, _, err := githubClient.Search.Issues(c.Ctx, query, &github.SearchOptions{})
 	if err != nil {
 		c.Log.WithError(err).With(logger.LogContext{"query": query}).Warnf("Failed to search for issues")
+		p.writeJSON(w, make([]*github.Issue, 0))


There are a lot of other endpoints that don't return valid JSON in the error case. Should we update the webapp to not expect JSON if the status code indicates an error?

@hanzei We were thinking of updating the plugin API response for each API where we are not returning anything in case we get any errors from the GitHub API.
For example: At this line, we can return an error something like p.writeAPIError(w, &APIErrorResponse{Message: "Failed to search for issues", StatusCode: http.StatusInternalServerError}) and handle the error similarly at the webapp side.
What are your thoughts on this?

If githubClient.Search.Issues finds nothing that matches the search, does it return an error?

If we put an invalid organisation or an organisation we don't have access to, we get an error, and it gets rendered as shown in the screenshot below.

But it doesn't return any errors when we search for an issue with a random input term (empty or an organisation we have access to).

We def want to avoid showing this Unexpected end of JSON input error in all cases

I see the value in consistently returning a JSON payload for calls the webapp makes. Should this be addressed holistically as part of this PR?

I suggest we update the signature of API methods like searchIssue from (c *UserContext, w http.ResponseWriter, r *http.Request) to (c *UserContext, r *http.Request) (any, int) and let a wrapper handle deal with writing the returned payload and status code. That way we ensure that all of these endpoint return valid JSON.

@hanzei Should we create a new issue to update the structure of the APIs and merge this PR to resolve the issue mentioned in the description?
Also, there were two ways we were thinking of fixing the response of the API:

Create a wrapper and update API methods from (c *UserContext, w http.ResponseWriter, r *http.Request) to (c *UserContext, r *http.Request) (any, int) as stated by you. We can also include a field of error with the data and the status code. In the wrapper, we can check for errors and data. If the error is returned, we can return p.writeAPIError(w, &APIErrorResponse{Message: "ErrorMessage", StatusCode: statusCode}) and handle similarly on webapp and if there is no error we can return data or an empty list if data is also empty.

In this way, we can directly update the API to simply return p.writeAPIError(w, &APIErrorResponse{Message: "ErrorMessage", StatusCode: statusCode}) in case of error. We can avoid making a wrapper in this case.

What are your thoughts on this? Please let me know if I missed something here.

Either one sounds good to me. Which one is less complicated in your opinion @ayusht2810? Have you tried implemented one of them to see if it fits well?

@mickmister We are thinking to proceed with the second method.
we can do this it in this PR or we can create a new PR and merge this so that the issue is fixed

@Kshitij-Katiyar Sure separate PR sounds good 👍

raghavaggarwal2308

LGTM, upon resolving @hanzei comment.

mickmister · 2024-04-22T14:59:51Z

webapp/src/actions/index.js

+            dispatch({
+                type: ActionTypes.RECEIVED_REPOSITORIES,
+                data: [],
+            });


What should the UI show/do in this case?

@mickmister Currently, we have the screenshot present here #761 (comment). I think instead of this, we should just show dropdown with no options present, which is currently done in this PR

Should we move forward with the approach mentioned in the #761 (comment) or should we just return an empty list from the backend to avoid the Unexpected end of JSON input error?

If there is an error from GitHub, we should show a useful error to the user. If we simply fetched no values with no error, then the backend should return an empty array and show no error

@mickmister I have created a new issue on Github to return proper json arrays from APIs. Maybe we can proceed further with this PR and handle the error case as part of the new issue. Please let me know your thoughts on this.

hanzei · 2024-04-30T09:43:00Z

server/plugin/api.go

@@ -806,6 +806,7 @@ func (p *Plugin) searchIssues(c *UserContext, w http.ResponseWriter, r *http.Req
 	result, _, err := githubClient.Search.Issues(c.Ctx, query, &github.SearchOptions{})
 	if err != nil {
 		c.Log.WithError(err).With(logger.LogContext{"query": query}).Warnf("Failed to search for issues")
+		p.writeJSON(w, make([]*github.Issue, 0))


I see the value in consistently returning a JSON payload for calls the webapp makes. Should this be addressed holistically as part of this PR?

I suggest we update the signature of API methods like searchIssue from (c *UserContext, w http.ResponseWriter, r *http.Request) to (c *UserContext, r *http.Request) (any, int) and let a wrapper handle deal with writing the returned payload and status code. That way we ensure that all of these endpoint return valid JSON.

…ub into MM-350

mickmister

LGTM 👍

There's a follow up ticket to address similar issues in other endpoints #794

AayushChaudhary0001

The above PR has been tested for the following scenarios:

Tested the dropdown values for repository present in the Create GitHub Issue modal
Tested the dropdown values for github issue present in the Attach to GitHub Issue modal

The PR was working fine for both the conditions, LGTM. Approved

mickmister · 2024-07-10T05:02:57Z

@wiggin77 This is ready for merge 👍

hanzei

I'm fine with moving the discussion from #761 (comment) to a separate endpoint

[MM-350] Fix issue in create issue modal and attach issue modal

40626e3

ayusht2810 requested review from hanzei and mickmister as code owners April 4, 2024 12:27

ayusht2810 requested a review from raghavaggarwal2308 April 4, 2024 12:28

ayusht2810 self-assigned this Apr 4, 2024

ayusht2810 added the 2: Dev Review Requires review by a core committer label Apr 4, 2024

hanzei requested changes Apr 4, 2024

View reviewed changes

raghavaggarwal2308 approved these changes Apr 9, 2024

View reviewed changes

mickmister reviewed Apr 22, 2024

View reviewed changes

hanzei requested changes Apr 30, 2024

View reviewed changes

raghavaggarwal2308 mentioned this pull request Jun 25, 2024

Return proper error reponses from APIs #794

Closed

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

1b57a12

…ub into MM-350

raghavaggarwal2308 requested a review from mickmister July 2, 2024 08:59

mickmister approved these changes Jul 2, 2024

View reviewed changes

raghavaggarwal2308 requested review from AayushChaudhary0001 and wiggin77 July 2, 2024 17:06

AayushChaudhary0001 approved these changes Jul 8, 2024

View reviewed changes

hanzei approved these changes Jul 10, 2024

View reviewed changes

wiggin77 approved these changes Jul 12, 2024

View reviewed changes

raghavaggarwal2308 merged commit a7e16de into master Jul 13, 2024
9 checks passed

raghavaggarwal2308 deleted the MM-350 branch July 13, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-350] Fix issue in create issue modal and attach issue modal #761

[MM-350] Fix issue in create issue modal and attach issue modal #761

ayusht2810 commented Apr 4, 2024

codecov-commenter commented Apr 4, 2024 •

edited

Loading

hanzei Apr 4, 2024

ayusht2810 Apr 5, 2024

hanzei Apr 5, 2024

ayusht2810 Apr 8, 2024

mickmister Apr 22, 2024

hanzei Apr 30, 2024

ayusht2810 May 7, 2024

mickmister May 13, 2024

Kshitij-Katiyar Jun 11, 2024

mickmister Jun 12, 2024

raghavaggarwal2308 left a comment

mickmister Apr 22, 2024

ayusht2810 Apr 24, 2024 •

edited

Loading

mickmister May 24, 2024

raghavaggarwal2308 Jun 25, 2024

hanzei Apr 30, 2024

mickmister left a comment

AayushChaudhary0001 left a comment

mickmister commented Jul 10, 2024

hanzei left a comment

[MM-350] Fix issue in create issue modal and attach issue modal #761

[MM-350] Fix issue in create issue modal and attach issue modal #761

Conversation

ayusht2810 commented Apr 4, 2024

Summary

Ticket Link

Checklist

What to test?

How to test?

codecov-commenter commented Apr 4, 2024 • edited Loading

Codecov Report

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

raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayusht2810 Apr 24, 2024 • 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

mickmister left a comment

Choose a reason for hiding this comment

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

mickmister commented Jul 10, 2024

hanzei left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 4, 2024 •

edited

Loading

ayusht2810 Apr 24, 2024 •

edited

Loading