Project copying #2861

zwolf · 2018-07-30T21:14:20Z

Adds project copying via a POST /projects/:project_id/copy. Requires that a) the project is not live, b) the project's configuration object includes template: true, and c) the user has write privileges on the project.

Utilizes the deep_cloneable gem to keep the logic clean. This gem just provides a new #dup that takes a list of associations to call the same on. List of associations included (as well as excluded) associations is in the ProjectCopier lib.

Review checklist

First, the most important one: is this PR small enough that you can actually review it? Feel free to just reject a branch if the changes are hard to review due to the length of the diff.
If there are any migrations, will they the previous version of the app work correctly after they've been run (e.g. the don't remove columns still known about by ActiveRecord).
If anything changed with regards to the public API, are those changes also documented in the apiary.apib file?
Are all the changes covered by tests? Think about any possible edge cases that might be left untested.

houndci-bot · 2018-07-30T21:14:55Z

app/workers/project_copy_worker.rb

+  def perform(project_id, user_id)
+    ProjectCopyWorker.copy(project_id, user_id)
+  end
+end


Layout/TrailingBlankLines: Final newline missing.

These newlines exist locally and git thinks the files are synced. That is, I can't commit or push them.

Do you have a linter for your editor? Something like https://github.com/ngmy/vim-rubocop might help to run it locally as well.

houndci-bot · 2018-07-30T21:14:55Z

lib/project_copier.rb

+    copied_project.save!
+    copied_project
+  end
+end


Layout/TrailingBlankLines: Final newline missing.

houndci-bot · 2018-07-30T21:14:56Z

lib/project_copier.rb

+                          :avatar,
+                          :background,
+                          { active_workflows: [ :tutorials, :attached_images, :workflow_contents ] }
+                        ]


Layout/MultilineArrayBraceLayout: Closing array brace must be on the same line as the last array element when opening brace is on the same line as the first array element.

houndci-bot · 2018-07-30T21:14:56Z

lib/project_copier.rb

+                          :tagged_resources,
+                          :avatar,
+                          :background,
+                          { active_workflows: [ :tutorials, :attached_images, :workflow_contents ] }


Style/SymbolArray: Use %i or %I for an array of symbols.

houndci-bot · 2018-07-30T21:14:56Z

lib/project_copier.rb

