mattermost-plugin-jira
mattermost-plugin-jira copied to clipboard
mattermost
[GH-507,699] Add Support for DM subscriptions for watchers and reporter
Summary
- In order to get a DM notification from the Jira plugin, the user has to meet certain criteria like being assigned or mentioned. But now even if a user is a reporter or watching that issue will also get a DM notification.
- This PR contains all the changes from Pr #840 So this PR alone is enough to fix this issue so we can close Pr #840 afterward.
Issue
- Fixes https://github.com/mattermost/mattermost-plugin-jira/issues/699
- Fixes https://github.com/mattermost/mattermost-plugin-jira/issues/507
- Pr #840
Hello @Kshitij-Katiyar,
Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.
@Kshitij-Katiyar @Nityanand13 This is what I mean, I think changing the way we store the data in the struct would make us do much less code and this code would be easier to test.
https://replit.com/@javaguirre/GH-507699#main.go
Let me know what you think!
@javaguirre Sorry for the late reply. Your approach seems good and now I am implementing the same.
Could you post a video on how to use this new change? Something like this so it's easier to QA.
https://github.com/mattermost/mattermost-plugin-jira/pull/863#issuecomment-1200475963
Thank you!
Could you post a video on how to use this new change? Something like this so it's easier to QA.
Thank you!
Vedio link for this PR:
- https://drive.google.com/file/d/1lSTDElqTwI1_XTwunTtaSCepWroMyb7Z/view?usp=sharing
- https://drive.google.com/file/d/1Xpi9jdftdPk0NBLZ8yROJmjMLhKRuK82/view?usp=sharing
Thank you very much @Nityanand13 ! Let me know when testing is done @Kshitij-Katiyar . :+1:
@Nityanand13 Can't open the videos in any of the PRs, can you check the extension of the files and tell me what kind of video is so I can open it? Also If I could already open it online would be nice, with something like loom or whatever video Drive supports.
Thank you very much!
@Nityanand13 Can't open the videos in any of the PRs, can you check the extension of the files and tell me what kind of video is so I can open it? Also If I could already open it online would be nice, with something like loom or whatever video Drive supports.
Thank you very much!
Vedio link for this PR:
The CI is failing @Nityanand13 , let me know if you can check it out! Thank you!
Codecov Report
Patch coverage: 38.84% and project coverage change: +0.44 :tada:
Comparison is base (
7a5c616) 30.14% compared to head (d769010) 30.59%.
Additional details and impacted files
@@ Coverage Diff @@
## master #872 +/- ##
==========================================
+ Coverage 30.14% 30.59% +0.44%
==========================================
Files 49 49
Lines 7467 7629 +162
==========================================
+ Hits 2251 2334 +83
- Misses 5027 5094 +67
- Partials 189 201 +12
| Impacted Files | Coverage Δ | |
|---|---|---|
| server/client.go | 7.46% <0.00%> (-0.58%) |
:arrow_down: |
| server/client_cloud.go | 0.00% <ø> (ø) |
|
| server/client_server.go | 0.00% <ø> (ø) |
|
| server/command.go | 15.72% <0.00%> (-0.35%) |
:arrow_down: |
| server/http.go | 43.67% <ø> (-0.65%) |
:arrow_down: |
| server/plugin.go | 9.50% <0.00%> (+0.02%) |
:arrow_up: |
| server/user_cloud.go | 0.00% <0.00%> (ø) |
|
| server/user_server.go | 6.50% <0.00%> (-0.40%) |
:arrow_down: |
| server/webhook.go | 29.13% <0.00%> (-1.96%) |
:arrow_down: |
| server/webhook_worker.go | 0.00% <0.00%> (ø) |
|
| ... and 4 more |
... and 1 file with indirect coverage changes
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
@Kshitij-Katiyar @Nityanand13 Is this ready to be reviewed again? Please request the review again so I effectively know I have to review again and we don't lose time, thank you!
@DHaussermann Given the high impact of this feature, I'm wondering what's the bare minimum we need to do here to get an RC on community with this change
@DHaussermann On second thought, are we supposed to avoid shipping features like this right now? cc @m1lt0n
@mickmister cc: @DHaussermann I believe so Michael, since it's not strictly a bug fix, but a change in the business logic. We can discuss sync during our planning calls.
@Kshitij-Katiyar Heads up that there are conflicts to resolve
@hanzei Fixed merge conflicts
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
Hey, is there still something blocking this issue? I'd love to have the option to get notifications on tickets, that I watch.
@Kshitij-Katiyar Can you please resolve the merge conflicts here? Thank you :+1:
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
@Kshitij-Katiyar Can you please resolve the merge conflicts here? Thank you 👍
@mickmister sure resolving them.
@Kshitij-Katiyar I think we should tell the user why they received a notification. e.g. "because you are watching x" or "because you reported x"