RHS Subscriptions #333

lieut-data · 2022-11-22T18:58:27Z

Summary

Highlight channel subscriptions in the RHS:

When there are no subscriptions:

And when not signed in:

For this change, I tried to stick with the existing dependencies as I didn't want to take on ownership of upgrading package-lock.json. (The styles would have been much clearer with styled-components, or even SASS.)

For context on this feature, see the private channel https://community.mattermost.com/private-core/channels/gitlab-strategic-integration.

Expose a RHS experience for GitLab that showcases the current channels's subscriptions, as well as information on the user currently logged in.

lieut-data · 2022-11-22T19:05:33Z

The tests are failing, it seems because there simply is no test target for the webapp. Wondering if something changed in the upstream orbs?

lieut-data · 2022-11-23T19:02:06Z

server/plugin.go

+	p.API.PublishWebSocketEvent(
+		WsChannelSubscriptionsUpdated,
+		map[string]interface{}{"payload": string(payloadJSON)},
+		&model.WebsocketBroadcast{ChannelId: channelID},


This constrains the websocket event to members of the channel, /but/ I see there is logic in the plugin around some users being denied access to some subset of projects. Worse, it looks like these permission checks actually involve a round trip per user -- is there some background information I can learn about?

IIUC, but isn't it implied that access to the channel gives access to the subscription information?
So shouldn't this be left to channel admins (by controlling channel memberships)?

@lieut-data It looks like the /gitlab subscriptions list command does not filter on any sort of permission checks, which is sort of the equivalent of this websocket message right? I'm thinking we may not need any further access control other than scoping it to the specific channel, as you've done here.

Awesome, thank you for this feedback -- will leave this as-is.

codecov · 2022-11-25T19:31:33Z

Codecov Report

Base: 28.48% // Head: 33.06% // Increases project coverage by +4.57% 🎉

Coverage data is based on head (2b6f434) compared to base (0d69bb5).
Patch coverage: 65.62% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   28.48%   33.06%   +4.57%     
==========================================
  Files          20       20              
  Lines        2619     2683      +64     
==========================================
+ Hits          746      887     +141     
+ Misses       1789     1689     -100     
- Partials       84      107      +23

Impacted Files	Coverage Δ
server/command.go	`26.34% <50.00%> (+0.11%)`	⬆️
server/plugin.go	`15.38% <53.84%> (+5.84%)`	⬆️
server/api.go	`23.20% <75.00%> (+23.19%)`	⬆️
server/subscriptions.go	`50.98% <0.00%> (+3.92%)`	⬆️
server/utils.go	`72.97% <0.00%> (+35.13%)`	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ashishbhate

LGTM, I don't know the Gitlab side of things very well (or plugins in general, tbh) so my review focuses on the actual written code.

I'll defer to the other reviewers on the high-level view.

ashishbhate · 2022-11-26T05:06:21Z

server/api.go

+
+	resp := subscriptionsToResponse(config, subscriptions)
+
+	b, _ := json.Marshal(resp)


We should probably check the err returned here. IIRC, we've had some incidents on the server around JSON Marshalling so a quick error check helps.

ashishbhate · 2022-11-26T05:15:03Z

server/plugin.go

+	p.API.PublishWebSocketEvent(
+		WsChannelSubscriptionsUpdated,
+		map[string]interface{}{"payload": string(payloadJSON)},
+		&model.WebsocketBroadcast{ChannelId: channelID},


IIUC, but isn't it implied that access to the channel gives access to the subscription information?
So shouldn't this be left to channel admins (by controlling channel memberships)?

mickmister

Great work @lieut-data! I left some non-blocking suggestions and questions. Let me know what you think 👍

mickmister · 2022-11-25T19:34:20Z

.nvmrc

@@ -0,0 +1 @@
+16.4.0


This will cause CI to use this npm version, which is ahead of this project's lockfile version. When you ran npm install with npm 16.4.0, did you receive the message of patching up the lockfile? We indeed need to update the project to use node 16+

Ah, good catch! When my first attempt to compile and deploy failed, I assumed it was the node version and mistakenly copied the version from webapp, then opted not to git add package-lock.json but forgot about my changes here in .nvmrc. I'll fix this to point at the latest v14 here which I believe uses the old lock version.

Can you please ensure this works locally with nvm install or nvm use if you already have this version installed?

If you do test this out, please remove the node_modules folder before testing. Likely, there won't be any issues since CI is passing

Gah, I forgot to revert this. One second.

Wait, sorry, I've got too much on the go...

Yes, I can confirm this .nvmrc works locally. What I did forget to revert was the addition of styled-components to package.json.

(Also triple checked after blowing away node_modules).

server/api.go

webapp/src/components/rhs_sidebar/index.js

webapp/src/components/rhs_sidebar/rhs_sidebar.jsx

webapp/src/actions/index.js

mickmister · 2022-11-28T14:41:16Z

webapp/src/components/rhs_sidebar/rhs_sidebar.jsx

+const notSignedInStyle = {
+    margin: '24px',
+    display: 'flex',
+    flexDirection: 'column',
+    alignItems: 'center',
+    textAlign: 'center',
+};


@lieut-data Thoughts on putting these styles in a css file? It's unfortunate that scss isn't available but I think using a regular css file should be manageable in this case

Actually, now that you've helped me with the node version, I wonder if we'd be open to just depending on the external styled-components vs. having to export global CSS rules and deal with that complexity? Let me take a stab at seeing how that plays out.

@lieut-data In general, our plugins have been using scss. I know Playbooks is using styled-components and no scss now, though I'd like to keep consistency with the other plugin projects our team manages in this aspect.

Using css instead of scss can be a pain, because you don't have the same easily accessible nesting capabilities with css. Of course there's the equivalent syntax in css, which may not be too bad in this case.

I'll leave it up to you to decide how to apply styles here. I'm 2/5 on not introducing styled-components here though. Hopefully you can understand why 🙂

I'm happy to keep things simpler for the maintainers and will switch to CSS.

webapp/src/components/rhs_sidebar/rhs_sidebar.jsx

mickmister · 2022-11-28T15:06:31Z

server/plugin.go

+	p.API.PublishWebSocketEvent(
+		WsChannelSubscriptionsUpdated,
+		map[string]interface{}{"payload": string(payloadJSON)},
+		&model.WebsocketBroadcast{ChannelId: channelID},


@lieut-data It looks like the /gitlab subscriptions list command does not filter on any sort of permission checks, which is sort of the equivalent of this websocket message right? I'm thinking we may not need any further access control other than scoping it to the specific channel, as you've done here.

ashishbhate · 2022-11-28T16:06:52Z

server/api.go

+
+		subscriptionResponses = append(subscriptionResponses, SubscriptionResponse{
+			RepositoryName: subscription.Repository,
+			RepositoryURL:  gitlabURL.JoinPath(subscription.Repository).String(),


I just ran into this while deploying:

JoinPath was added in Go 1.19 https://pkg.go.dev/net/url#URL.JoinPath

The go.mod for this project states that it works with go 1.16.

The mattermost-server project requires go 1.18.

Thank you for this: I didn't even check the supported versions on this method!

mickmister

LGTM! Just one request to remove the styled-components dependency. Great work @lieut-data!

mickmister · 2022-11-29T18:39:47Z

webapp/src/components/rhs_sidebar/rhs_sidebar.css

+    color: var(--center-channel-color);
+}
+
+.gitlab-rhs-Connect.gitlab-rhs-Connect.gitlab-rhs-Connect {


Just so I understand, is the selector repeated to increase specificity?

mickmister · 2022-11-29T18:41:20Z

webapp/package.json

@@ -15,7 +16,8 @@
        "react": "16.8.6",
        "react-bootstrap": "0.32.4",
        "react-redux": "5.1.1",
-        "redux": "4.0.1"
+        "redux": "4.0.1",
+        "styled-components": "5.3.3"


We can remove this since styled-components is not being used anymore

mickmister · 2022-11-29T18:45:46Z

@spirosoik I'll leave it up to you if you would like to review before merge, or we can call this dev reviewed since @ashishbhate and I both looked at it. As @ashishbhate mentioned, he's not as familiar with the project, which is why I'm giving you a decision here. Please let me know your thoughts

lieut-data · 2022-11-29T18:57:11Z

@spirosoik & @DHaussermann, is there anything you'd like to take a peek at before we merge?

DHaussermann · 2022-11-30T06:41:09Z

Hi @lieut-data I took a look at this PR. This is woking as expected. I did see a coupe issues. Points 1. and 2. maybe worth investigating before we merge to see if there are quick solutions.

Connection state and state where the signed in user is shown seem to work fine
RHS is correctly aware of current channel open when fetching list and updated in real time as channels change
New elements have reasonable contrast in all themes
Performed CRUD testing on subscription
Adds, Edit and Deletes do all work to update the RHS view

The App Bar icon works as expected. But, there is no fail-over icon shown in the channel header if App Bar is in the off state (Also likely no legacy monochromatic icon exists for GitLab yet)
When deleting a subscription a "refresh" event such as a channel change or RHS open / close must occur for the newly deleted subscription to no longer show. (Using the add command to change the feature list of an existing subscription does update in real time
When App Bar is first enabled a webapp refresh is required to see the icon. No issue if the App bar is already on when the plugin is enabled. 0/5 This feels like an App Bar issue not specific to GitLab a vaguely remember seeing this before. @mickmister may know about what causes this.

spirosoik

LGTM

lieut-data · 2022-11-30T13:48:14Z

Thanks again, @DHaussermann!

re: 1, I'm syncing with Neil on next steps: stay tuned!

re: 2, I'm not currently able to reproduce, with subscriptions being added and deleted as the slash commands are being invoked. Is there any extra detail you can help provide on how you reproduced?

Subscriptions.-.Websockets.mp4

re: 3, I agree with your assessment.

lieut-data · 2022-11-30T23:23:03Z

re: 1, I synced with Neil and we agreed to keep this "apps bar only". Thanks for raising that, @DHaussermann :)

DHaussermann · 2022-12-01T00:30:00Z

@lieut-data for 1. this is a bit strange I saw this 2 or 3 times in a row so, I thought it was consistently reproducible. Now I can not see the issue either. Delete works in real time. I wonder if something else may have been happening on the server at the time? I've also gone back and checked 2 other servers and they are working normally as well.

Give the relatively low impact and that I have no concrete details. I feel we should consider it resolved. If I see this again - now that I know it intermittent - I'll see if there is info on the network tab I can capture and the I can open a separate issue to prioritize and address.

DHaussermann

Tested and passed as per testing notes above ☝️

For the issues found:

No change will be made and the icon will only show in the App Bar
Is no longer reproducible. Dylan will open a new issue with console details if it is seen again
Is not specific to the GitHub integration

As such no further code changes or functional testing needed for this PR.
LGTM!

@lieut-data Please merge when you have a chance.

lieut-data · 2022-12-01T00:39:25Z

Sweet, thank you @DHaussermann :)

lieut-data added 2 commits November 15, 2022 21:34

add .nvmrc

9941bc2

GitLab subscriptions RHS

8920f9c

Expose a RHS experience for GitLab that showcases the current channels's subscriptions, as well as information on the user currently logged in.

lieut-data added the 2: Dev Review Requires review by a core committer label Nov 22, 2022

lieut-data requested a review from ashishbhate November 22, 2022 18:58

lieut-data requested a review from mickmister as a code owner November 22, 2022 18:58

lieut-data requested a review from DHaussermann November 23, 2022 15:21

lieut-data commented Nov 23, 2022

View reviewed changes

lieut-data added 4 commits November 23, 2022 15:29

fix PropTypes

1ecac6f

ensure subscription key is unique

03b7162

refactor subscriptionsr response

f24413b

show subscription features

ec9e14e

lieut-data requested a review from spirosoik as a code owner November 25, 2022 16:07

implement empty webapp test script

e4a0d01

ashishbhate approved these changes Nov 26, 2022

View reviewed changes

mickmister reviewed Nov 28, 2022

View reviewed changes

ashishbhate reviewed Nov 28, 2022

View reviewed changes

lieut-data added 9 commits November 28, 2022 14:28

avoid re-render on new array

e58be27

avoid unnecessary Promise.all

371487f

nvm v14.21.1

27c734a

consistent action return types

eea88e7

enhance PropTypes with isRequired

492e49d

ensure unique subscriptions key

e915ef0

avoid depending on url.JoinPath from Go 1.19

44dd034

switch to styled-components

bf59419

log error on failed marshal

7ad1d82

lieut-data requested a review from mickmister November 28, 2022 20:10

switch to vanilla CSS

2bb90be

hanzei added this to the v1.6.0 milestone Nov 29, 2022

hanzei added the 3: QA Review Requires review by a QA tester label Nov 29, 2022

hanzei mentioned this pull request Nov 29, 2022

Release v1.6.0 #337

Closed

5 tasks

mickmister approved these changes Nov 29, 2022

View reviewed changes

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

lieut-data added 2 commits November 29, 2022 14:48

revert package.json

8b7d471

restore npm test

2b6f434

spirosoik approved these changes Nov 30, 2022

View reviewed changes

DHaussermann approved these changes Dec 1, 2022

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 Dec 1, 2022

lieut-data merged commit 2275492 into master Dec 1, 2022

lieut-data deleted the rhs-subscriptions branch December 1, 2022 00:39

mickmister mentioned this pull request Jan 24, 2023

[GH-61]: RHS Support #300

Merged


		resp := subscriptionsToResponse(config, subscriptions)

		b, _ := json.Marshal(resp)

		@@ -0,0 +1 @@
		16.4.0

RHS Subscriptions #333

RHS Subscriptions #333

Conversation

lieut-data commented Nov 22, 2022

Summary

lieut-data commented Nov 22, 2022

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

codecov bot commented Nov 25, 2022 • edited

Codecov Report

ashishbhate left a 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

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

Choose a reason for hiding this comment

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

mickmister commented Nov 29, 2022

lieut-data commented Nov 29, 2022

DHaussermann commented Nov 30, 2022

spirosoik left a comment

Choose a reason for hiding this comment

lieut-data commented Nov 30, 2022 • edited

lieut-data commented Nov 30, 2022

DHaussermann commented Dec 1, 2022 • edited

DHaussermann left a comment

Choose a reason for hiding this comment

lieut-data commented Dec 1, 2022

codecov bot commented Nov 25, 2022 •

edited

lieut-data commented Nov 30, 2022 •

edited

DHaussermann commented Dec 1, 2022 •

edited