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

Fix risk triggers #522

Merged
merged 4 commits into from
Feb 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
15 changes: 14 additions & 1 deletion app/models/risk_assessment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,20 @@ def initialize(notice)
end

def high_risk?
risk_triggers.any? { |trigger| trigger.risky?(notice) }
risky = false

risk_triggers.each do |trigger|
trigger_risky = trigger.risky?(notice)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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


if trigger_risky && trigger.force_not_risky_assessment?
risky = false
break
end

risky = true if trigger_risky
end

risky
end

private
Expand Down
31 changes: 10 additions & 21 deletions app/models/risk_trigger.rb
Original file line number Diff line number Diff line change
@@ -1,29 +1,18 @@
class RiskTrigger < ActiveRecord::Base
def risky?(notice)
if notice.submitter.try(:email) == 'google@lumendatabase.org' &&
notice.try(:type) == 'Defamation'
false
else
begin
field_present?(notice) && condition_matches?(notice)
rescue NoMethodError => ex
Rails.logger.warn "Invalid risk trigger (#{id}): #{ex}"
false
end
end
end
ALLOWED_MATCHING_TYPES = %w[any all].freeze

private
has_many :risk_trigger_conditions, dependent: :destroy

def field_present?(notice)
notice.send(field).present?
end
validates_presence_of :name, :matching_type
validates_inclusion_of :matching_type, in: ALLOWED_MATCHING_TYPES

def risky?(notice)
return false unless risk_trigger_conditions.any?

def condition_matches?(notice)
if negated?
notice.send(condition_field) != condition_value
if matching_type == 'any'
risk_trigger_conditions.any? { |condition| condition.risky?(notice) }
else
notice.send(condition_field) == condition_value
risk_trigger_conditions.all? { |condition| condition.risky?(notice) }
end
end
end
62 changes: 62 additions & 0 deletions app/models/risk_trigger_condition.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
class RiskTriggerCondition < ActiveRecord::Base
ALLOWED_FIELDS = (
Notice.attribute_names.map { |field| 'notice.' + field } +
Entity.attribute_names.map do |field|
%w[submitter recipient principal sender].map do |entity|
entity + '.' + field
end
end.flatten
).freeze

ALLOWED_MATCHING_TYPES = %w[exact broad].freeze

belongs_to :risk_trigger

validates_presence_of :field, :value
validates_inclusion_of :field, in: ALLOWED_FIELDS
validates_inclusion_of :matching_type, in: ALLOWED_MATCHING_TYPES

def risky?(notice)
field_present?(notice) && condition_matches?(notice)
rescue NoMethodError => ex
Rails.logger.warn 'Invalid risk trigger condition (trigger condition ' \
"id:#{id}, notice id: #{notice.id}): #{ex}"
false
end

private

def field_present?(notice)
notice_field_value(notice).present?
end

def condition_matches?(notice)
notice_field_value = notice_field_value(notice)

if matching_type == 'exact'
match_exact(notice_field_value)
else
match_broad(notice_field_value)
end
end

def notice_field_value(notice)
field.gsub('notice.', '').split('.').inject(notice, :send)
end

def match_exact(notice_field_value)
if negated?
notice_field_value != value
else
notice_field_value == value
end
end

def match_broad(notice_field_value)
if negated?
!notice_field_value.include?(value)
else
notice_field_value.include?(value)
end
end
end
25 changes: 25 additions & 0 deletions config/initializers/rails_admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,29 @@ def custom_work_label
end
end
end

config.model 'RiskTriggerCondition' do
edit do
configure :field, :enum do
enum do
RiskTriggerCondition::ALLOWED_FIELDS.sort
end
end
configure :matching_type, :enum do
enum do
RiskTriggerCondition::ALLOWED_MATCHING_TYPES
end
end
end
end

