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 :tada:

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

:exclamation: 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%)	:arrow_down:
server/subscribe.go	65.26% <64.77%> (-0.92%)	:arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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]

@mickmister I did a round of testing and this seems to be working as expected.

Is there a reason this is not just a bool radio button? Also - Can I try and tweak the wording on the option for clarity?

Also - With this design Security Level is no longer optional. The UI should reflect this when a new subscription is being created. Right now it's just allowing users to create non-functional subscriptions with no feedback.

[asatkinson]

@DHaussermann what should I be checking for here?

• UI, default values etc.?
• my one comment would be does this need to be turned on by default, or just in high-security shops. I can definitely see why there are cases not enabling this would be useful. If this is just turned on will it break any existing subscriptions?

[DHaussermann]

I have discussed this with @mickmister.

1. For now, a compromise would be to add the following text below the "approximate JQL" line: "Note that since you have not selected a security level filter, the subscription will only allow issues that have no security level assigned." The text would then disappear once a security level is added.

2. I think the label of the enforcement flag would be more clear if we set the text to: "Subscriptions will only include issues that have a security level assigned if the appropriate security level has been included as a filter"

3. A correction from above

"With this design Security Level is no longer optional. The UI should reflect this when ...."

This is not true. Technically they are still optional. Without a filter only issues with no security level assigned are included. This is a valid use case.

cc @asatkinson

[mickmister]

@DHaussermann I've updated the PR with the changes mentioned above. Note that the sentence wraps around with one word on the second line. This looks a little asymmetrical/odd to me:

[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.