@@ -0,0 +1,30 @@
+class ProjectCopier
+  EXCLUDE_ATTRIBUTES = [ :classifications_count, :launched_row_order, :beta_row_order ]
+  INCLUDE_ATTRIBUTES = [  :project_contents,


Style/MutableConstant: Freeze mutable objects assigned to constants.

houndci-bot · 2018-07-30T21:14:56Z

lib/project_copier.rb

@@ -0,0 +1,30 @@
+class ProjectCopier
+  EXCLUDE_ATTRIBUTES = [ :classifications_count, :launched_row_order, :beta_row_order ]


Style/MutableConstant: Freeze mutable objects assigned to constants.
Style/SymbolArray: Use %i or %I for an array of symbols.

camallen

Nice work this looks pretty good, i left some minor comments on your code.

My main interest is in the gem dependency code, i.e. How does the deep clone work under the hood?

A quick read of the source shows that it dups a record, excludes attributes, selects specific attributes and then builds up a chain of associations to link before assigning them to the dup'ed new record.
https://github.com/moiristo/deep_cloneable/blob/1d67dd44031b1df7ae77c8a33b4afd39ade7706f/lib/deep_cloneable.rb#L85

https://github.com/moiristo/deep_cloneable/blob/1d67dd44031b1df7ae77c8a33b4afd39ade7706f/lib/deep_cloneable.rb#L68
Also that looks interesting and the copy is always assumed to be valid. Does this validate: false persist to later saves? Could this cause issues?

Some AR associations auto save the new record (has_many, etc to link the FKs), does this mean that for a small period of time the owner of the template project now owns the new copy as well?

Could it leave the copied template project owned by the non-requesting user? Is this an issue or just something to be aware of (i.e. what happens if the copy fails after the association auto save?

camallen · 2018-08-02T09:20:26Z

app/policies/project_policy.rb

@@ -21,7 +21,7 @@ class TranslateScope < Scope
        :create_subjects_export,
        :create_workflows_export,
        :create_workflow_contents_export,
-        :retire_subjects, with: WriteScope
+        :retire_subjects, :copy, with: WriteScope


How will users copy template projects they don't collab / own? Should this be ReadScope i.e. public projects or ones i can see in my lab?

Depends. Write makes sense to me to roll out this feature on a small number of hand selected projects (via the manually setting the configuration) and the button in PFE doesn't exist yet. For those same reasons, though, I can see not mattering very much and maybe it's better to put it in Read now and not have to change it later.

camallen · 2018-08-02T09:22:20Z

app/workers/project_copy_worker.rb

+class ProjectCopyWorker
+  include Sidekiq::Worker
+
+  sidekiq_options queue: :data_high


Do we want to look at locking on unique args to ensure users don't trigger multiple copies by double clicking form submission?

camallen · 2018-08-02T09:23:20Z

app/workers/project_copy_worker.rb

+  def perform(project_id, user_id)
+    ProjectCopyWorker.copy(project_id, user_id)
+  end
+end


Do you have a linter for your editor? Something like https://github.com/ngmy/vim-rubocop might help to run it locally as well.

camallen · 2018-08-02T09:34:09Z

lib/project_copier.rb

+                        ].freeze
+
+  def self.copy(project_id, user_id)
+    project = Project.find(project_id)


what happens if i can't find the project / user by ID?

✔️ Good call, added a rescue to the worker.

camallen · 2018-08-02T09:35:29Z

lib/project_copier.rb

@@ -0,0 +1,30 @@
+class ProjectCopier
+  EXCLUDE_ATTRIBUTES = [ :classifications_count, :launched_row_order, :beta_row_order ].freeze
+  INCLUDE_ATTRIBUTES = [  :project_contents,


These aren't really attributes but associations. Should the variable name reflect that?

camallen · 2018-08-02T09:50:37Z

lib/project_copier.rb

+    user = User.find(user_id)
+
+    copied_project = project.deep_clone include: INCLUDE_ATTRIBUTES, except: EXCLUDE_ATTRIBUTES
+    copied_project.owner = user


did you look at the block syntax for updating the clone? https://github.com/moiristo/deep_cloneable#optional-block

I'm not doing anything with the copy that needs any pre-exclude original attributes or anything else I can think of that requires that block. It's all happening pre-save!, so it's not hitting the db yet.

camallen · 2018-08-02T09:53:39Z

spec/lib/project_copier_spec.rb

+
+      it "has matching attributes" do
+        expect(copied_project.display_name).to eq(project.display_name)
+      end


You might want to test the other changes to the copied instance as well, e.g. copied_project.update_attributes(launch_approved: false, live: false)

zwolf · 2018-08-02T16:06:30Z

Thanks, updated with suggested changes. Also, somehow we both managed to miss that I was calling the ProjectCopyWorker from itself instead of the ProjectCopier. There are specs for that, now, too. :3

Also that looks interesting and the copy is always assumed to be valid. Does this validate: false persist to later saves? Could this cause issues?

First, that would only happen using the validate: false option, which isn't being used, so all saves should be being validated. But also, there's only the one save before the Worker, the Copier instance, and everything else is garbage collected so I don't see how that option could persist.

Some AR associations auto save the new record (has_many, etc to link the FKs), does this mean that for a small period of time the owner of the template project now owns the new copy as well? Could it leave the copied template project owned by the non-requesting user? Is this an issue or just something to be aware of (i.e. what happens if the copy fails after the association auto save?

I didn't see that happening. Running a deep clone on a project, it's workflows are unsaved until you save the project. As a matter of fact, a copied_project.workflows is empty while copied_project.active_workflows contains the unsaved workflow. When you save! and reload the copied_project, the .workflows association exists and it can be pulled from the db.

houndci-bot · 2018-08-02T16:06:31Z

spec/workers/project_copy_worker_spec.rb

+      }.not_to raise_error
+    end
+  end
+end


Layout/TrailingBlankLines: Final newline missing.

houndci-bot · 2018-08-02T16:06:31Z

app/workers/subject_workflow_status_create_worker.rb

@@ -1,4 +1,4 @@
-class SubjectWorkflowStatusCreateWorker
+  class SubjectWorkflowStatusCreateWorker


Layout/InitialIndentation: Indentation of first line in file detected.

aw c'mon, The Hound, this wasn't even--aw, forget it. Fine, I'll fix it.

camallen

Nice - my original comments were misfounded and your interpretation is correct. This gem doesn't use the validate option unless specified and doesn't cause auto save on certain associations to fire (still not 100% sure why though).

I was thinking should we store the originating copy ID in the project configuration for data provenance? That way we can see which project it was copied from if we have to debug it.

A couple of minor comments on this PR, nice work 👍

camallen · 2018-08-07T11:57:55Z

lib/project_copier.rb

@@ -0,0 +1,29 @@
+class ProjectCopier
+  EXCLUDE_ASSOCIATIONS = [ :classifications_count, :launched_row_order, :beta_row_order ].freeze


These aren't associations, rather attributes.

doh! shouldn't have changed both.

camallen · 2018-08-07T11:58:31Z

lib/project_copier.rb

+      copied_project.display_name += " (copy)"
+    end
+
+    copied_project.update_attributes(launch_approved: false, live: false)


this will run a save after assigning the attributes, i think you want assign_attributes instead to avoid the double save.

absolutely, ✔️

camallen · 2018-08-07T12:00:16Z

spec/lib/project_copier_spec.rb

+  describe '#copy' do
+    let(:project) { create(:full_project, build_extra_contents: true)}
+    let(:copyist) { create(:user) }
+    let(:tags) { create(:tag, resource: project) }


none of these associated records exist as they are never called before the copy. Might want to add a let!(:tags) or call them before you use them. Also you should probably test these associations exist on the copied project, i wouldn't validate the copy just that the included associations are copied.

… to avoid double save

houndci-bot · 2018-08-07T16:56:02Z

lib/project_copier.rb

+                            :avatar,
+                            :background,
+                            :translations,
+                            { active_workflows: [ :tutorials, :attached_images, :workflow_contents] }].freeze


Style/SymbolArray: Use %i or %I for an array of symbols.

houndci-bot reviewed Jul 30, 2018

View reviewed changes

zwolf assigned marten and camallen Jul 31, 2018

camallen reviewed Aug 2, 2018

View reviewed changes

zwolf added 5 commits August 2, 2018 09:45

WIP kode klub kommit

1199f0c

Project copying: lib, worker, controller, specs

f8f7c11

Freeze constants.

97854a0

PR review changes

a375b3d

Removed unused double

d6a62fb

zwolf force-pushed the project-copy branch from 76da2a9 to d6a62fb Compare August 2, 2018 16:06

houndci-bot reviewed Aug 2, 2018

View reviewed changes

Houndnotes

f33d01b

camallen approved these changes Aug 7, 2018

View reviewed changes

zwolf added 2 commits August 7, 2018 11:21

Test copied association validity, assign instead of update attributes…

9fcc7f4

… to avoid double save

Include project translations

00967ea

houndci-bot reviewed Aug 7, 2018

View reviewed changes

Include source_project_id in copied project's configuration

57cf03d

zwolf merged commit 33ea1e4 into zooniverse:master Aug 20, 2018

zwolf deleted the project-copy branch August 22, 2018 15:03

mcbouslog mentioned this pull request Nov 5, 2018

Copy template project zooniverse/Panoptes-Front-End#4725

Closed

camallen mentioned this pull request Jan 16, 2020

[WIP] Add workflow import command zooniverse/panoptes-cli#134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project copying #2861

Project copying #2861

zwolf commented Jul 30, 2018

houndci-bot Jul 30, 2018

zwolf Jul 31, 2018

camallen Aug 2, 2018

houndci-bot Jul 30, 2018

houndci-bot Jul 30, 2018

houndci-bot Jul 30, 2018

houndci-bot Jul 30, 2018

houndci-bot Jul 30, 2018

camallen left a comment •

edited

Loading

camallen Aug 2, 2018

zwolf Aug 2, 2018

camallen Aug 2, 2018

zwolf Aug 2, 2018

camallen Aug 2, 2018

camallen Aug 2, 2018

zwolf Aug 2, 2018

camallen Aug 2, 2018

zwolf Aug 2, 2018

camallen Aug 2, 2018

zwolf Aug 2, 2018

camallen Aug 2, 2018

zwolf Aug 2, 2018

zwolf commented Aug 2, 2018 •

edited

Loading

houndci-bot Aug 2, 2018

houndci-bot Aug 2, 2018

zwolf Aug 2, 2018

camallen left a comment

camallen Aug 7, 2018

zwolf Aug 7, 2018

camallen Aug 7, 2018

zwolf Aug 7, 2018

camallen Aug 7, 2018

zwolf Aug 7, 2018

houndci-bot Aug 7, 2018

		@@ -0,0 +1,30 @@
		class ProjectCopier
		EXCLUDE_ATTRIBUTES = [ :classifications_count, :launched_row_order, :beta_row_order ]

		@@ -1,4 +1,4 @@
		class SubjectWorkflowStatusCreateWorker
		class SubjectWorkflowStatusCreateWorker

		@@ -0,0 +1,29 @@
		class ProjectCopier
		EXCLUDE_ASSOCIATIONS = [ :classifications_count, :launched_row_order, :beta_row_order ].freeze

Project copying #2861

Project copying #2861

Conversation

zwolf commented Jul 30, 2018

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camallen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwolf commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camallen left a comment •

edited

Loading

zwolf commented Aug 2, 2018 •

edited

Loading