[GH-54] sync with latest mattermost-plugin-starter-template #68

jfrerich · 2020-07-27T21:02:02Z

Summary

Syncs with latest mattermost-plugin-starter-template

Ticket Link

codecov-commenter · 2020-07-27T21:10:23Z

Codecov Report

Merging #68 into master will increase coverage by 0.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   29.48%   30.08%   +0.59%     
==========================================
  Files           8        9       +1     
  Lines         234      236       +2     
==========================================
+ Hits           69       71       +2     
  Misses        153      153              
  Partials       12       12

Impacted Files	Coverage Δ
server/service/delete_subscription.go	`35.29% <ø> (ø)`
server/service/get_subscription.go	`75.00% <ø> (ø)`
server/service/save_subscription.go	`31.57% <ø> (ø)`
server/util/util.go	`40.67% <ø> (ø)`
server/manifest.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eed9fc7...93d55b1. Read the comment docs.

golangci-lint fixes make check-style fixes broken test fixes

jfrerich · 2020-07-28T16:50:35Z

I quite like having colorized terminal output and although I am pulling this OUT of the Makefile, what do you guys think about adding this color highlighting to our Makefile :)

https://github.com/mattermost/mattermost-plugin-confluence/pull/68/files#diff-b67911656ef5d18c4ae36cb6741b7965L27-L34

mickmister · 2020-07-29T18:34:10Z

@jfrerich Do you mind going in detail with your process how you performed this sync?

jfrerich · 2020-07-29T19:41:21Z

@mickmister, I followed the steps found inside #69.

It would be great to have this PR for doing this. I will have to get back to that ticket and see if I can repro much of the changes needed for the syncing in this PR.

mickmister · 2020-07-29T19:45:37Z

@jfrerich Any reason we can't use the plugin repo sync tool? Here's my test drive of using it mattermost-community/mattermost-plugin-todo#103 , including the plan.yml I used to sync. You can adjust the original plan.yml as you see fit.

jfrerich · 2020-07-29T19:49:29Z

Nice. I hadn't seen the sync tool in use! I'll have to test it out also and see how it reacts against my changes.

…update package.json files.\n add eslint exception rule to test so it passes eslint

jfrerich · 2020-07-29T20:38:56Z

So, I did find some changes that I missed. I also have quite a few differences with .editorconfig and webapp/.eslintrc.json.

