Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#6402] Warning notes on request preview #7963

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .ruby-style.yml
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ Layout/LineLength:
- "^\\s*context\\s+.*do$"
- "^\\s*describe\\s+.*do$"
- "^RSpec\\.describe(\\s+|\\().*do$"
- "^RSpec\\.shared_examples(\\s+|\\().*do.*$"
- "^\\s*class\\s+[A-Z].*<.*"
Exclude:
- bin/setup
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ def new
return
end

@outgoing_message.update_taggable_terms

# Show preview page, if it is a preview
return render_new_preview if params[:preview].to_i == 1

Expand Down
77 changes: 77 additions & 0 deletions app/models/concerns/taggable_terms.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
require 'set'

# Apply tags to a Taggable record based on configured attributes matching terms
# mapped to tags get applied when there is a match.
#
# Example:
#
# Record.taggable_terms = { body: { /foo/ => 'foo' } }
# r = Record.new
# r.tagged?('foo')
# # => false
#
# r.update(body: 'foo bar baz')
# r.tagged?('foo')
# # => true
module TaggableTerms
extend ActiveSupport::Concern

included do
cattr_accessor :taggable_terms, default: {}
before_save :update_taggable_terms, if: :taggable_term_attribute_changed?
end

def update_taggable_terms
tags_to_add, tags_to_remove = taggable_terms_changed_tags
tags_to_add.each { |tag| add_tag_if_not_already_present(tag) }
tags_to_remove.each { |tag| remove_tag(tag) }
end

private

def taggable_term_attribute_changed?
changed.include?(taggable_terms.keys)
end

def taggable_terms_changed_tags
tags_to_add = Set.new
tags_to_remove = Set.new

taggable_terms_to_tag_to_attr_terms.each do |tag, attr_terms_pairs|
tag_str = tag.to_s

attr_terms_pairs.each do |attr, term|
if attribute_matches_taggable_term?(attr, term)
tags_to_add << tag_str
break
end
end

tags_to_remove << tag_str unless tags_to_add.include?(tag_str)
end

[tags_to_add, tags_to_remove]
end

def attribute_matches_taggable_term?(attr, term)
read_attribute(attr) =~ Regexp.new(term)
end

# Restructure taggable_terms so that it's more processing friendly
#
# Before:
# => {:body=>{/train/i=>"trains", /bus/i=>"bus", /locomotive/i=>"trains"}}
#
# After:
# => {"trains"=>[[:body, /train/i], [:body, /locomotive/i]],
# "bus"=>[[:body, /bus/i]]}
Comment on lines +66 to +67
Copy link
Member

@gbp gbp Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought of using Regexp.union so this would return something like:

Suggested change
# => {"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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought of using Regexp.union so 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.

def taggable_terms_to_tag_to_attr_terms
seed = Hash.new { |h, k| h[k] = [] }

taggable_terms.each_with_object(seed) do |(attr, terms_tags), memo|
terms_tags.each do |term, tag|
memo[tag] << [attr, term]
end
end
end
end
2 changes: 2 additions & 0 deletions app/models/outgoing_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ class OutgoingMessage < ApplicationRecord
include MessageProminence
include Rails.application.routes.url_helpers
include LinkToHelper
include Notable
include Taggable
include TaggableTerms

STATUS_TYPES = %w(ready sent failed).freeze
MESSAGE_TYPES = %w(initial_request followup).freeze
Expand Down
2 changes: 2 additions & 0 deletions app/views/request/preview.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

<div class="message-preview">
<div class="preview-advice">
<%= render_notes(@outgoing_message.all_notes) %>

<div class="advice-panel">
<ul>
<li><%= _('Check you haven\'t included any <strong>personal information</strong>.') %></li>
Expand Down
33 changes: 33 additions & 0 deletions spec/models/concerns/taggable_terms.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
RSpec.shared_examples 'concerns/taggable_terms' do |*factory_opts, attr_under_test|
let(:record) { FactoryBot.build(*factory_opts) }

let(:terms_tags) do
{ /train/i => 'trains',
/bus/i => 'bus',
/locomotive/i => 'trains',
/bike/ => 'bicycles' }
end

before do
record.taggable_terms = { attr_under_test => terms_tags }
end

describe '#update_taggable_terms' do
subject { record.update_taggable_terms }
before { subject }

it 'applies a tag when a term matches' do
expect(record).to be_tagged('bus')
end

it 'does not apply a tag when there is no match for a term' do
expect(record).not_to be_tagged('bicycles')
end

it 'applies a tag when one term term matches but a later term with the same tag does not' do
# i.e. keep "trains" because it matched /train/, so don't remove it
# because it didn't match /locomotive/
expect(record).to be_tagged('trains')
end
end
end
4 changes: 4 additions & 0 deletions spec/models/outgoing_message_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@
require 'spec_helper'
require 'models/concerns/message_prominence'
require 'models/concerns/taggable'
require 'models/concerns/taggable_terms'

RSpec.describe OutgoingMessage do
it_behaves_like 'concerns/message_prominence', :initial_request
it_behaves_like 'concerns/taggable', :initial_request
it_behaves_like 'concerns/taggable_terms',
:initial_request, { body: 'Some trains and a bus.' },
:body

describe '.is_searchable' do
subject { described_class.is_searchable }
Expand Down
Loading