From acfd9f35a8b8da00694bd8c5c0a9ddae3c87c314 Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Wed, 17 Apr 2024 15:40:03 +0200 Subject: [PATCH 1/8] Start using STI for decision model --- .../reports_modal_component.html.haml | 3 +- .../app/components/reports_modal_component.rb | 3 -- .../controllers/webui/decisions_controller.rb | 2 +- src/api/app/models/decision.rb | 11 +++++- src/api/app/models/decision_cleared.rb | 11 ++++++ src/api/app/models/decision_favored.rb | 34 +++++++++++++++++++ src/api/app/policies/appeal_policy.rb | 4 +-- .../app/policies/decision_favored_policy.rb | 2 ++ .../webui/appeals/_list_decision.html.haml | 6 ++-- ...et_the_decision_type_from_decision_kind.rb | 15 ++++++++ src/api/db/data_schema.rb | 2 +- ...ation_action_description_component_spec.rb | 2 ++ 12 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 src/api/app/models/decision_favored.rb create mode 100644 src/api/app/policies/decision_favored_policy.rb create mode 100644 src/api/db/data/20240417122944_set_the_decision_type_from_decision_kind.rb diff --git a/src/api/app/components/reports_modal_component.html.haml b/src/api/app/components/reports_modal_component.html.haml index af787938e36..363d1bc847a 100644 --- a/src/api/app/components/reports_modal_component.html.haml +++ b/src/api/app/components/reports_modal_component.html.haml @@ -21,8 +21,7 @@ -# Decision-related canned responses exclusively: = render CannedResponsesDropdownComponent.new(canned_responses) = form.text_area(:reason, class: 'form-control mb-3', required: true, placeholder: 'Reason for the decision') - -# TODO: this should be adapted when we move to Decision#type - = form.select(:kind, DECISION_KIND_MAP.to_a, {}, class: 'form-select') + = form.select(:type, Decision::TYPES, {}, class: 'form-select') .modal-footer - reports.each do |report| = form.hidden_field(:report_ids, multiple: true, value: report.id) diff --git a/src/api/app/components/reports_modal_component.rb b/src/api/app/components/reports_modal_component.rb index 9080ef23700..b6b7b365773 100644 --- a/src/api/app/components/reports_modal_component.rb +++ b/src/api/app/components/reports_modal_component.rb @@ -1,9 +1,6 @@ class ReportsModalComponent < ApplicationComponent attr_reader :reportable, :reportable_name, :user, :reports - # TODO: temporary solution until Decision#type replaces Decision#kind with new values - DECISION_KIND_MAP = { 'cleared' => 'cleared', 'favored' => 'favor' }.freeze - def initialize(reportable:, reportable_name:, user:, reports:) super diff --git a/src/api/app/controllers/webui/decisions_controller.rb b/src/api/app/controllers/webui/decisions_controller.rb index c62419c15ca..e5774621f15 100644 --- a/src/api/app/controllers/webui/decisions_controller.rb +++ b/src/api/app/controllers/webui/decisions_controller.rb @@ -19,6 +19,6 @@ def create private def decision_params - params.require(:decision).permit(:reason, :kind, report_ids: []) + params.require(:decision).permit(:reason, :type, report_ids: []) end end diff --git a/src/api/app/models/decision.rb b/src/api/app/models/decision.rb index c76aa03e975..c210e65f86e 100644 --- a/src/api/app/models/decision.rb +++ b/src/api/app/models/decision.rb @@ -1,4 +1,6 @@ class Decision < ApplicationRecord + TYPES = { favored: 'DecisionFavored', cleared: 'DecisionCleared' }.freeze + validates :reason, presence: true, length: { maximum: 65_535 } validates :type, presence: true, length: { maximum: 255 } @@ -6,6 +8,7 @@ class Decision < ApplicationRecord has_many :reports, dependent: :nullify + # TODO: Remove this after type is deployed enum kind: { cleared: 0, favor: 1 @@ -14,8 +17,13 @@ class Decision < ApplicationRecord after_create :create_event after_create :track_decision + def description + 'The moderator decided on the report' + end + private + # TODO: Replace this with `AbstractMethodCalled` after type is deployed def create_event case kind when 'cleared' @@ -29,8 +37,9 @@ def event_parameters { id: id, moderator_id: moderator.id, reason: reason, report_last_id: reports.last.id, reportable_type: reports.first.reportable.class.name } end + # TODO: Remove kind after type is deployed def track_decision - RabbitmqBus.send_to_bus('metrics', "decision,kind=#{kind} hours_before_decision=#{hours_before_decision},count=1") + RabbitmqBus.send_to_bus('metrics', "decision,kind=#{kind},type=#{type} hours_before_decision=#{hours_before_decision},count=1") end def hours_before_decision diff --git a/src/api/app/models/decision_cleared.rb b/src/api/app/models/decision_cleared.rb index 0b73ae7eb90..f366eb7af6f 100644 --- a/src/api/app/models/decision_cleared.rb +++ b/src/api/app/models/decision_cleared.rb @@ -1,4 +1,15 @@ class DecisionCleared < Decision + after_create :create_event + + def description + 'The moderator decided to clear the report' + end + + private + + def create_event + Event::ClearedDecision.create(event_parameters) + end end # == Schema Information diff --git a/src/api/app/models/decision_favored.rb b/src/api/app/models/decision_favored.rb new file mode 100644 index 00000000000..cf4f7b5025a --- /dev/null +++ b/src/api/app/models/decision_favored.rb @@ -0,0 +1,34 @@ +class DecisionFavored < Decision + after_create :create_event + + def description + 'The moderator decided to favor the report' + end + + private + + def create_event + Event::FavoredDecision.create(event_parameters) + end +end + +# == Schema Information +# +# Table name: decisions +# +# id :bigint not null, primary key +# kind :integer default("cleared") +# reason :text(65535) not null +# type :string(255) not null, default("DecisionCleared") +# created_at :datetime not null +# updated_at :datetime not null +# moderator_id :integer not null, indexed +# +# Indexes +# +# index_decisions_on_moderator_id (moderator_id) +# +# Foreign Keys +# +# fk_rails_... (moderator_id => users.id) +# diff --git a/src/api/app/policies/appeal_policy.rb b/src/api/app/policies/appeal_policy.rb index eece1b313c9..d0b4f3d46ca 100644 --- a/src/api/app/policies/appeal_policy.rb +++ b/src/api/app/policies/appeal_policy.rb @@ -31,12 +31,12 @@ def create? def decision_cleared_report_from_user? return false unless record.appellant == user - record.decision.kind == 'cleared' && record.decision.reports.pluck(:user_id).include?(user.id) + record.decision.type == 'DecisionCleared' && record.decision.reports.pluck(:user_id).include?(user.id) end def decision_favored_report_of_action_from_user? return false unless record.appellant == user - record.decision.kind == 'favor' && "#{@report.reportable_type}Policy".constantize.new(user, @report.reportable).update? + record.decision.type == 'DecisionFavored' && "#{@report.reportable_type}Policy".constantize.new(user, @report.reportable).update? end end diff --git a/src/api/app/policies/decision_favored_policy.rb b/src/api/app/policies/decision_favored_policy.rb new file mode 100644 index 00000000000..309a2459f1b --- /dev/null +++ b/src/api/app/policies/decision_favored_policy.rb @@ -0,0 +1,2 @@ +class DecisionFavoredPolicy < DecisionPolicy +end diff --git a/src/api/app/views/webui/appeals/_list_decision.html.haml b/src/api/app/views/webui/appeals/_list_decision.html.haml index 8f382c03e17..f52943489da 100644 --- a/src/api/app/views/webui/appeals/_list_decision.html.haml +++ b/src/api/app/views/webui/appeals/_list_decision.html.haml @@ -2,7 +2,7 @@ .card-body %h3 Decision %p - The moderator decided to - = decision.kind - the reports. The reason was: + = succeed '.' do + = decision.description + The reason was: = decision.reason diff --git a/src/api/db/data/20240417122944_set_the_decision_type_from_decision_kind.rb b/src/api/db/data/20240417122944_set_the_decision_type_from_decision_kind.rb new file mode 100644 index 00000000000..07c945abf1e --- /dev/null +++ b/src/api/db/data/20240417122944_set_the_decision_type_from_decision_kind.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class SetTheDecisionTypeFromDecisionKind < ActiveRecord::Migration[7.0] + def up + Decision.in_batches do |batch| + batch.each do |decision| + decision.type = decision.favor? ? 'DecisionFavored' : 'DecisionCleared' + end + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/src/api/db/data_schema.rb b/src/api/db/data_schema.rb index f573bffb548..21ba9312435 100644 --- a/src/api/db/data_schema.rb +++ b/src/api/db/data_schema.rb @@ -1 +1 @@ -DataMigrate::Data.define(version: 20240321103332) +DataMigrate::Data.define(version: 20240417122944) diff --git a/src/api/spec/components/notification_action_description_component_spec.rb b/src/api/spec/components/notification_action_description_component_spec.rb index bed58d66f61..957e808c78b 100644 --- a/src/api/spec/components/notification_action_description_component_spec.rb +++ b/src/api/spec/components/notification_action_description_component_spec.rb @@ -1,3 +1,5 @@ +require 'decision' # To break the circular dependency between Decision and DecisionFavored and DecisionCleared + RSpec.describe NotificationActionDescriptionComponent, type: :component do context 'when the notification is for a Event::RequestStatechange event with a request having only a target' do let(:target_project) { create(:project, name: 'project_123') } From e459815ce63478b90722ea3004a131cf90acc4cb Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Thu, 18 Apr 2024 10:53:08 +0200 Subject: [PATCH 2/8] Make sure that tests work with the new decision sti --- src/api/spec/factories/decisions.rb | 6 +++--- src/api/spec/factories/notification.rb | 2 +- src/api/spec/features/beta/webui/appeals_spec.rb | 2 +- src/api/spec/features/beta/webui/decisions_spec.rb | 2 +- src/api/spec/mailers/event_mailer_spec.rb | 6 +++--- src/api/spec/policies/appeal_policy_spec.rb | 10 +++++----- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/api/spec/factories/decisions.rb b/src/api/spec/factories/decisions.rb index 1c4f489f55c..518af546282 100644 --- a/src/api/spec/factories/decisions.rb +++ b/src/api/spec/factories/decisions.rb @@ -8,11 +8,11 @@ end trait :cleared do - kind { 'cleared' } + type { 'DecisionCleared' } end - trait :favor do - kind { 'favor' } + trait :favored do + type { 'DecisionFavored' } end end end diff --git a/src/api/spec/factories/notification.rb b/src/api/spec/factories/notification.rb index dafed73d090..77d9ac6d7ba 100644 --- a/src/api/spec/factories/notification.rb +++ b/src/api/spec/factories/notification.rb @@ -103,7 +103,7 @@ trait :favored_decision do event_type { 'Event::FavoredDecision' } - notifiable { association(:decision, :favor) } + notifiable { association(:decision, :favored) } after(:build) do |notification| notification.event_payload['reportable_type'] ||= notification.notifiable.reports.first.reportable.class.to_s diff --git a/src/api/spec/features/beta/webui/appeals_spec.rb b/src/api/spec/features/beta/webui/appeals_spec.rb index 57eaf8a4e3c..c31daa2cd45 100644 --- a/src/api/spec/features/beta/webui/appeals_spec.rb +++ b/src/api/spec/features/beta/webui/appeals_spec.rb @@ -10,7 +10,7 @@ let(:user) { create(:confirmed_user) } let(:comment) { create(:comment_package, user: user) } let(:report) { create(:report, reportable: comment) } - let!(:decision) { Decision.create!(kind: Decision.kinds['favor'], reason: "It's spam indeed", reports: [report], moderator: moderator) } + let!(:decision) { Decision.create!(type: 'DecisionFavored', reason: "It's spam indeed", reports: [report], moderator: moderator) } before do EventSubscription.create!(eventtype: Event::FavoredDecision.name, diff --git a/src/api/spec/features/beta/webui/decisions_spec.rb b/src/api/spec/features/beta/webui/decisions_spec.rb index 7bca9d7bf2e..f5f177896c3 100644 --- a/src/api/spec/features/beta/webui/decisions_spec.rb +++ b/src/api/spec/features/beta/webui/decisions_spec.rb @@ -11,7 +11,7 @@ def fill_decisions_modal(reportable) within("#reports-modal-#{reportable.class.to_s.downcase}-#{reportable.id}") do fill_in id: 'decision_reason', with: 'Reason for reporting is correct.' - select('favor', from: 'decision[kind]') + select('favored', from: 'decision[type]') click_button('Submit') end end diff --git a/src/api/spec/mailers/event_mailer_spec.rb b/src/api/spec/mailers/event_mailer_spec.rb index 62964878f78..34ec31e82a5 100644 --- a/src/api/spec/mailers/event_mailer_spec.rb +++ b/src/api/spec/mailers/event_mailer_spec.rb @@ -492,7 +492,7 @@ let!(:reporter_subscription) { create(:event_subscription_favored_decision, user: reporter) } let!(:offender_subscription) { create(:event_subscription_favored_decision, user: offender, receiver_role: 'offender') } - let(:decision) { create(:decision, :favor, moderator: admin, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision, :favored, moderator: admin, reason: 'This is spam for sure.', reports: [report]) } let(:event) { Event::FavoredDecision.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } @@ -535,7 +535,7 @@ let!(:moderator_subscription) { create(:event_subscription_appeal_created, user: moderator) } - let(:decision) { create(:decision, :favor, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision, :favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } let(:appeal) { create(:appeal, appellant: appellant, decision: decision, reason: 'I strongly disagree!') } let(:event) { Event::AppealCreated.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } @@ -579,7 +579,7 @@ let!(:moderator_subscription) { create(:event_subscription_appeal_created, user: moderator) } - let(:decision) { create(:decision, :favor, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision, :favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } let(:appeal) { create(:appeal, appellant: appellant, decision: decision, reason: 'I strongly disagree!') } let(:event) { Event::AppealCreated.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } diff --git a/src/api/spec/policies/appeal_policy_spec.rb b/src/api/spec/policies/appeal_policy_spec.rb index 03c30faf451..c425ac94f95 100644 --- a/src/api/spec/policies/appeal_policy_spec.rb +++ b/src/api/spec/policies/appeal_policy_spec.rb @@ -40,7 +40,7 @@ context 'when the decision cleared a report created by the reporter' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, kind: 'cleared', reports: [report]) } + let(:decision) { create(:decision, type: 'DecisionCleared', reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } permissions :create? do @@ -56,7 +56,7 @@ context 'when the decision is on reports for a now-deleted reportable' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, kind: 'favor', reports: [report]) } + let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } before do @@ -76,7 +76,7 @@ context 'when the decision favored a report created by the reporter' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, kind: 'favor', reports: [report]) } + let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } permissions :create? do @@ -91,7 +91,7 @@ context 'when the decision cleared a report for something the appellant did' do let(:report) { create(:report, reportable: create(:comment_package, user: appellant)) } - let(:decision) { create(:decision, kind: 'cleared', reports: [report]) } + let(:decision) { create(:decision, type: 'DecisionCleared', reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: appellant) } permissions :create? do @@ -106,7 +106,7 @@ context 'when the decision favored a report for something the appellant did' do let(:report) { create(:report, reportable: create(:comment_package, user: appellant)) } - let(:decision) { create(:decision, kind: 'favor', reports: [report]) } + let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: appellant) } permissions :create? do From cead984a8badedbf620dbcb9e932d2aa9bad72ca Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Thu, 18 Apr 2024 12:47:44 +0200 Subject: [PATCH 3/8] Improve the way we display the decision types --- .../reports_modal_component.html.haml | 2 +- src/api/app/models/decision.rb | 19 ++++++++++++++++++- src/api/app/models/decision_cleared.rb | 4 ++++ src/api/app/models/decision_favored.rb | 4 ++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/api/app/components/reports_modal_component.html.haml b/src/api/app/components/reports_modal_component.html.haml index 363d1bc847a..1014f3e7439 100644 --- a/src/api/app/components/reports_modal_component.html.haml +++ b/src/api/app/components/reports_modal_component.html.haml @@ -21,7 +21,7 @@ -# Decision-related canned responses exclusively: = render CannedResponsesDropdownComponent.new(canned_responses) = form.text_area(:reason, class: 'form-control mb-3', required: true, placeholder: 'Reason for the decision') - = form.select(:type, Decision::TYPES, {}, class: 'form-select') + = form.select(:type, Decision.types(reportable), {}, class: 'form-select') .modal-footer - reports.each do |report| = form.hidden_field(:report_ids, multiple: true, value: report.id) diff --git a/src/api/app/models/decision.rb b/src/api/app/models/decision.rb index c210e65f86e..139380590b2 100644 --- a/src/api/app/models/decision.rb +++ b/src/api/app/models/decision.rb @@ -1,5 +1,5 @@ class Decision < ApplicationRecord - TYPES = { favored: 'DecisionFavored', cleared: 'DecisionCleared' }.freeze + TYPES = [DecisionFavored, DecisionCleared].freeze validates :reason, presence: true, length: { maximum: 65_535 } validates :type, presence: true, length: { maximum: 255 } @@ -21,6 +21,23 @@ def description 'The moderator decided on the report' end + # List of all viable types for a reportable, used in the decision creation form + def self.types(reportable) + TYPES.filter_map do |decision_type| + [decision_type.display_name, decision_type.name] if decision_type.display?(reportable) + end.compact.to_h + end + + # We use this to determine if the decision type should be displayed for reportable + def self.display?(_reportable) + true + end + + # We display this in the decision creation form + def self.display_name + 'unknown' + end + private # TODO: Replace this with `AbstractMethodCalled` after type is deployed diff --git a/src/api/app/models/decision_cleared.rb b/src/api/app/models/decision_cleared.rb index f366eb7af6f..d5d171c29a0 100644 --- a/src/api/app/models/decision_cleared.rb +++ b/src/api/app/models/decision_cleared.rb @@ -5,6 +5,10 @@ def description 'The moderator decided to clear the report' end + def self.display_name + 'cleared' + end + private def create_event diff --git a/src/api/app/models/decision_favored.rb b/src/api/app/models/decision_favored.rb index cf4f7b5025a..cad543097af 100644 --- a/src/api/app/models/decision_favored.rb +++ b/src/api/app/models/decision_favored.rb @@ -5,6 +5,10 @@ def description 'The moderator decided to favor the report' end + def self.display_name + 'favored' + end + private def create_event From 6d33d979d7dc53883db2761f485708348ae5aafb Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Thu, 18 Apr 2024 14:39:31 +0200 Subject: [PATCH 4/8] Use type in the rake task for generating decisions --- src/api/lib/tasks/dev/reports.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/lib/tasks/dev/reports.rake b/src/api/lib/tasks/dev/reports.rake index 82d58f57460..f44ebb90f0b 100755 --- a/src/api/lib/tasks/dev/reports.rake +++ b/src/api/lib/tasks/dev/reports.rake @@ -55,7 +55,7 @@ namespace :dev do Report.find_each do |report| # Reports with even id will be 'cleared' (0). Those with odd id will be 'favor' (1). - Decision.create!(reason: "Just because! #{report.id}", moderator: admin, kind: (report.id % 2), reports: [report]) + Decision.create!(reason: "Just because! #{report.id}", moderator: admin, type: Decision::TYPES[(report.id % 2)].name, reports: [report]) end # The same decision applies to more than one report about the same object/reportable. From a8be2041a50da24186e078a751b8006173cd1f85 Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Thu, 18 Apr 2024 15:53:47 +0200 Subject: [PATCH 5/8] Make sure canned responses work with the new decision types --- .../assets/javascripts/webui/canned_responses.js | 13 +++++++++---- .../canned_responses_dropdown_component.html.haml | 5 +++-- .../webui/users/canned_responses/edit.html.haml | 3 ++- .../webui/users/canned_responses/index.html.haml | 5 +++-- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/api/app/assets/javascripts/webui/canned_responses.js b/src/api/app/assets/javascripts/webui/canned_responses.js index 46379f64f17..81c252a434b 100644 --- a/src/api/app/assets/javascripts/webui/canned_responses.js +++ b/src/api/app/assets/javascripts/webui/canned_responses.js @@ -1,10 +1,15 @@ function setupCannedResponses() { // jshint ignore:line $('[data-canned-controller]').on('click', '[data-canned-response]', function(e) { $(e.target).closest('[data-canned-controller]').find('textarea').val(e.target.dataset.cannedResponse); - // TODO: adapt the following line when Decision#kind is replaced with Decision#type - // Set `cleared` by default for canned responses with decision_kind nil - let kind = e.target.dataset.decisionKind === 'favored' ? 'favor' : 'cleared'; - $(e.target).closest('[data-canned-controller]').find('#decision_kind').val(kind); + // let's make sure this exists first, to avoid errors + if ($(e.target).closest('[data-canned-controller]').find('#decision_type').length !== 0) { + // we gather up all the possible options, to make sure whatever we click doesn't wipe out the selection box + var optionsHtml = $(e.target).closest('[data-canned-controller]').find('#decision_type')[0].options; + var options = Array.from(optionsHtml).map(el => el.value); + // and if whatever we are trying to set exists within the options, we set it, otherwise we don't change the select box + if (options.includes(e.target.dataset.decisionType)) + $(e.target).closest('[data-canned-controller]').find('#decision_type').val(e.target.dataset.decisionType); + } // we have to enable the submit button for the comments form $(e.target).closest('[class*="-comment-form"]').find('input[type="submit"]').prop('disabled', false); }); diff --git a/src/api/app/components/canned_responses_dropdown_component.html.haml b/src/api/app/components/canned_responses_dropdown_component.html.haml index e0ce3f09183..e87b422bebf 100644 --- a/src/api/app/components/canned_responses_dropdown_component.html.haml +++ b/src/api/app/components/canned_responses_dropdown_component.html.haml @@ -4,9 +4,10 @@ Canned Responses %ul.dropdown-menu - @canned_responses_by_kind.each do |decision_kind, canned_responses| - .dropdown-header= decision_kind&.capitalize || 'Generic' + .dropdown-header= decision_kind&.humanize || 'Generic' - canned_responses.each do |canned_response| - %li.dropdown-item{ data: { 'canned-response': canned_response.content, 'decision_kind': decision_kind }, role: 'button' } + - decision_type = "Decision#{decision_kind.camelize}" if decision_kind.present? + %li.dropdown-item{ data: { 'canned-response': canned_response.content, 'decision-type': decision_type }, role: 'button' } = canned_response.title %hr.dropdown-divider/ = link_to('Create and modify' , canned_responses_path, class: 'dropdown-item') diff --git a/src/api/app/views/webui/users/canned_responses/edit.html.haml b/src/api/app/views/webui/users/canned_responses/edit.html.haml index 0295c6d7175..912866ae669 100644 --- a/src/api/app/views/webui/users/canned_responses/edit.html.haml +++ b/src/api/app/views/webui/users/canned_responses/edit.html.haml @@ -13,5 +13,6 @@ - if policy(Decision.new).create? .mb-3 = f.label :decision_kind, "Choose 'cleared' or 'favored' if this is a decision's reason" - = f.select(:decision_kind, CannedResponse.decision_kinds.keys, { include_blank: 'none' }, class: 'form-select') + - kinds = CannedResponse.decision_kinds.keys.to_h { |k| [k.humanize, k] } + = f.select(:decision_kind, kinds, { include_blank: 'none' }, class: 'form-select') = f.submit 'Save', class: 'btn btn-primary' diff --git a/src/api/app/views/webui/users/canned_responses/index.html.haml b/src/api/app/views/webui/users/canned_responses/index.html.haml index 8919c798b40..820693c74be 100644 --- a/src/api/app/views/webui/users/canned_responses/index.html.haml +++ b/src/api/app/views/webui/users/canned_responses/index.html.haml @@ -13,7 +13,8 @@ - if policy(Decision.new).create? .mb-3 = f.label :decision_kind, "Choose 'cleared' or 'favored' if this is a decision's reason" - = f.select(:decision_kind, CannedResponse.decision_kinds.keys, { include_blank: 'none', selected: nil }, class: 'form-select') + - kinds = CannedResponse.decision_kinds.keys.to_h { |k| [k.humanize, k] } + = f.select(:decision_kind, kinds, { include_blank: 'none', selected: nil }, class: 'form-select') = f.submit 'Create', class: 'btn btn-primary' .card.mt-2 @@ -31,7 +32,7 @@ .accordion-collapse.collapse{ id: "canned-response-#{canned_response.id}-collapse" } .accordion-body .d-flex.justify-content-between - %strong= canned_response.decision_kind.try(:capitalize) || 'Generic' + %strong= canned_response.decision_kind.try(:humanize) || 'Generic' .d-flex = link_to(edit_canned_response_path(canned_response), class: 'btn py-0 px-1 btn-link') do %i.fas.fa-edit From c67a27b186064b4cd7e082187fd9d8cfa95a339a Mon Sep 17 00:00:00 2001 From: Dani Donisa Date: Thu, 18 Apr 2024 17:07:40 +0200 Subject: [PATCH 6/8] Make specs work with Decision STI --- src/api/app/models/decision.rb | 2 +- src/api/spec/factories/appeal.rb | 2 +- .../factories/{decisions.rb => decision_cleared.rb} | 10 +--------- src/api/spec/factories/decision_favored.rb | 10 ++++++++++ src/api/spec/factories/notification.rb | 4 ++-- src/api/spec/features/webui/canned_responses_spec.rb | 2 +- src/api/spec/mailers/event_mailer_spec.rb | 8 ++++---- src/api/spec/models/report_spec.rb | 4 ++-- src/api/spec/policies/appeal_policy_spec.rb | 10 +++++----- 9 files changed, 27 insertions(+), 25 deletions(-) rename src/api/spec/factories/{decisions.rb => decision_cleared.rb} (61%) create mode 100644 src/api/spec/factories/decision_favored.rb diff --git a/src/api/app/models/decision.rb b/src/api/app/models/decision.rb index 139380590b2..c27f93a383f 100644 --- a/src/api/app/models/decision.rb +++ b/src/api/app/models/decision.rb @@ -1,5 +1,5 @@ class Decision < ApplicationRecord - TYPES = [DecisionFavored, DecisionCleared].freeze + TYPES = [::DecisionFavored, ::DecisionCleared].freeze validates :reason, presence: true, length: { maximum: 65_535 } validates :type, presence: true, length: { maximum: 255 } diff --git a/src/api/spec/factories/appeal.rb b/src/api/spec/factories/appeal.rb index bb6c045d047..61dbd782334 100644 --- a/src/api/spec/factories/appeal.rb +++ b/src/api/spec/factories/appeal.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :appeal do - decision + decision { association :decision_cleared } appellant { association :confirmed_user } reason { 'Some random reason' } end diff --git a/src/api/spec/factories/decisions.rb b/src/api/spec/factories/decision_cleared.rb similarity index 61% rename from src/api/spec/factories/decisions.rb rename to src/api/spec/factories/decision_cleared.rb index 518af546282..decca72e2ca 100644 --- a/src/api/spec/factories/decisions.rb +++ b/src/api/spec/factories/decision_cleared.rb @@ -1,18 +1,10 @@ FactoryBot.define do - factory :decision do + factory :decision_cleared do moderator factory: [:user] reason { Faker::Markdown.emphasis } after(:build) do |decision| decision.reports << create(:report, reason: 'This is spam!') if decision.reports.empty? end - - trait :cleared do - type { 'DecisionCleared' } - end - - trait :favored do - type { 'DecisionFavored' } - end end end diff --git a/src/api/spec/factories/decision_favored.rb b/src/api/spec/factories/decision_favored.rb new file mode 100644 index 00000000000..37057db116f --- /dev/null +++ b/src/api/spec/factories/decision_favored.rb @@ -0,0 +1,10 @@ +FactoryBot.define do + factory :decision_favored do + moderator factory: [:user] + reason { Faker::Markdown.emphasis } + + after(:build) do |decision| + decision.reports << create(:report, reason: 'This is spam!') if decision.reports.empty? + end + end +end diff --git a/src/api/spec/factories/notification.rb b/src/api/spec/factories/notification.rb index 77d9ac6d7ba..ee9f012a741 100644 --- a/src/api/spec/factories/notification.rb +++ b/src/api/spec/factories/notification.rb @@ -94,7 +94,7 @@ trait :cleared_decision do event_type { 'Event::ClearedDecision' } - notifiable { association(:decision, :cleared) } + notifiable { association(:decision_cleared) } after(:build) do |notification| notification.event_payload['reportable_type'] ||= notification.notifiable.reports.first.reportable.class.to_s @@ -103,7 +103,7 @@ trait :favored_decision do event_type { 'Event::FavoredDecision' } - notifiable { association(:decision, :favored) } + notifiable { association(:decision_favored) } after(:build) do |notification| notification.event_payload['reportable_type'] ||= notification.notifiable.reports.first.reportable.class.to_s diff --git a/src/api/spec/features/webui/canned_responses_spec.rb b/src/api/spec/features/webui/canned_responses_spec.rb index 6a781c78fe6..31969fe6849 100644 --- a/src/api/spec/features/webui/canned_responses_spec.rb +++ b/src/api/spec/features/webui/canned_responses_spec.rb @@ -58,7 +58,7 @@ visit canned_responses_path fill_in(name: 'canned_response[title]', with: 'wow') fill_in(name: 'canned_response[content]', with: 'a decision-related canned response') - find(:id, 'canned_response_decision_kind').select('favored') + find(:id, 'canned_response_decision_kind').select('Favored') click_button('Create') find('.accordion-button').click end diff --git a/src/api/spec/mailers/event_mailer_spec.rb b/src/api/spec/mailers/event_mailer_spec.rb index 34ec31e82a5..81cfb3d1b3b 100644 --- a/src/api/spec/mailers/event_mailer_spec.rb +++ b/src/api/spec/mailers/event_mailer_spec.rb @@ -449,7 +449,7 @@ let(:report) { create(:report, user: reporter) } let(:package) { report.reportable.commentable } let!(:subscription) { create(:event_subscription_cleared_decision, user: reporter) } - let(:decision) { create(:decision, :cleared, moderator: admin, reason: 'This is NOT spam.', reports: [report]) } + let(:decision) { create(:decision_cleared, moderator: admin, reason: 'This is NOT spam.', reports: [report]) } let(:event) { Event::ClearedDecision.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } @@ -492,7 +492,7 @@ let!(:reporter_subscription) { create(:event_subscription_favored_decision, user: reporter) } let!(:offender_subscription) { create(:event_subscription_favored_decision, user: offender, receiver_role: 'offender') } - let(:decision) { create(:decision, :favored, moderator: admin, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision_favored, moderator: admin, reason: 'This is spam for sure.', reports: [report]) } let(:event) { Event::FavoredDecision.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } @@ -535,7 +535,7 @@ let!(:moderator_subscription) { create(:event_subscription_appeal_created, user: moderator) } - let(:decision) { create(:decision, :favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision_favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } let(:appeal) { create(:appeal, appellant: appellant, decision: decision, reason: 'I strongly disagree!') } let(:event) { Event::AppealCreated.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } @@ -579,7 +579,7 @@ let!(:moderator_subscription) { create(:event_subscription_appeal_created, user: moderator) } - let(:decision) { create(:decision, :favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } + let(:decision) { create(:decision_favored, moderator: moderator, reason: 'This is spam for sure.', reports: [report]) } let(:appeal) { create(:appeal, appellant: appellant, decision: decision, reason: 'I strongly disagree!') } let(:event) { Event::AppealCreated.last } let(:mail) { EventMailer.with(subscribers: event.subscribers, event: event).notification_email.deliver_now } diff --git a/src/api/spec/models/report_spec.rb b/src/api/spec/models/report_spec.rb index ab9a44026dd..ff4daa245b1 100644 --- a/src/api/spec/models/report_spec.rb +++ b/src/api/spec/models/report_spec.rb @@ -1,7 +1,7 @@ RSpec.describe Report do describe '#reports_pointing_to_same_reportable' do context 'when different reportables' do - let(:decision) { create(:decision) } + let(:decision) { create(:decision_cleared) } let(:report) { build(:report, decision: decision) } before do @@ -14,7 +14,7 @@ context 'when reports are pointing to same reportable' do let(:report) { create(:report) } - let(:decision) { create(:decision, reports: [report]) } + let(:decision) { create(:decision_cleared, reports: [report]) } let(:new_report) { build(:report, reportable: report.reportable, decision: decision) } it { expect(new_report.valid?).to be(true) } diff --git a/src/api/spec/policies/appeal_policy_spec.rb b/src/api/spec/policies/appeal_policy_spec.rb index c425ac94f95..4c320ad39ad 100644 --- a/src/api/spec/policies/appeal_policy_spec.rb +++ b/src/api/spec/policies/appeal_policy_spec.rb @@ -40,7 +40,7 @@ context 'when the decision cleared a report created by the reporter' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, type: 'DecisionCleared', reports: [report]) } + let(:decision) { create(:decision_cleared, reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } permissions :create? do @@ -56,7 +56,7 @@ context 'when the decision is on reports for a now-deleted reportable' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } + let(:decision) { create(:decision_favored, reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } before do @@ -76,7 +76,7 @@ context 'when the decision favored a report created by the reporter' do let(:report) { create(:report) } let(:reporter) { report.user } - let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } + let(:decision) { create(:decision_favored, reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: reporter) } permissions :create? do @@ -91,7 +91,7 @@ context 'when the decision cleared a report for something the appellant did' do let(:report) { create(:report, reportable: create(:comment_package, user: appellant)) } - let(:decision) { create(:decision, type: 'DecisionCleared', reports: [report]) } + let(:decision) { create(:decision_cleared, reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: appellant) } permissions :create? do @@ -106,7 +106,7 @@ context 'when the decision favored a report for something the appellant did' do let(:report) { create(:report, reportable: create(:comment_package, user: appellant)) } - let(:decision) { create(:decision, type: 'DecisionFavored', reports: [report]) } + let(:decision) { create(:decision_favored, reports: [report]) } let(:appeal) { create(:appeal, decision: decision, appellant: appellant) } permissions :create? do From 05103149335f32ec88b5de9e3f5741310e1b196c Mon Sep 17 00:00:00 2001 From: Jacob Michalskie Date: Fri, 19 Apr 2024 14:49:40 +0200 Subject: [PATCH 7/8] Make sure that the kind matches the type --- src/api/app/controllers/webui/decisions_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/api/app/controllers/webui/decisions_controller.rb b/src/api/app/controllers/webui/decisions_controller.rb index e5774621f15..a4f0e195f6e 100644 --- a/src/api/app/controllers/webui/decisions_controller.rb +++ b/src/api/app/controllers/webui/decisions_controller.rb @@ -19,6 +19,8 @@ def create private def decision_params - params.require(:decision).permit(:reason, :type, report_ids: []) + # TODO: remove merge and replace decision_kind with decision_type + kind = params[:decision][:type] == 'DecisionFavored' ? 'favor' : 'cleared' + params.require(:decision).permit(:reason, :type, report_ids: []).merge(kind: kind) end end From aa898e2ad43c0186a6a44725714b7efe913d5398 Mon Sep 17 00:00:00 2001 From: Dani Donisa Date: Fri, 19 Apr 2024 15:44:58 +0200 Subject: [PATCH 8/8] Use strings instead of constans to get rid of circular dependencies --- src/api/app/models/decision.rb | 5 +++-- src/api/lib/tasks/dev/reports.rake | 2 +- .../notification_action_description_component_spec.rb | 2 -- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/api/app/models/decision.rb b/src/api/app/models/decision.rb index c27f93a383f..fd0ef1c19e6 100644 --- a/src/api/app/models/decision.rb +++ b/src/api/app/models/decision.rb @@ -1,5 +1,5 @@ class Decision < ApplicationRecord - TYPES = [::DecisionFavored, ::DecisionCleared].freeze + TYPES = %w[DecisionFavored DecisionCleared].freeze validates :reason, presence: true, length: { maximum: 65_535 } validates :type, presence: true, length: { maximum: 255 } @@ -23,7 +23,8 @@ def description # List of all viable types for a reportable, used in the decision creation form def self.types(reportable) - TYPES.filter_map do |decision_type| + TYPES.filter_map do |decision_type_name| + decision_type = decision_type_name.constantize [decision_type.display_name, decision_type.name] if decision_type.display?(reportable) end.compact.to_h end diff --git a/src/api/lib/tasks/dev/reports.rake b/src/api/lib/tasks/dev/reports.rake index f44ebb90f0b..049934731ae 100755 --- a/src/api/lib/tasks/dev/reports.rake +++ b/src/api/lib/tasks/dev/reports.rake @@ -55,7 +55,7 @@ namespace :dev do Report.find_each do |report| # Reports with even id will be 'cleared' (0). Those with odd id will be 'favor' (1). - Decision.create!(reason: "Just because! #{report.id}", moderator: admin, type: Decision::TYPES[(report.id % 2)].name, reports: [report]) + Decision.create!(reason: "Just because! #{report.id}", moderator: admin, type: Decision::TYPES[(report.id % 2)], reports: [report]) end # The same decision applies to more than one report about the same object/reportable. diff --git a/src/api/spec/components/notification_action_description_component_spec.rb b/src/api/spec/components/notification_action_description_component_spec.rb index 957e808c78b..bed58d66f61 100644 --- a/src/api/spec/components/notification_action_description_component_spec.rb +++ b/src/api/spec/components/notification_action_description_component_spec.rb @@ -1,5 +1,3 @@ -require 'decision' # To break the circular dependency between Decision and DecisionFavored and DecisionCleared - RSpec.describe NotificationActionDescriptionComponent, type: :component do context 'when the notification is for a Event::RequestStatechange event with a request having only a target' do let(:target_project) { create(:project, name: 'project_123') }