[MM-44185] Add conditions for subscription validation

[mickmister]

Summary

This PR adds more checks to the validateSubscription function, as well as adding more test cases.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-44185

[mickmister]

Not sure why CI is failing:

cd webapp && /usr/local/bin/npm install
make: *** [Makefile:68: webapp/.npminstall] Terminated

Too long with no output (exceeded 10m0s): context deadline exceeded

[mickmister]

Fixed CI with most recent commit

[mickmister]

@DHaussermann Gentle ping on review for this

[DHaussermann]

/update-branch

[codecov-commenter]

Codecov Report

Patch coverage: 61.29% and project coverage change: +0.23 🎉

Comparison is base (0ba44b3) 30.91% compared to head (9239901) 31.15%.

❗ Current head 9239901 differs from pull request most recent head a84d6af. Consider uploading reports for the commit a84d6af to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
+ Coverage   30.91%   31.15%   +0.23%     
==========================================
  Files          50       50              
  Lines        7332     7405      +73     
==========================================
+ Hits         2267     2307      +40     
- Misses       4884     4907      +23     
- Partials      181      191      +10     

Impacted Files	Coverage Δ
server/issue.go	30.41% <ø> (ø)
server/plugin.go	11.65% <ø> (ø)
server/user.go	26.70% <0.00%> (-0.43%)	⬇️
server/subscribe.go	65.26% <64.77%> (-0.92%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[DHaussermann]

@mickmister - Not sure if I'm seeing a deployment issue or if something else is happening here. I don't see a new filter as expected for Comment Visibility so I am unable to do any functional testing for this.

I have the matching commit deployed. Any thoughts here? Maybe I've misunderstood something here?

[mickmister]

@DHaussermann and I have discussed offline the scope of this PR, which does not include changes to comment visibility

[jupenur]

@mickmister following up on our brief chat yesterday, what's missing to get this merged? Just the QA review? cc @DHaussermann

[mickmister]

@jupenur Yes QA review is the next step here

[DHaussermann]

/update-branch

[DHaussermann]

I have done a round of testing and this is looking good.

• Tested that security level filter is no longer opt-in on upgrade
• Tested that the "kill switch" works as expected to continue subscription delivery
• Regression tested security level filter is working
• Labeling is system console has been improved for clarity
• Subscription creation screen now has user feed back that there is now security level filter set when this is the case

When I tried to repeat this testing on server (testing above was on cloud) I get an error saving the subscription

@mickmister couple questions...

• Is there any chance this change is agnostic to Jira Server Vs Jira Cloud? to me this seems like a risk.
• The error above seems unrelated to the PR but can you confirm if it's happening on our side or on the Jira side?

[DHaussermann]

I tested this again and it is working as expected in this branch provided @mickmister here https://github.com/mattermost/mattermost-plugin-jira/tree/validate-subscription_debug.

[DHaussermann]

@m1lt0n or @asatkinson we should discuss the impact of this change and if clients need to be warned about it.
This PR really should have some type of Business Analysts approval.

Testing this has caused me to wonder how common the case is where people use projects in Jira with no issue security scheme applied. In such a case, users won't quickly be able to go edit their subscription to get it working again once they upgrade the plugin. Some effort is required in Jira as well to associate the project with a security scheme.

In short...
Issue where a scheme is applied and the security level is set to none works fine and no changes are needed to the subscription. But our solution here kind of assumes all projects have issue security schemes applied.
For any project where there is no issue security scheme in use there will be no way of delivering subscriptions unless the "kill switch" to make it work the old way is turned on to not require security level.

[m1lt0n]

@asatkinson Since this is a business decision, what do you propose to be the recommended course of action with this? cc: @DHaussermann

Let's bring this up in either our grooming or planning or async in our chat.

[asatkinson]

@DHaussermann I've been looking through this and I don't think I fully understand why this is so problematic.
It seems like they just want a switch so that if this filter is set then we restrict subscription notification to those posts below a certain security level - as defined in this JIRA help doc

Can we do a quick call so you can walk me through this? I don't have access to this UI.

[asatkinson]

If the issue is that it applies this and prevents all other subscriptions from working, then yes, this is a big problem and we should find another way to do this.

[mickmister]

@DHaussermann Gentle ping to give this PR another look. I've resolved issues in CI

[hanzei]

@mickmister Heads up that there is a conflict to resolve

[DHaussermann]

/update-branch

[DHaussermann]

After discussing with @mickmister again the last concern I had is not applicable here.

I have checked again and tested on both Cloud and Server. If the issue is part of a project where there is no security level to assign because no level is in use by the project, the subscription will deliver the event even though the subscription has no applicable security level to evaluate.

[DHaussermann]

Tested and passed

• On upgrade, subscription with security level continues to work
• On upgrade, you can add a security level to a subscription to make it start delivering again
• On Upgrade there is valid feedback that Exclude operator can no longer be used for security level when user open edit view to see why it's not working.
  
• "Kill switch" works to revert to old logic
• Updating a subscription to add security level works
• Targeting a project where there is no applicable security level works as expected without the filter as none is applicable

LGTM!

Thanks @mickmister for the effort on this.