MM-51188 Use mux.Router #923

srkgupta · 2023-03-17T12:55:12Z

Summary

Migrate JIRA codebase to use mux.Router

Ticket Link

https://mattermost.atlassian.net/browse/MM-51188

codecov-commenter · 2023-03-17T12:59:39Z

Codecov Report

Patch coverage: 76.98% and project coverage change: +1.05 🎉

Comparison is base (a7f0e41) 29.96% compared to head (09fdf38) 31.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #923      +/-   ##
==========================================
+ Coverage   29.96%   31.01%   +1.05%     
==========================================
  Files          49       50       +1     
  Lines        7520     7383     -137     
==========================================
+ Hits         2253     2290      +37     
+ Misses       5078     4907     -171     
+ Partials      189      186       -3

Impacted Files	Coverage Δ
server/atlassian_connect.go	`0.00% <ø> (ø)`
server/autocomplete_search.go	`0.00% <ø> (ø)`
server/command.go	`16.06% <0.00%> (ø)`
server/info.go	`62.50% <ø> (+8.92%)`	⬆️
server/instances.go	`32.90% <ø> (+2.72%)`	⬆️
server/issue.go	`30.37% <ø> (+0.43%)`	⬆️
server/plugin.go	`10.99% <0.00%> (+1.51%)`	⬆️
server/setup_flow.go	`0.00% <0.00%> (ø)`
server/user.go	`27.12% <ø> (+4.33%)`	⬆️
server/user_cloud.go	`0.00% <ø> (ø)`
... and 6 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

m1lt0n

LGTM! One minor/non-blocking comment: I was torn between the prefixed router or the full route names as approaches, but not a both have their pros and cons, so 0/5 on it.

hanzei

Neat 🚀

There are some method check accross the repo like in

mattermost-plugin-jira/server/issue.go

Lines 172 to 175 in 2f8fa78

    
           if r.Method != http.MethodPost { 
        
           	return respondErr(w, http.StatusMethodNotAllowed, 
        
           		errors.New("method "+r.Method+" is not allowed, must be POST")) 
        
           }

. Can these get removed?

hanzei · 2023-03-23T12:05:46Z

server/http.go

+			return
+		}
+
+		status, err := fn(w, r, callbackInstanceID)


Nit pick: Can handleResponse be used here with an adapter?

@hanzei, can you please elaborate?

By replacing this line and the following with

Suggested change

status, err := fn(w, r, callbackInstanceID)

p.handleResponse(func(w http.ResponseWriter, r *http.Request) (int, error) {

return fn(w, r, callbackInstanceID)

})

we estimate the duplicate code that takes care of the status code..

Got it. Makes sense 👍 Updated the code now.

@hanzei the suggested logic would not work, as the underlying functions being called were not getting the expected callbackInstanceID. Hence instead of this, I have now added a common function to parse the status and the error and log them from a single function instead of writing duplicate code. Hope this is fine.

b6ff1d8

mickmister

Great improvement here 👍

Just a few suggestions

server/command.go

server/http.go

server/webhook.go

srkgupta · 2023-03-28T06:48:30Z

@hanzei @mickmister, I have implemented the review comments and some additional findings like using checkAuth as part of the router handler. Re-requesting your reviews.

DHaussermann · 2023-03-29T21:24:08Z

@srkgupta I may be mistaken but I believe something is broken here. When I try to smoke test by installing a cloud instance, the .json which provides the installation details seem to no longer be reachable and returns a 404. This causes the install from the Jira side to fail.

Please let me know if you can reproduce this or perhaps there is some unrelated cause I have overlooked.

Steps:

Install the latest production release and enable the plugin
Run /jira instance install cloud https://mmtest.atlassian.net
In the ephemeral post note that a link in generated to https://dkh-local.ngrok.io/plugins/jira/instance/aHR0cHM6Ly9tbXRlc3QuYXRsYXNzaWFuLm5ldA==/ac/atlassian-connect.json Your MM URL will differ but clicking the link open a .json object in a new tab with installation details
Upgrade to MM-51188_mux-routerbranch
Run connect command again (optional as the lik address has not changed)
Observed the same URL now goes to a 404 when clicked

srkgupta · 2023-03-30T08:16:26Z

Thanks @DHaussermann for catching this issue. I have fixed the issue and pushed a new commit. It should resolve the issue. Can you please specifically check the entire OAuth1 & the Connection flows. The HTTP methods for some of these API endpoints were not clearly defined previously. Hence any HTTP method like GET, POST request would work. However with the new mux.Router approach, since its specifically defined, just want to be sure that these are correctly working now.

hanzei

Two non-blockin comments: #923 (comment)

server/http.go

hanzei · 2023-03-30T09:22:49Z

Great work @srkgupta 👍

DHaussermann · 2023-03-30T16:29:56Z

@srkgupta I have a strange issue where I don't see any change in behavior. When I upload the build and try again I still see the 404 when I click the link to the .json.

