Project Copier: tweaks to initialization

[lcjohnso]

Add additional fields to EXCLUDE_ATTRIBUTES so that they are not carried over to newly-copied project from original project.

[yuenmichelle1]

Hey Toyosi!

I like your idea of checking that the new copied projects have excluded attributes as whatever the defaults are, but I think the original project also had these attributes as defaults. (View factory here to see what attributes were set for the original project).
Because of that, I don't think we are truly testing that these attributes are actually not being copied over.

I think a better test would be to set the excluded attributes of the original project and then check that the copied project's excluded attributes does not equal that of the original project. (Then we don't have to add the secondary test about launched_row_order and beta_row_order)

Maybe something like:

it 'does not copy over excluded attributes' do
  project_with_excluded_keys = create(:full_project, classifications_count: 3, classifiers_count: 2, launch_date: Date.yesterday, completeness: 0.5, activity: 1, lock_version: 8)

   other_copied_project = described_class.new(project_with_excluded_keys.id, copyist.id).copy

    ProjectCopier::EXCLUDE_ATTRIBUTES.each do |attr|
        expect(other_copied_project[attr]).not_to eq(project_with_excluded_keys[attr])
    end
end

Side note:
If you did want to check that the new copied project has defaults set in addition to above test, I would suggest using a hash as opposed to a default_values array so we don't have to use an index to keep track of what attribute we're comparing.

[yuenmichelle1]

Unrelated, but I dont think launched_row_order and beta_row_order are primary keys.

I think for projects the only primary key is id. BUT I do think launched_row_order and beta_row_order are indexes of project. (See: here and here )

[Tooyosi]

Hey Toyosi!

I like your idea of checking that the new copied projects have excluded attributes as whatever the defaults are, but I think the original project also had these attributes as defaults. (View factory here to see what attributes were set for the original project). Because of that, I don't think we are truly testing that these attributes are actually not being copied over.

I think a better test would be to set the excluded attributes of the original project and then check that the copied project's excluded attributes does not equal that of the original project. (Then we don't have to add the secondary test about launched_row_order and beta_row_order)

Maybe something like:

it 'does not copy over excluded attributes' do
  project_with_excluded_keys = create(:full_project, classifications_count: 3, classifiers_count: 2, launch_date: Date.yesterday, completeness: 0.5, activity: 1, lock_version: 8)

   other_copied_project = described_class.new(project_with_excluded_keys.id, copyist.id).copy

    ProjectCopier::EXCLUDE_ATTRIBUTES.each do |attr|
        expect(other_copied_project[attr]).not_to eq(project_with_excluded_keys[attr])
    end
end

Side note: If you did want to check that the new copied project has defaults set in addition to above test, I would suggest using a hash as opposed to a default_values array so we don't have to use an index to keep track of what attribute we're comparing.

Thanks, this makes alot of sense. I initially thought to compare the attributes between both projects but felt to compare with the defaults. But this works well especially with the launched_row_order and beta_row_order indexes

[yuenmichelle1]

Thanks, this makes alot of sense. I initially thought to compare the attributes between both projects but felt to compare with the defaults. But this works well especially with the launched_row_order and beta_row_order indexes

Awesome! Glad it made sense :) @Tooyosi Whenever you are ready, feel free to re-request my review and I'll take a look at it.

[yuenmichelle1]

LGTM 🎉 👍