Fix risk triggers

[peter-hank]

Ready for merge?

YES

What does this PR do?

Updates the risk triggers functionality. The current implementation is not complete and not useful.

How can a reviewer manually see the effects of these changes?

Create a new risk trigger and risk trigger conditions. Assign the conditions to the trigger and try to add a new notice, properly configured it should make the notice as a risky one.

What are the relevant tickets?

• https://cyber.harvard.edu/projectmanagement/issues/15785

Screenshots (if appropriate)

Todo:

• Tests
  - [ ] Documentation
• Stakeholder approval

Requires Database Migrations?

YES

Includes new or updated dependencies?

NO

[coveralls]

Pull Request Test Coverage Report for Build 872

• 42 of 42 (100.0%) changed or added relevant lines in 2 files are covered.
• 12 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.02%) to 83.624%

---

Files with Coverage Reduction	New Missed Lines	%
lib/tasks/lumen.rake	6	23.71%
app/models/notice.rb	6	95.71%

Totals
Change from base Build 835:	0.02%
Covered Lines:	1777
Relevant Lines:	2125

---

💛 - Coveralls

[thatandromeda]

Thanks for writing so many tests! This is definitely a place where we want to check a bunch of logical possibilities.

Please address the questions above and fill out the TO DO items in the PR.

Some things I need to check once that's done:

• run the migration on localhost
• check anything indicated by TO DO items
• reread tests, make sure I understand them

[thatandromeda]

I'm nervous about this long hardcoded list; it runs the risk of becoming out of sync with reality if our model definitions change.

Why are these the allowed fields? And can we write some kind of test that will warn us if the underlying semantics end up out of sync? (For instance, if this is a comprehensive list of fields on particular models, can we check that RiskTriggerCondition::ALLOWED_FIELDS in fact equals Work.attribute_names plus Notice.attribute_names plus whatever else?)

A comment explaining why these are the fields we care about would also be helpful, in case we need to update it in future.

[peter-hank]

It was just cleaner to have a list of fields that we know that they will be in use. After rethinking this maybe it won't be so problematic for users to select from every possible field. I've updated the code.

[thatandromeda]

Original logic (cells represent outcome of risky?:

.	email = google@lumendatabase.org	email = something else
Type = Defamation	false	?
Type = something else	?	?

The ? means "other risk triggers are applied if they exist, so the outcome may or may not be risky".

This logic (I think; I'm finding it hard to follow with all the negations):

.	email = google@lumendatabase.org	email = something else
Type = Defamation	false && false, hence false	true && false, hence false
Type = something else	false && true, hence false	true && true, hence true

Two conclusions:

1. This logic does not match the original and should be amended.
2. Although I get why the negations are in there, this system might be extremely confusing to use and result in unintentional errors. Can we provide a less expressive system and still meet Adam's needs? (Possibly we want any all none as options on the RiskTrigger, rather than negated on the conditional?)

[peter-hank]

1. It should now match the original implementation.
2. Adding none won't be able to replace the negated field. If you have, for example, two conditions:

• title == "Something"
• body != "Something else"
  in the same risk trigger then you won't be able to do the same using the none option.

I will create good documentation for Adam, I don't think it will be difficult to understand.

[thatandromeda]

Is it correct that this each logic boils down to trigger.force_not_risky_assessment? && trigger.risky?(notice)?

If so, I think high_risk? is just

risk_triggers.any? do |trigger|
 trigger.force_not_risky_assessment? && trigger.risky?(notice)
end

[peter-hank]

@thatandromeda no really, if any of risk_triggers is force_not_risky_assessment? and is risky? then the assessment is not risky?, it would be risky? in your example

[thatandromeda]

It's been nagging at me how tricky it's been for me to follow the logic as a code reviewer, which has made me concerned that we're setting Adam up for outcomes he doesn't expect, as different risk triggers interact in the wild. So I talked through this feature with him face-to-face to get a clearer sense of how he intends to use it (e.g. "if it comes from Google and has the magic email address, but it ALSO trips one of our other risk triggers, do you want it to be marked as risky or not-risky?").

I'm now feeling comfortable that the intended use of the feature is that if any risk triggers come up positive, the notice should be marked as risky, and we should not have a force_not_risky_assessment element. (The hard-coded not-risky Google stuff reflects some business logic that used to be important, but isn't any more.) So the correct thing to do is back out of the force_not_risky_assessment logic entirely; remove the Google-specific risk triggers; and go back to high_risk? being a simple any? condition on risk triggers.

Sorry for all the confusion on this one.

[peter-hank]

the notice should be marked as risky, and we should not have a force_not_risky_assessment element

Awesome, thanks, will update it soon.

[peter-hank]

the notice should be marked as risky, and we should not have a force_not_risky_assessment element

Awesome, thanks, will update it soon.

@thatandromeda done!

[thatandromeda]

👍 🌈 🎆 💯