config.model 'RiskTrigger' do
edit do
configure :matching_type, :enum do
enum do
RiskTrigger::ALLOWED_MATCHING_TYPES
end
end
end
end
end
12 changes: 12 additions & 0 deletions db/migrate/20190122150704_rename_risk_triggers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class RenameRiskTriggers < ActiveRecord::Migration
def change
rename_table :risk_triggers, :risk_trigger_conditions
remove_column :risk_trigger_conditions, :field
rename_column :risk_trigger_conditions, :condition_field, :field
rename_column :risk_trigger_conditions, :condition_value, :value
add_column :risk_trigger_conditions, :type, :string
add_column :risk_trigger_conditions, :matching_type, :string
change_column_null :risk_trigger_conditions, :field, false
change_column_null :risk_trigger_conditions, :value, false
end
end
44 changes: 44 additions & 0 deletions db/migrate/20190122151538_create_new_risk_triggers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
class CreateNewRiskTriggers < ActiveRecord::Migration
def change
# Create a new table
create_table :risk_triggers do |t|
t.string :name, null: false
t.string :matching_type, null: false
t.string :comment
t.boolean :force_not_risky_assessment, default: false, null: false
end

# Add a reference field to the risk_trigger_conditions
add_reference :risk_trigger_conditions, :risk_trigger, index: true

# Create a default risk trigger
default_risk_trigger = RiskTrigger.create(
name: 'Default',
matching_type: 'all'
)

# Assign all the exisiting conditions to the default trigger
RiskTriggerCondition.update_all(risk_trigger_id: default_risk_trigger)
peter-hank marked this conversation as resolved.
Show resolved Hide resolved

# Create a risk trigger and its conditions from the ones that were hard-coded in the RiskTrigger model
google_risk_trigger = RiskTrigger.create(
name: 'Google Defamation',
matching_type: 'all',
force_not_risky_assessment: true
)
RiskTriggerCondition.create(
field: 'notice.type',
value: 'Defamation',
negated: false,
risk_trigger: google_risk_trigger,
matching_type: 'exact'
)
RiskTriggerCondition.create(
field: 'submitter.email',
value: 'google@lumendatabase.org',
negated: false,
risk_trigger: google_risk_trigger,
matching_type: 'exact'
)
end
end
21 changes: 16 additions & 5 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20180815162606) do
ActiveRecord::Schema.define(version: 20190122151538) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
Expand Down Expand Up @@ -229,11 +229,22 @@
add_index "relevant_questions_topics", ["relevant_question_id"], name: "index_relevant_questions_topics_on_relevant_question_id", using: :btree
add_index "relevant_questions_topics", ["topic_id"], name: "index_relevant_questions_topics_on_topic_id", using: :btree

create_table "risk_triggers", force: :cascade do |t|
t.string "field"
t.string "condition_field"
t.string "condition_value"
create_table "risk_trigger_conditions", force: :cascade do |t|
t.string "field", null: false
t.string "value", null: false
t.boolean "negated"
t.string "type"
t.string "matching_type"
t.integer "risk_trigger_id"
end

add_index "risk_trigger_conditions", ["risk_trigger_id"], name: "index_risk_trigger_conditions_on_risk_trigger_id", using: :btree

create_table "risk_triggers", force: :cascade do |t|
t.string "name", null: false
t.string "matching_type", null: false
t.string "comment"
t.boolean "force_not_risky_assessment", default: false, null: false
end

create_table "roles", force: :cascade do |t|
Expand Down
16 changes: 11 additions & 5 deletions db/seeds/risk_triggers.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
RiskTrigger.create!(
field: :body,
condition_field: :country_code,
condition_value: 'us',
negated: true
risk_trigger = RiskTrigger.create!(
name: 'Submitter country code not US',
matching_type: 'all'
)

RiskTriggerCondition.create!(
field: 'submitter.country_code',
value: 'us',
negated: true,
risk_trigger: risk_trigger,
matching_type: 'exact'
)
Loading