[#6402] Warning notes on request preview #7963
Conversation
garethrees
force-pushed
the
6402-warning-notes-on-request-preview
branch
3 times, most recently
from
e5c448f
to
3962e7e
Compare
These can get a bit long but much like other rspec stuff, single line is better than splitting over multiple.
Allow a record to be tagged based on attributes matching given terms.
Refactored to reduce performance issues. As the size of `taggable_terms` grows. Each call to `tag_applied_via_other_taggable_term?` involves iterating over potentially the entire `taggable_terms[attr]` hash, which can get expensive. Additionally, because this is nested within two loops in `update_taggable_terms`, you have a cubic time complexity in the worst-case scenario.
Use TaggableTerms to check OutgoingMessage#body for terms that we have a
tag-based Note for.
We can use this to warn users that they’re about to make a mistake.
For example, we could configure OutgoingMessage like so:
OutgoingMessage.taggable_terms = {
body: {
/my 999 call/i => ‘preview_warning_sar’
}
}
Then, if a user includes “my 999 call” in their request content, we’d
detect this and render a Note applied via the ‘preview_warning_sar’ tag.
garethrees
force-pushed
the
6402-warning-notes-on-request-preview
branch
from
dc26f5b
to
5600427
Compare
| # => {"trains"=>[[:body, /train/i], [:body, /locomotive/i]], | ||
| # "bus"=>[[:body, /bus/i]]} |
There was a problem hiding this comment.
Have you thought of using Regexp.union so this would return something like:
| # => {"trains"=>[[:body, /train/i], [:body, /locomotive/i]], | |
| # "bus"=>[[:body, /bus/i]]} | |
| # => {"trains"=>[[:body, /(?i-mx:train)|(?i-mx:locomotive)/]], | |
| # "bus"=>[[:body, /bus/i]]} |
Which should make taggable_terms_changed_tags simpler.
Also what about if the body contains say "constrain", we could in theory convert the terms into a Regexp (if not already) and wrap between word boundaries EG: \b...\b
There was a problem hiding this comment.
Have you thought of using
Regexp.unionso this would return something like:
I did think about whether it might be worth munging into a single regexp. Are there any performance issues with that past a certain point?
Also what about if the body contains say "constrain", we could in theory convert the terms into a Regexp (if not already) and wrap between word boundaries EG:
\b...\b
In real use I'm expecting the regexps to be more like /my 999 call/, /my visa application/, etc – more phrases than singular words. I think if we do want to match a singular word (SAR) for example, then when we configure it we explicitly include things in the regexp that reduces the chances of false positives.
Relevant issue(s)
A version of #6402
What does this do?
Show tag-based notes on new request preview if the message content matches a configured term.
Why was this needed?
Allow us to detect terms in messages and warn users before they go on to submit their request
Implementation notes
Could use on_change to update the tags after any update to an attribute, rather than only doing so before saving.
Screenshots
Notes to reviewer
remove_tag(tag.to_s) unless tag_applied_via_other_taggable_term?(tag, attr)seems like it could get quite computationally expensive. Might be a nicer way of doing this part.