.circleci/config.yml` was drastically reduced.
.editorconfig updated
webapp/.eslintrc.json updated

jfrerich · 2020-08-03T17:48:16Z

ok! There were quite a few commits needed after addressing those latest comments (syncing webpack, package.json, socket API), but this is ready for re-review

hanzei

LGMT 👍

build/sync/plan.yml

mickmister

Nothing blocking but I'm curious about some of the string changes.

mickmister · 2020-08-07T17:00:49Z

server/service/get_subscription.go

@@ -8,7 +8,7 @@ import (
 	"github.com/mattermost/mattermost-plugin-confluence/server/serializer"
 )

-const generalError = "Some error occurred. Please try again after sometime."
+const generalError = "some error occurred. Please try again after sometime"


Just out of curiosity, why was this changed to lowercase? Same with other similar string changes.

Also a grammar suggestion:

Suggested change

const generalError = "some error occurred. Please try again after sometime"

const generalError = "some error occurred. Please try again after some time"

What about the grammar suggestions regarding capitalization? Is there a reason why this was made lowercase?

It was a linting error, so I changed it

What was the linting error? It seems incorrect to me to display an error message that begins with a lowercase letter, when it is meant to be a sentence. Definitely something we want to fix in our lint settings rather than doing it this way 2/5

Here is the explanation for the linting error. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

I understand their reasoning for the check. I also checked several other plugins. These plugins already adhere to the lint check

I sort of disagree with it, if you're trying to have full sentences in an error message. But I think they are advising to not have full sentences in the error description portion of a log for example.

mickmister · 2020-08-07T17:04:43Z

webapp/src/client/client.js

+        this.baseUrl = url.protocol + '//' + url.host;
+        this.pluginUrl = this.baseUrl + '/plugins/' + Constants.PLUGIN_NAME;
+        this.apiUrl = this.baseUrl + '/api/v4';
+        this.pluginApiUrl = this.pluginUrl + '/api/v1';


Why were these changed to using the + syntax?

Leaving these with the ${url.protocol} causes eslint to fail. I don't like the change, but made it to move forward with the PR

webapp/webpack.config.js

mickmister · 2020-08-07T17:08:02Z

webapp/src/constants/action_types.js

-    RECEIVED_SUBSCRIPTION: `${PLUGIN_NAME}_received_subscription`,
+    OPEN_SUBSCRIPTION_MODAL: PLUGIN_NAME + '_open_subscription_modal',
+    CLOSE_SUBSCRIPTION_MODAL: PLUGIN_NAME + '_close_subscription_modal',
+    RECEIVED_SUBSCRIPTION: PLUGIN_NAME + '_received_subscription',


Is something in the eslint config suggesting this change?

Unfortunately yes this caused eslint to fail.

What rules caused it to fail? The config should be updated to support the better syntax

Apparently this is a bug eslint/eslint#13248

webapp/src/constants/action_types.js

use id, from webapp/src/manifest.js, instead of hardcoded PLUGIN_NAME from webapp/src/constants/manifest

jfrerich · 2020-08-12T03:47:08Z

@mickmister I went ahead and addressed some of your comments in another commit. Can you please re-review?

DHaussermann

Tested and Passed

Build and deployed
Briefly regression tested
No issues found no further functional testing needed for this PR
LGTM!

Release testing will take place on next version bump.

jfrerich changed the title ~~initial add~~ [GH-54] sync with latest mattermost-plugin-starter-template Jul 27, 2020

jfrerich force-pushed the GH-54-make-apply branch 3 times, most recently from 5e7c446 to 832471c Compare July 27, 2020 23:48

sync with latest starter-template files

321dfe9

golangci-lint fixes make check-style fixes broken test fixes

jfrerich force-pushed the GH-54-make-apply branch from 832471c to 321dfe9 Compare July 27, 2020 23:51

hanzei linked an issue Jul 28, 2020 that may be closed by this pull request

Integrate GolangCI-Lint #69

Closed

jfrerich added 2 commits July 28, 2020 10:09

update local-prefixes

ae567e0

fix goimports lint error

12eb78f

jfrerich marked this pull request as ready for review July 28, 2020 15:46

jfrerich requested review from hanzei and mickmister and removed request for hanzei July 28, 2020 15:47

jfrerich self-assigned this Jul 28, 2020

jfrerich added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 28, 2020

jfrerich linked an issue Jul 28, 2020 that may be closed by this pull request

make apply produces invalid files #54

Closed

jfrerich added 4 commits July 29, 2020 15:05

add build/sync/plan.yml file

c70fc75

add build/sync/plan.yml file

e111387

udpate to latest starter-tempalte

23f6440

update .editorconfig and .eslintrc.json to match starter-template.\n …

0013910

…update package.json files.\n add eslint exception rule to test so it passes eslint

jfrerich added 5 commits August 3, 2020 12:00

add react-select package

94b73d1

fix golangci-lint error

1f1fa54

npm install --save react-dom

a1ca3da

update min_server_version with make apply

591c544

remove build/bin dir

b4379e2

jfrerich requested a review from hanzei August 3, 2020 17:46

hanzei approved these changes Aug 3, 2020

View reviewed changes

build/sync/plan.yml Outdated Show resolved Hide resolved

compbine commone paths

9e76826

hanzei unassigned jfrerich Aug 5, 2020

hanzei added the Awaiting Submitter Action Blocked on the author label Aug 7, 2020

mickmister approved these changes Aug 7, 2020

View reviewed changes

jfrerich added 3 commits August 11, 2020 22:22

remove constants/manifest.js

4f2061d

use id, from webapp/src/manifest.js, instead of hardcoded PLUGIN_NAME from webapp/src/constants/manifest

Merge branch 'master' into GH-54-make-apply

a1a3145

update after merging master

0fd40c2

jfrerich requested a review from mickmister August 12, 2020 03:46

jfrerich removed the Awaiting Submitter Action Blocked on the author label Aug 12, 2020

jfrerich self-assigned this Aug 18, 2020

Merge branch 'master' into GH-54-make-apply

93d55b1

mickmister approved these changes Aug 22, 2020

View reviewed changes

jfrerich removed the 2: Dev Review Requires review by a core committer label Aug 24, 2020

jfrerich requested a review from DHaussermann August 24, 2020 18:05

DHaussermann approved these changes Aug 31, 2020

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 Aug 31, 2020

jfrerich merged commit 3cbffad into master Sep 3, 2020

jfrerich deleted the GH-54-make-apply branch September 3, 2020 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-54] sync with latest mattermost-plugin-starter-template #68

[GH-54] sync with latest mattermost-plugin-starter-template #68

jfrerich commented Jul 27, 2020 •

edited

Loading

codecov-commenter commented Jul 27, 2020 •

edited

Loading

jfrerich commented Jul 28, 2020

mickmister commented Jul 29, 2020 •

edited

Loading

jfrerich commented Jul 29, 2020

mickmister commented Jul 29, 2020

jfrerich commented Jul 29, 2020

jfrerich commented Jul 29, 2020

jfrerich commented Aug 3, 2020

hanzei left a comment

mickmister left a comment

mickmister Aug 7, 2020 •

edited

Loading

mickmister Aug 17, 2020

jfrerich Aug 17, 2020

mickmister Aug 17, 2020

jfrerich Aug 21, 2020

jfrerich Aug 21, 2020

mickmister Aug 22, 2020

mickmister Aug 7, 2020

jfrerich Aug 12, 2020

mickmister Aug 7, 2020

jfrerich Aug 12, 2020

mickmister Aug 12, 2020

jfrerich Aug 12, 2020

jfrerich commented Aug 12, 2020

DHaussermann left a comment

	const generalError = "some error occurred. Please try again after sometime"
	const generalError = "some error occurred. Please try again after some time"

[GH-54] sync with latest mattermost-plugin-starter-template #68

[GH-54] sync with latest mattermost-plugin-starter-template #68

Conversation

jfrerich commented Jul 27, 2020 • edited Loading

Summary

Ticket Link

codecov-commenter commented Jul 27, 2020 • edited Loading

Codecov Report

jfrerich commented Jul 28, 2020

mickmister commented Jul 29, 2020 • edited Loading

jfrerich commented Jul 29, 2020

mickmister commented Jul 29, 2020

jfrerich commented Jul 29, 2020

jfrerich commented Jul 29, 2020

jfrerich commented Aug 3, 2020

hanzei left a comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

mickmister Aug 7, 2020 • 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

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

jfrerich commented Aug 12, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

jfrerich commented Jul 27, 2020 •

edited

Loading

codecov-commenter commented Jul 27, 2020 •

edited

Loading

mickmister commented Jul 29, 2020 •

edited

Loading

mickmister Aug 7, 2020 •

edited

Loading