From 110506eb610c15f9e3dc4ef0211dc2f5287e20ac Mon Sep 17 00:00:00 2001 From: Mohamed El-Sawy Date: Thu, 15 Aug 2024 21:32:17 +0300 Subject: [PATCH] CV2-5051: Item history updates (#1989) * CV2-5051: log version for ExplainerItem * CV2-5051: log FactCheck update and add meta for ClaimDescription logs * CV2-5051: migrate FactCheck logs to a new item * CV2-5051: fix tests * CV2-5051: exclude logs related to add/remove fact-checks whem migrate fact-check logs --- app/models/claim_description.rb | 24 +++++++++++++++++++++++ app/models/concerns/article.rb | 2 -- app/models/explainer.rb | 2 +- app/models/explainer_item.rb | 6 ++++++ app/models/fact_check.rb | 2 ++ app/models/version.rb | 11 +++++++++-- test/models/claim_description_test.rb | 28 +++++++++++++++++++++++++-- test/models/explainer_item_test.rb | 20 +++++++++++++++++++ test/models/explainer_test.rb | 15 -------------- 9 files changed, 88 insertions(+), 22 deletions(-) diff --git a/app/models/claim_description.rb b/app/models/claim_description.rb index 048f72a4d5..24bed73609 100644 --- a/app/models/claim_description.rb +++ b/app/models/claim_description.rb @@ -1,6 +1,8 @@ class ClaimDescription < ApplicationRecord include Article + has_paper_trail on: [:create, :update], ignore: [:updated_at, :created_at], if: proc { |_x| User.current.present? }, versions: { class_name: 'Version' } + before_validation :set_team, on: :create belongs_to :project_media, optional: true belongs_to :team @@ -13,6 +15,7 @@ class ClaimDescription < ApplicationRecord after_commit :update_fact_check, on: [:update] after_update :update_report_status after_update :replace_media + after_update :migrate_claim_and_fact_check_logs, if: proc { |cd| cd.saved_change_to_project_media_id? && !cd.project_media_id.nil? } # To avoid GraphQL conflict with name `context` alias_attribute :claim_context, :context @@ -42,6 +45,10 @@ def project_media_was ProjectMedia.find_by_id(self.project_media_id_before_last_save) end + def version_metadata(_changes) + { fact_check: self.fact_check&.title }.to_json + end + private def set_team @@ -89,4 +96,21 @@ def replace_media old_pm.replace_by(new_pm) end end + + def migrate_claim_and_fact_check_logs + # Migrate ClaimDescription logs + cd_versions = Version.from_partition(self.team_id).where(item_type: 'ClaimDescription', item_id: self.id) + # Exclude the one related to add/remove based on object_changes field. + cd_versions = cd_versions.reject do |v| + oc = begin JSON.parse(v.object_changes) rescue {} end + oc.length == 1 && oc.keys.include?('project_media_id') + end + Version.from_partition(self.team_id).where(id: cd_versions.map(&:id)).update_all(associated_id: self.project_media_id) + fc_id = self.fact_check&.id + unless fc_id.nil? + # Migrate FactCheck logs and exclude create event + Version.from_partition(self.team_id).where(item_type: 'FactCheck', item_id: fc_id) + .where.not(event: 'create').update_all(associated_id: self.project_media_id) + end + end end diff --git a/app/models/concerns/article.rb b/app/models/concerns/article.rb index 614fa2a660..e4b1ed48be 100644 --- a/app/models/concerns/article.rb +++ b/app/models/concerns/article.rb @@ -6,8 +6,6 @@ module Article included do include CheckElasticSearch - has_paper_trail on: [:create, :update], ignore: [:updated_at, :created_at], if: proc { |_x| User.current.present? }, versions: { class_name: 'Version' } - belongs_to :user before_validation :set_user diff --git a/app/models/explainer.rb b/app/models/explainer.rb index e58e653e3d..a4319e718a 100644 --- a/app/models/explainer.rb +++ b/app/models/explainer.rb @@ -10,7 +10,7 @@ class Explainer < ApplicationRecord belongs_to :team has_annotations - has_many :explainer_items + has_many :explainer_items, dependent: :destroy has_many :project_medias, through: :explainer_items before_validation :set_team diff --git a/app/models/explainer_item.rb b/app/models/explainer_item.rb index 114ecb3347..7fc5be1f49 100644 --- a/app/models/explainer_item.rb +++ b/app/models/explainer_item.rb @@ -1,11 +1,17 @@ # Join model class ExplainerItem < ApplicationRecord + has_paper_trail on: [:create, :destroy], ignore: [:updated_at, :created_at], if: proc { |_x| User.current.present? }, versions: { class_name: 'Version' } + belongs_to :explainer belongs_to :project_media validates_presence_of :explainer, :project_media validate :same_team + def version_metadata(_changes) + { explainer_title: self.explainer.title }.to_json + end + private def same_team diff --git a/app/models/fact_check.rb b/app/models/fact_check.rb index ac0138e853..5d42782496 100644 --- a/app/models/fact_check.rb +++ b/app/models/fact_check.rb @@ -1,6 +1,8 @@ class FactCheck < ApplicationRecord include Article + has_paper_trail on: [:create, :update], ignore: [:updated_at, :created_at, :rating, :report_status], if: proc { |_x| User.current.present? }, versions: { class_name: 'Version' } + enum report_status: { unpublished: 0, published: 1, paused: 2 } attr_accessor :skip_report_update, :publish_report diff --git a/app/models/version.rb b/app/models/version.rb index cc53ab1f20..7c13f00601 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -145,9 +145,16 @@ def get_associated when 'create_assignment', 'destroy_assignment' self.get_associated_from_assignment when 'create_claimdescription', 'update_claimdescription' - ['ProjectMedia', self.item.project_media_id] - when 'create_factcheck' + pm_id = self.item.project_media_id + if pm_id.nil? + changes = self.get_object_changes + pm_id = changes['project_media_id'][0] if changes.has_key?('project_media_id') && changes['project_media_id'][1].nil? + end + ['ProjectMedia', pm_id] + when 'create_factcheck', 'update_factcheck' ['ProjectMedia', self.item.claim_description.project_media_id] + when 'create_explaineritem', 'destroy_explaineritem' + ['ProjectMedia', self.item.project_media_id] end end diff --git a/test/models/claim_description_test.rb b/test/models/claim_description_test.rb index 16048ddafc..2ff78762df 100644 --- a/test/models/claim_description_test.rb +++ b/test/models/claim_description_test.rb @@ -18,12 +18,36 @@ def setup t = create_team create_team_user team: t, user: u, role: 'admin' pm = create_project_media team: t + pm2 = create_project_media team: t with_current_user_and_team(u, t) do cd = nil - assert_difference 'PaperTrail::Version.count', 1 do + fc = nil + assert_difference 'PaperTrail::Version.count', 2 do cd = create_claim_description project_media: pm, user: u + fc = create_fact_check claim_description: cd end - assert_equal 1, cd.versions.count + cd.description = 'update description' + cd.save! + fc.title = 'update title' + fc.save! + # Remove FactCheck + cd.project_media_id = nil + cd.save! + assert_equal 3, cd.versions.count + assert_equal 2, fc.versions.count + v_count = Version.from_partition(t.id).where(associated_type: 'ProjectMedia', associated_id: pm.id, item_type: ['ClaimDescription', 'FactCheck']).count + assert_equal 5, v_count + # Add existing FactCheck to another media + cd.project_media_id = pm2.id + cd.save! + assert_equal 4, cd.versions.count + assert_equal 2, fc.versions.count + # Old item logs + v_count = Version.from_partition(t.id).where(associated_type: 'ProjectMedia', associated_id: pm.id, item_type: ['ClaimDescription', 'FactCheck']).count + assert_equal 2, v_count + # New item logs + v_count = Version.from_partition(t.id).where(associated_type: 'ProjectMedia', associated_id: pm2.id, item_type: ['ClaimDescription', 'FactCheck']).count + assert_equal 4, v_count end end end diff --git a/test/models/explainer_item_test.rb b/test/models/explainer_item_test.rb index 477a2b730a..39fff5a3c9 100644 --- a/test/models/explainer_item_test.rb +++ b/test/models/explainer_item_test.rb @@ -54,6 +54,26 @@ def teardown assert_not ExplainerItem.new(project_media: pm2, explainer: e1).valid? end + test "should have versions" do + with_versioning do + u = create_user + t = create_team + create_team_user team: t, user: u, role: 'admin' + e = create_explainer team: t + pm = create_project_media team: t + with_current_user_and_team(u, t) do + assert_difference 'PaperTrail::Version.count', 1 do + pm.explainers << e + end + ei = ExplainerItem.where(project_media_id: pm.id, explainer_id: e.id).last + assert_equal 1, ei.versions.count + assert_difference 'PaperTrail::Version.count', 1 do + ei.destroy + end + end + end + end + test "should have permission to create explainer item" do t1 = create_team u1 = create_user diff --git a/test/models/explainer_test.rb b/test/models/explainer_test.rb index cab34a1484..cce9645166 100644 --- a/test/models/explainer_test.rb +++ b/test/models/explainer_test.rb @@ -11,21 +11,6 @@ def setup end end - test "should have versions" do - with_versioning do - u = create_user - t = create_team - create_team_user team: t, user: u, role: 'admin' - with_current_user_and_team(u, t) do - ex = nil - assert_difference 'PaperTrail::Version.count', 1 do - ex = create_explainer user: u, team: t - end - assert_equal 1, ex.versions.count - end - end - end - test "should not create explainer without user or team" do assert_no_difference 'Explainer.count' do assert_raises ActiveRecord::RecordInvalid do