[GH-130] Adds a reminder when subscribing to a project that a web hook is needed. #134

ericjaystevens · 2020-02-16T23:03:22Z

Summary

Adds a reminder when using /github subscribe that a project hook is needed if one is not already pointing to the MatterMosts server URL.

Webhooks in Gitlab can be created in GitLab at the System, project, or group level (paid plans only). The go-gitlab module used in this project does not yet support listing group hooks ( issue created ). System hooks won't be accessible for most developers. So we are just checking for project hooks, and if we are unsure if a webhook exists, providing a link to our documentation on how to set one up.

Ticket Link

Fixes #130

gabrieljackson · 2020-02-17T00:09:36Z

Thanks for working on this @ericjaystevens.

A few things before we start reviewing:

I believe the linked ticket URL may have been a typo, so please correct that if so.
Please squash/fixup/etc. your commits a bit so that we start with a cleaner git history.
If possible, please increase coverage a bit on tests to the codecov tests pass.

Let us know if you need any help with this!

ericjaystevens · 2020-02-25T03:05:27Z

@gabrieljackson Thanks for the feedback on this pull request.

I have updated the linked ticket URL, squashed my commits, and improved coverage.

gabrieljackson · 2020-02-27T17:29:30Z

Thanks @ericjaystevens. Reviewers have been added.

aaronrothschild

Thanks for your contribution @ericjaystevens ! This should help avoid users wondering "why is't that repo notification working? I just set it up..."

jfrerich

LGTM, @ericjaystevens! This will help some confusion on the end-uses. I look forward to seeing the implementation for a webhook syncing via mattermost slash commands :)

mickmister

@ericjaystevens Awesome work! No issues with implementation, but I have some comments on style and mocking. Thanks for adding this feature 👍

go.mod

server/command.go

server/gitlab/api.go

server/command.go

