From 4b25e4adc0f09fec80b2816797d12a7b683ca38e Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 11 Dec 2024 14:11:35 -0700 Subject: [PATCH 1/4] Refactor deep_copy: Remove redundant `plan.save!` Considering that the removed line occurred immediately after `plan_copy.identifier = plan_copy.id.to_s`, I believe this was meant to be `plan_copy.save!`. However, rather than correcting the variable, I think the line can be removed completely (explanation below). The earlier execution of `plan_copy.save!` (line 257) is necessary because we need to generate a pk for `plan_copy`. However, the `duplicate` action within PlansController appears to be the only thing calling Plan.deep_copy, and the action is performing its own save on the copied plan returned from deep_copy: def duplicate plan = Plan.find(params[:id]) authorize plan @plan = Plan.deep_copy(plan) respond_to do |format| if @plan.save ... --- app/models/plan.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/plan.rb b/app/models/plan.rb index f2fb6a2901..ea78f4e087 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -257,7 +257,6 @@ def self.deep_copy(plan) plan_copy.save! # Copy newly generated Id to the identifier plan_copy.identifier = plan_copy.id.to_s - plan.save! plan.answers.each do |answer| answer_copy = Answer.deep_copy(answer) answer_copy.plan_id = plan_copy.id From d8f897880a7fc3cc70b8aa84ef21d9ed7fdbde47 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 11 Dec 2024 13:22:54 -0700 Subject: [PATCH 2/4] Refactor / use ActiveRecord association operator Using << here replaces manual foreign key assignment as well as the explicit save call. --- app/models/plan.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/plan.rb b/app/models/plan.rb index ea78f4e087..83fac933e9 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -259,8 +259,7 @@ def self.deep_copy(plan) plan_copy.identifier = plan_copy.id.to_s plan.answers.each do |answer| answer_copy = Answer.deep_copy(answer) - answer_copy.plan_id = plan_copy.id - answer_copy.save! + plan_copy.answers << answer_copy end plan.guidance_groups.each do |guidance_group| plan_copy.guidance_groups << guidance_group if guidance_group.present? From 521ba8ef76b7ead4b769137358a8be1729872c52 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 11 Dec 2024 13:37:21 -0700 Subject: [PATCH 3/4] Add test for plan identifier copying --- spec/models/plan_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/plan_spec.rb b/spec/models/plan_spec.rb index 08bcca6f5b..bbddb5b76e 100644 --- a/spec/models/plan_spec.rb +++ b/spec/models/plan_spec.rb @@ -398,6 +398,10 @@ expect(subject.title).to include(plan.title) end + it "copies the new plan's id to its identifer" do + expect(subject.identifier).to eql(subject.id.to_s) + end + it 'persists the record' do expect(subject).to be_persisted end From f0a3ecf3dcecc22ff363a2b34161ad2038e7fbe8 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 16 Dec 2024 08:06:16 -0700 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 101dd4c5af..0c1959f48f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +- Refactor Plan.deep_copy(plan) [#3469](https://github.com/DMPRoadmap/roadmap/pull/3469) - Fixed a bug in the deep copy of plans where the old identifier was being copied into the new plan. We now copy the generated id of the new plan to the identifier field. - Fixed bar chart click function in the Usage dashboard (GitHub issue #3443)