MM-50981 Remove usage of model.AppError #921

srkgupta · 2023-03-07T15:07:12Z

Summary

Remove usage of model.AppError in JIRA plugin

Ticket Link

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

codecov-commenter · 2023-03-09T05:53:43Z

Codecov Report

Patch coverage: 21.05% and project coverage change: -0.13 ⚠️

Comparison is base (d6ba56f) 31.01% compared to head (aca3b96) 30.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
- Coverage   31.01%   30.89%   -0.13%     
==========================================
  Files          50       50              
  Lines        7389     7332      -57     
==========================================
- Hits         2292     2265      -27     
+ Misses       4911     4886      -25     
+ Partials      186      181       -5

Impacted Files	Coverage Δ
server/atlassian_connect.go	`0.00% <0.00%> (ø)`
server/client.go	`8.04% <0.00%> (ø)`
server/telemetry.go	`0.00% <0.00%> (ø)`
server/user_cloud.go	`0.00% <0.00%> (ø)`
server/user_server.go	`7.54% <0.00%> (ø)`
server/utils.go	`17.69% <0.00%> (ø)`
server/utils/kvstore/ots.go	`0.00% <0.00%> (ø)`
server/utils/kvstore/plugin_store.go	`0.00% <0.00%> (ø)`
server/webhook_parser.go	`85.05% <0.00%> (ø)`
server/plugin.go	`11.65% <10.00%> (+0.12%)`	⬆️
... and 10 more

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

Great changes and cleanup of the deprecated built-in go APIs too!

hanzei · 2023-03-23T10:03:20Z

@srkgupta Heads up that there is a merge conflict.

hanzei · 2023-03-23T10:09:26Z