… used. (#130) Message alerting the user to the need to add a webhook is functioning. Additional refactoring and tests needed. single webhook test with mocking refactored project hook tests added no webhooks test moved mock function to gitlabtest package replaced manual mocks with auto generated refactored plugin command handling and clarified messages added api error test removed mocks that are no longer needed removed test for GetProjectHooks as it seems to implementation-specific go format removed shadow declaration based on vet output broke out subscribeComand method to improve testability tested no subscripions paramaterized testSubscribeCommand tests mocked KVSet tests webhook found moved tests out of plugin_test and into command_test fixed linting issues fixed Error identifier Co-Authored-By: Michael Kochell <mjkochell@gmail.com> fixed Error identifier Co-Authored-By: Michael Kochell <mjkochell@gmail.com> spelling Co-Authored-By: Michael Kochell <mjkochell@gmail.com> generated mock with gomock works for all test except no Substitutions all tests pass changed mock boolean flag name WIP

ericjaystevens · 2020-03-05T23:43:30Z

@mickmister I think I addressed all the issues based on your great feedback. Let me know if you need anything else.

go.mod

go.sum

codecov · 2020-03-07T00:34:20Z

Codecov Report

Merging #134 into master will increase coverage by 3.30%.
The diff coverage is 52.77%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   36.58%   39.89%   +3.30%     
==========================================
  Files          16       16              
  Lines        1465     1494      +29     
==========================================
+ Hits          536      596      +60     
+ Misses        881      838      -43     
- Partials       48       60      +12

Impacted Files	Coverage Δ
server/api.go	`0.00% <ø> (ø)`
server/configuration.go	`15.38% <ø> (+5.12%)`	⬆️
server/subscriptions.go	`47.57% <ø> (+17.47%)`	⬆️
server/utils.go	`38.46% <ø> (+3.07%)`	⬆️
server/webhook.go	`41.17% <ø> (ø)`
server/webhook/webhook.go	`57.14% <ø> (ø)`
server/command.go	`17.07% <45.16%> (+17.07%)`	⬆️
server/plugin.go	`11.64% <100.00%> (+4.93%)`	⬆️
... and 1 more

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 331ba44...7125b7d. Read the comment docs.

ericjaystevens · 2020-03-07T00:40:48Z

Removed all extra lines at the beginning of functions and cleaned up go.sum and go.mod. Let me know if there is anything else I can do. Thanks again for all the feedback to improve this PR.

mickmister

LGTM! Thanks @ericjaystevens 👍

DHaussermann

@ericjaystevens thanks for this improvement. This is excellent for improving useably and preventing user oversight!
I did find a minor issue with this. It seems like the link you've added for "setup a WebHook" in GitLab may be outdated. it's pointing to <project name>/-/settings/integrations when I click it, I see a GitLab page reminding me that the config has moved to <project name>/hooks

Aside from the link address...

Ensured post appears when target project does not have a hook
Ensured post does not appear when there is already a hook in place
Ensured functionality works as expected with subgroups
Briefly regression tested subscription functionality
No other issues found.

mickmister

@ericjaystevens Thanks for the fix on the webhook link. LGTM!

ericjaystevens · 2020-03-10T22:26:41Z

@DHaussermann Good Catch the URL must have recently changed. Thanks! I just pushed with the updated link. Let me know if you need anything else.

I forgot to update the URL's in the tests in my last commit, just pushed those changes up.

DHaussermann

@ericjaystevens thanks for this fix.

Testing noted in the comment above.
Updated Release Testing to cover this case.
No other issues found.
LGTM!

Help should output `/gitlab pipelines run` instead of `/gitlab pipeline run`

gabrieljackson added the Work In Progress Not yet ready for review label Feb 17, 2020

gabrieljackson requested review from aaronrothschild, cpoile, DHaussermann and jfrerich February 27, 2020 17:28

gabrieljackson added 1: PM Review Requires review by a product manager 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 Feb 27, 2020

aaronrothschild approved these changes Feb 28, 2020

View reviewed changes

aaronrothschild removed the 1: PM Review Requires review by a product manager label Feb 28, 2020

cpoile requested review from mickmister and removed request for cpoile February 28, 2020 16:11

jfrerich approved these changes Mar 2, 2020

View reviewed changes

mickmister requested changes Mar 4, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

server/command.go Outdated Show resolved Hide resolved

server/command.go Outdated Show resolved Hide resolved

server/gitlab/api.go Outdated Show resolved Hide resolved

server/command.go Outdated Show resolved Hide resolved

hanzei reviewed Mar 6, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

hanzei reviewed Mar 6, 2020

View reviewed changes

go.sum Outdated Show resolved Hide resolved

hanzei requested a review from mickmister March 6, 2020 09:19

cleaned up go files and extra lines

018b34f

mickmister approved these changes Mar 10, 2020

View reviewed changes

mickmister removed the 2: Dev Review Requires review by a core committer label Mar 10, 2020

DHaussermann reviewed Mar 10, 2020

View reviewed changes

updated to new project hook link

c42471b

mickmister self-requested a review March 10, 2020 21:38

mickmister approved these changes Mar 10, 2020

View reviewed changes

updated hook creation URL in tests

7125b7d

DHaussermann approved these changes Mar 10, 2020

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Mar 10, 2020

mickmister merged commit 43a23dc into mattermost:master Mar 11, 2020

hanzei added this to the v1.2.0 milestone Mar 11, 2020

iomodo mentioned this pull request Jul 29, 2020

Bump version to 1.2.0 #191

Merged

DieAkuteSense added a commit to DieAkuteSense/mattermost-plugin-gitlab that referenced this pull request Jun 21, 2023

mattermost#134 Fix Typo in /gitlab help output

86d994f

Help should output `/gitlab pipelines run` instead of `/gitlab pipeline run`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-130] Adds a reminder when subscribing to a project that a web hook is needed. #134

[GH-130] Adds a reminder when subscribing to a project that a web hook is needed. #134

ericjaystevens commented Feb 16, 2020 •

edited

gabrieljackson commented Feb 17, 2020

ericjaystevens commented Feb 25, 2020

gabrieljackson commented Feb 27, 2020

aaronrothschild left a comment

jfrerich left a comment

mickmister left a comment

ericjaystevens commented Mar 5, 2020

codecov bot commented Mar 7, 2020 •

edited

ericjaystevens commented Mar 7, 2020

mickmister left a comment

DHaussermann left a comment

mickmister left a comment

ericjaystevens commented Mar 10, 2020

DHaussermann left a comment

[GH-130] Adds a reminder when subscribing to a project that a web hook is needed. #134

[GH-130] Adds a reminder when subscribing to a project that a web hook is needed. #134

Conversation

ericjaystevens commented Feb 16, 2020 • edited

Summary

Ticket Link

gabrieljackson commented Feb 17, 2020

ericjaystevens commented Feb 25, 2020

gabrieljackson commented Feb 27, 2020

aaronrothschild left a comment

Choose a reason for hiding this comment

jfrerich left a comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

ericjaystevens commented Mar 5, 2020

codecov bot commented Mar 7, 2020 • edited

Codecov Report

ericjaystevens commented Mar 7, 2020

mickmister left a comment

Choose a reason for hiding this comment

DHaussermann left a comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

ericjaystevens commented Mar 10, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

ericjaystevens commented Feb 16, 2020 •

edited

codecov bot commented Mar 7, 2020 •

edited