Refactor Plan.deep_copy(plan)

[aaronskiba]

Changes proposed in this PR:

• Remove redundant plan.save! (line 260) 4b25e4a

  • 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 .
    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 (see below):

  def duplicate
  plan = Plan.find(params[:id])
  authorize plan
  @plan = Plan.deep_copy(plan)
  respond_to do |format|
  if @plan.save
  ...

• Refactor / use ActiveRecord association operator d8f8978

  • Using << here replaces manual foreign key assignment as well as the explicit save call.

• Add test for plan identifier copying 521ba8e

[johnpinto1]

@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

[aaronskiba]

@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!