server/issue.go

 	}

 	if post != nil && len(post.FileIds) > 0 {
 		go func() {
 			conf := instance.Common().getConfig()
 			for _, fileID := range post.FileIds {
-				mattermostName, _, _, e := client.AddAttachment(p.API, created.ID, fileID, conf.maxAttachmentSize)
+				mattermostName, _, _, e := client.AddAttachment(*p.client, created.ID, fileID, conf.maxAttachmentSize)


While we are here: Can we rename the error to the standard err?

hanzei

Thanks for taking over this huge effort 👍

plugin_store.go was left untouched. Why is that the case? Can the code in there also get migrated?
Let's double-check that the code migration around the KV store is correct. There are some edge cases we need to be aware of. More details in the comments below ⬇️

server/issue.go

server/command.go

hanzei · 2023-03-23T11:01:35Z

server/kv.go

-	}
-
-	err := json.Unmarshal(data, v)
+	err := store.plugin.client.KV.Get(key, &v)


Previously, v was passed to json.Unmarshal meaning it had to be an pointer. Why does a pointer to v get passed now?

hanzei · 2023-03-23T11:13:26Z

server/kv.go

-	if appErr != nil {
-		return "", errors.WithMessage(appErr, "failed to load one-time secret "+key)
+	var secret string
+	err := store.get(hashkey(prefixOneTimeSecret, key), secret)


That looks suspicious. The stored data is not in JSON format, but store.get will try to unmarshal it. Are you sure this line works?

Nice catch. Yes, this looks like the code was accidentally over-written. It should read:

err := store.plugin.client.KV.Get(hashkey(prefixOneTimeSecret, key), secret)

I will push a commit to fix this.

hanzei · 2023-03-23T11:16:10Z

server/kv.go

+		return nil, nil
+	}


Previously, the code returned an error in this case. Why was it removed?

server/plugin.go

go.mod

hanzei · 2023-03-23T11:26:17Z

server/plugin_test.go

 			Configuration:      validConfiguration,
-			CreatePostError:    model.NewAppError("foo", "bar", nil, "", http.StatusInternalServerError),
+			CreatePostError:    model.NewAppError("foo", "bar", nil, "", http.StatusBadRequest),
 			Request:            httptest.NewRequest("POST", "/webhook?team=theteam&channel=thechannel&secret=thesecret", validRequestBody()),
-			ExpectedStatusCode: http.StatusInternalServerError,
+			ExpectedStatusCode: http.StatusBadRequest,


Why was there a behaviour change?

Earlier, we were returning the status Code from the appError. However, since the error no longer has access to the returned Status Code, I had hardcoded it as http.StatusBadRequest which had caused this change. I will revert and hardcode the return status as http.StatusInternalServerError instead.

server/subscribe.go

srkgupta · 2023-03-23T11:56:58Z

Thanks for the detailed review @hanzei. Will be making the suggested changes shortly and request your re-review once its committed.

mickmister

No issues found other than Ben's comments above 👍

srkgupta · 2023-03-27T09:19:14Z

Hi @hanzei

I have made all the changes based on your feedback. Requesting your re-review on this.

Thanks

hanzei

Two nits, but the rest LGTM 👍

server/kv.go

hanzei

🚀

hanzei · 2023-04-03T05:19:43Z

@srkgupta Heads up that there are merge conflicts to resolve

srkgupta · 2023-04-03T08:30:41Z

@DHaussermann this is ready to be QA'd, as it also has the code for mux.Router PR which was recently merged with master.

DHaussermann

Smoke tested Jira

For the most part I saw no issue.

One problem I see on this branch is that authenticating to Jira server seems to no longer work. (no issues with Cloud authentication)
Steps:

Install a Jira server instance (mine is 9.5.0)
Run the /jira connect command
In the new tab click allow
On the oAuth complete page I get a 405 response

-The ngrok tunnel to Jira shows nothing after the 302 (the other 2 calls in the screenshot happened later and are unrelated)

@srkgupta unfortunately, because of a licensing issue on Jira server I was still working to resolve Friday, I did not confirm this was working on #923 and deffered testing until now so, I am unsure of which PR introduces the behavior.

srkgupta · 2023-04-04T12:52:07Z

Thanks @DHaussermann. I have fixed the issue now. This was again one of those cases, where the HTTP method wasn't clearly defined previously in the OAuth1 Complete endpoint. Please use the latest code and try again.

DHaussermann

Tested and passes
I have spent some time smoke testing and see no more regressions
Tested against both Cloud and Serever

Setup
Authentication flow (browser and desktop)
Issue creation and attachment
Subscription and delivery
Multiple instances connected
Tested various commands for assign, view and transition etc..

LGTM!

Thanks @srkgupta!

srkgupta added 3 commits March 6, 2023 20:03

Remove usage of model.AppError

90c9198

Fixed build errors

a6e112f

Fixed unit tests

a3f323b

srkgupta added the Work In Progress Not yet ready for review label Mar 7, 2023

srkgupta requested a review from mickmister as a code owner March 7, 2023 15:07

Replaced ioutil depreciations in go1.18

af91d9c

Fixed go.sum

ea32bdd

srkgupta added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Work In Progress Not yet ready for review labels Mar 9, 2023

srkgupta requested a review from hanzei March 9, 2023 06:19

Updated Webhook test with mocked post response

5ec7e7e

srkgupta requested a review from m1lt0n March 15, 2023 08:35

m1lt0n approved these changes Mar 15, 2023

View reviewed changes

Replaced appError in ExecuteCommand

725ac02

hanzei reviewed Mar 23, 2023

View reviewed changes

hanzei requested changes Mar 23, 2023

View reviewed changes

srkgupta added 3 commits March 24, 2023 10:53

Fixed few review comments

b6e7839

Merge branch 'master' into MM-50981_remove_model.AppError

6744e83

Fixed merge conflicts & lint error

6256797

mickmister approved these changes Mar 24, 2023

View reviewed changes

srkgupta added 3 commits March 27, 2023 11:53

Migrated plugin_store and implemented additional review comments

b64a47c

Fixed nil pointer dereference while activating the plugin

f172981

Fixed error while connecting to Jira Cloud

2db5ddd

srkgupta requested a review from hanzei March 27, 2023 09:18

hanzei approved these changes Mar 30, 2023

View reviewed changes

server/kv.go Outdated Show resolved Hide resolved

server/kv.go Outdated Show resolved Hide resolved

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

reverted to use existing JSON unmarshal logic

ebd44c1

hanzei approved these changes Mar 30, 2023

View reviewed changes

hanzei requested a review from DHaussermann March 30, 2023 11:42

hanzei added the Awaiting Submitter Action Blocked on the author label Apr 3, 2023

srkgupta added 2 commits April 3, 2023 13:45

Resolved merge conflicts with latest master

385d172

Replaced usage of p.API in new code in master

7114beb

srkgupta removed the Awaiting Submitter Action Blocked on the author label Apr 3, 2023

hanzei added this to the v3.3.0 milestone Apr 3, 2023

DHaussermann reviewed Apr 3, 2023

View reviewed changes

srkgupta added 2 commits April 4, 2023 18:10

Corrected HTTP method in OAuth1 complete endpoint

fd67a87

Resolved conflicts with latest master

aca3b96

srkgupta requested a review from DHaussermann April 4, 2023 12:52

DHaussermann approved these changes Apr 5, 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 Apr 5, 2023

hanzei merged commit fcc1205 into master Apr 5, 2023

hanzei deleted the MM-50981_remove_model.AppError branch April 5, 2023 19:57

mickmister mentioned this pull request May 4, 2023

[GH-558] Fix issue: /jira assign should accept @username #938

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-50981 Remove usage of model.AppError #921

MM-50981 Remove usage of model.AppError #921

srkgupta commented Mar 7, 2023

codecov-commenter commented Mar 9, 2023 •

edited

Loading

m1lt0n left a comment

hanzei commented Mar 23, 2023

hanzei Mar 23, 2023

hanzei left a comment

hanzei Mar 23, 2023

hanzei Mar 23, 2023

srkgupta Mar 24, 2023

hanzei Mar 23, 2023

hanzei Mar 23, 2023

srkgupta Mar 24, 2023

srkgupta commented Mar 23, 2023

mickmister left a comment

srkgupta commented Mar 27, 2023 •

edited

Loading

hanzei left a comment

hanzei left a comment

hanzei commented Apr 3, 2023

srkgupta commented Apr 3, 2023

DHaussermann left a comment •

edited

Loading

srkgupta commented Apr 4, 2023

DHaussermann left a comment

MM-50981 Remove usage of model.AppError #921

MM-50981 Remove usage of model.AppError #921

Conversation

srkgupta commented Mar 7, 2023

Summary

Ticket Link

codecov-commenter commented Mar 9, 2023 • edited Loading

Codecov Report

m1lt0n left a comment

Choose a reason for hiding this comment

hanzei commented Mar 23, 2023

hanzei Mar 23, 2023

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

hanzei Mar 23, 2023

Choose a reason for hiding this comment

srkgupta Mar 24, 2023

Choose a reason for hiding this comment

hanzei Mar 23, 2023

Choose a reason for hiding this comment

hanzei Mar 23, 2023

Choose a reason for hiding this comment

srkgupta Mar 24, 2023

Choose a reason for hiding this comment

srkgupta commented Mar 23, 2023

mickmister left a comment

Choose a reason for hiding this comment

srkgupta commented Mar 27, 2023 • edited Loading

hanzei left a comment

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

hanzei commented Apr 3, 2023

srkgupta commented Apr 3, 2023

DHaussermann left a comment • edited Loading

Choose a reason for hiding this comment

srkgupta commented Apr 4, 2023

DHaussermann left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 9, 2023 •

edited

Loading

srkgupta commented Mar 27, 2023 •

edited

Loading

DHaussermann left a comment •

edited

Loading