I have made sure I have your commits.

Any thoughts on if maybe something is causing a deployment issue? Can you confirm you see this working on the latest?

srkgupta · 2023-03-31T09:55:02Z

Hi @DHaussermann

I looked into this in detail and found that the URLs for certain paths started with /instance/{instance_id} and hence had to be handled in a slightly different way. I have fixed this issue and have tested a basic Jira Cloud setup flow and it is working fine now. Please recheck now and let me know if you see any other issues.

DHaussermann

Tested and passed

Issue discussed above has been resolved
Did a smoke test covering happy path
Install works
Create and attach funtionallity works
Subscriptions creation and delivery works

LGTM!

Thanks @srkgupta 🎉 If we can get this merged, I can then pull this up into #921 and do a bit more testing on all changes.

srkgupta added 2 commits March 17, 2023 18:04

Use mux.Router

76cb9a2

Fixed golint

2f8fa78

srkgupta requested a review from mickmister as a code owner March 17, 2023 12:55

srkgupta changed the title ~~Mm 51188 mux router~~ MM-51188 Use mux.Router Mar 17, 2023

srkgupta requested review from hanzei and m1lt0n March 17, 2023 12:56

srkgupta added the 2: Dev Review Requires review by a core committer label Mar 17, 2023

m1lt0n approved these changes Mar 20, 2023

View reviewed changes

hanzei added the 3: QA Review Requires review by a QA tester label Mar 20, 2023

hanzei reviewed Mar 23, 2023

View reviewed changes

mickmister reviewed Mar 23, 2023

View reviewed changes

Implemented review comments

49e9f35

srkgupta requested review from mickmister and hanzei March 28, 2023 06:47

mickmister approved these changes Mar 28, 2023

View reviewed changes

Corrected HTTP methods for some endpoints

10fa514

hanzei approved these changes Mar 30, 2023

View reviewed changes

server/http.go Outdated Show resolved Hide resolved

hanzei added this to the v3.3.0 milestone Mar 30, 2023

hanzei removed the 2: Dev Review Requires review by a core committer label Mar 30, 2023

hanzei requested a review from DHaussermann March 30, 2023 09:23

srkgupta added 2 commits March 30, 2023 15:27

nit improvements

64338c8

Handled duplicate code while logging response

b6ff1d8

Fixed instance router paths

09fdf38

DHaussermann approved these changes Mar 31, 2023

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Mar 31, 2023

srkgupta merged commit a8167cc into master Apr 3, 2023

srkgupta deleted the MM-51188_mux-router branch April 3, 2023 05:01

srkgupta mentioned this pull request Apr 3, 2023

MM-50981 Remove usage of model.AppError #921

Merged

mickmister mentioned this pull request Sep 14, 2023

Release 4.0.0 #966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-51188 Use mux.Router #923

MM-51188 Use mux.Router #923

srkgupta commented Mar 17, 2023

codecov-commenter commented Mar 17, 2023 •

edited

Loading

m1lt0n left a comment

hanzei left a comment

hanzei Mar 23, 2023

srkgupta Mar 28, 2023

hanzei Mar 30, 2023

srkgupta Mar 30, 2023

srkgupta Mar 30, 2023

mickmister left a comment

srkgupta commented Mar 28, 2023

DHaussermann commented Mar 29, 2023

srkgupta commented Mar 30, 2023

hanzei left a comment

hanzei commented Mar 30, 2023

DHaussermann commented Mar 30, 2023

srkgupta commented Mar 31, 2023

DHaussermann left a comment

	if r.Method != http.MethodPost {
	return respondErr(w, http.StatusMethodNotAllowed,
	errors.New("method "+r.Method+" is not allowed, must be POST"))
	}

-		status, err := fn(w, r, callbackInstanceID)
+	p.handleResponse(func(w http.ResponseWriter, r *http.Request) (int, error) {
+		return fn(w, r, callbackInstanceID)
+	})

MM-51188 Use mux.Router #923

MM-51188 Use mux.Router #923

Conversation

srkgupta commented Mar 17, 2023

Summary

Ticket Link

codecov-commenter commented Mar 17, 2023 • edited Loading

Codecov Report

m1lt0n left a comment

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

hanzei Mar 23, 2023

Choose a reason for hiding this comment

srkgupta Mar 28, 2023

Choose a reason for hiding this comment

hanzei Mar 30, 2023

Choose a reason for hiding this comment

srkgupta Mar 30, 2023

Choose a reason for hiding this comment

srkgupta Mar 30, 2023

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

srkgupta commented Mar 28, 2023

DHaussermann commented Mar 29, 2023

srkgupta commented Mar 30, 2023

hanzei left a comment

Choose a reason for hiding this comment

hanzei commented Mar 30, 2023

DHaussermann commented Mar 30, 2023

srkgupta commented Mar 31, 2023

DHaussermann left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2023 •

edited

Loading