DMPRoadmap / roadmap

DCC/UC3 collaboration for a data management planning tool
MIT License
106 stars 109 forks source link

Possible redundant deep copy in app/controllers/concerns/versionable.rb #1993

Open Yu-Chieh-Henry-Yang opened 5 years ago

Yu-Chieh-Henry-Yang commented 5 years ago

Inside app/controllers/concerns/versionable.rb new_template = Template.find_or_generate_version!(template) already deep copy the whole template, so we shouldn't need to do it again here

        if belongs == :template
          obj = obj.send(:deep_copy)
          obj.template = new_template
        else
          found = find_in_space(obj.send(belongs), new_template.phases)
          obj = obj.send(:deep_copy)
          obj.send("#{belongs}=", found)
        end

because we are just adding new elements like Phase, Section, Question, or Annotation. I think lines obj = obj.send(:deep_copy) can be removed, what do people think?

(@Bodacious I saw you editing these 2 lines so I thought I might tag you here.)

magdalenadrafiova commented 4 years ago

@briri @xsrust apologies one more of those where I am not sure whether this is still relevant (?)

briri commented 4 years ago

@xsrust was this the deep copy you just cleaned up during the Rails5 upgrade?

xsrust commented 4 years ago

This looks to be a different issue from that cleanup.

I had a quick-look and the initial concerns (double-template-generation) seem valid in the current code so it's worth addressing: https://github.com/DMPRoadmap/roadmap/blame/rails5/app/controllers/concerns/versionable.rb#L67-L73