Fix for bug in Plan duplication results in original plan identifier b… #3441
Conversation
johnpinto1
force-pushed
the
plan-deep-copy-bug-copies-old-id-as-identifier
branch
from
de27ebf
to
574f3bc
Compare
…eing copied.
Change in class method Plan.deep_copy:
- Firstly, on duplicating the Plan we set the plan identifier to nil and save.
- Then we fill the identifier variable with the Plan id that was
regenerated when the duplicate copy was save in the previous.
- We then persist this change by saving the Plan again.
johnpinto1
force-pushed
the
plan-deep-copy-bug-copies-old-id-as-identifier
branch
from
574f3bc
to
36e5b7d
Compare
| plan_copy.save! | ||
| # Copy newly generated Id to the identifier | ||
| plan_copy.identifier = plan_copy.id | ||
| plan.save! |
There was a problem hiding this comment.
Looks good. I tested the PR and it all appears to be behaving as it should. :) I just have a couple of thoughts as well.
The last two lines are having the final say in the saved value of plan_copy.identifier. So assigning plan_copy.identifier = nil prior to the first save doesn't seem necessary.
Also, plan_copy.identifier is a String, but plan_copy.id is an integer. I guess we have similar code in https://github.com/DMPRoadmap/roadmap/blob/main/app/controllers/plans_controller.rb#L140 (@plan.identifier = @plan.id.to_s). However, I guess Rails is automatically typecasting the value to a String here.
There was a problem hiding this comment.
Thanks @aaronskiba. I have made the changes suggested in a separate commit.
…eing copied. Changes (suggested by @aaronskiba): - removed an unnecessary line plan_copy.identifier = nil - cast to string the integer-valued plan id plan_copy.identifier = plan_copy.id.to_s
…eing copied.
Change in class method Plan.deep_copy: