search

mattermost / mattermost-plugin-gitlab

GitLab plugin for Mattermost
Apache License 2.0
137 stars 83 forks source link

Webhooks MM userId not GitlabUserId #126

Open manland opened 4 years ago

manland commented 4 years ago

WebHooks received GitlabId bu we use an internal method which look for MMUserID. Change this behavior by storing gitlabId in kvStore or use loginGitlab wich is already stored in kvStore.

I also see some strange behaviour related to webhooks and I may have found the reason, but need an extra pair of eyes here, maybe I'm missing something.

@manland, please correct me if I'm wrong,

In https://github.com/mattermost/mattermost-plugin-gitlab/blob/master/server/plugin.go#L152

p.API.KVSet(userID + GITLAB_IDUSERNAME_KEY, []byte(gitlabUsername) // userID is Mattermost user ID

But in https://github.com/mattermost/mattermost-plugin-gitlab/blob/master/server/plugin.go#L167

p.API.KVGet(gitlabUserID + GITLAB_IDUSERNAME_KEY) // gitlabUserID is GitLab user ID

This mismatch makes GITLAB_IDUSERNAME_KEY useless and ToUsers will never be filled by webhooks.

Originally posted by @kop in https://github.com/mattermost/mattermost-plugin-gitlab/issues/121#issuecomment-565140183

manland commented 4 years ago

@hanzei please feel free to update description and/or labels.

olafik commented 4 years ago

@manland @hanzei @kop

This mismatch makes GITLAB_IDUSERNAME_KEY useless and ToUsers will never be filled by webhooks.

If this is true then it seems that the functions handleDMMergeRequest and handleMention achieve nothing? – e.g. line 43 in server/webhook/merge_request.go But the handleChannelMergeRequest function would still work – as it seems to be in my case! (see my comment at #147)

Doesn't this issue imply that one of the core functionalities of this plugin does not work? Namely:

Notifications - get a direct message in Mattermost when someone mentions you, requests your review, comments on or modifies one of your merge requests/issues, or assigns you on GitLab

It would fit my experience... but I find it strange: I guess that anyone stumbling upon this plugin does so because he/she is looking for the "Notifications" functionality – the project subscription feature is already covered by Gitlab iirc. And it seems that relevant lines of code weren't changed since the first commit.

hanzei commented 4 years ago

Indeed, From look at the code it seams like some parts of the notification system are broken.

@olafik Would you be interested in submitting a fix?

haardikdharma10 commented 3 years ago

Hi @hanzei, I'd like to fix this. Can you help me on how to get started. Additionally, if you can help me on how the flow of editing will go, that'd be great as well :)

hanzei commented 3 years ago

Hey @haardikdharma10, Thanks for taking a shot. :+1: Do the docs at https://github.com/mattermost/mattermost-plugin-gitlab#development help you with getting started?