Refactor Plan.deep_copy(plan)
#3469
Conversation
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
...
Using << here replaces manual foreign key assignment as well as the explicit save call.
There was a problem hiding this comment.
@aaronskiba I have reviewed this and it works as suggested. Attached is spreadsheet of my checks.
I copied an existing plan by making a copy through the UI and a deep copy in the Rails console.
In both cases: the plan_id (was new as expected), the plan identifier matched the plan id and plan answers matched with plan_id changed as expected.
TestingPlanCopy.xlsx
Excellent! Thank you for the approval and the testing @johnpinto1! |
Changes proposed in this PR:
Remove redundant
plan.save!(line 260) 4b25e4aplan_copy.identifier = plan_copy.id.to_s, I believe this was meant to beplan_copy.save!. However, rather than correcting the variable, I think the line can be removed completely .The earlier execution of
plan_copy.save!(line 257) is necessary because we need to generate a pk forplan_copy. However, theduplicateaction withinPlansControllerappears to be the only thing callingPlan.deep_copy, and the action is performing its own save on the copied plan returned fromdeep_copy(see below):Refactor / use ActiveRecord association operator d8f8978
<<here replaces manual foreign key assignment as well as the explicit save call.Add test for plan identifier copying 521ba8e