Add button to duplicate job definition #92
Conversation
|
@cookpad/dev-infra review please |
| @@ -81,7 +81,7 @@ def proceed_multi_instance? | |||
| end | |||
|
|
|||
| def text_tags | |||
| tags.pluck(:name).join(',') | |||
| tags.map(&:name).join(',') | |||
There was a problem hiding this comment.
Without this change, text_tags of duplicated definition always returns empty.
definition = Kuroko2::JobDefinition.first
duplicated_definition = definition.dup
duplicated_definition.tags = definition.tags.dup
definition.tags.pluck(:name) #=> ["HelloWorld"]
definition.tags.map(&:name) #=> ["HelloWorld"]
duplicated_definition.tags.pluck(:name) #=> []
duplicated_definition.tags.map(&:name) #=> ["HelloWorld"]
There was a problem hiding this comment.
text_tags method is called at here.
In new page, text_tags would return empty array without this change because the job_definition having this tags does not exist in DB (so #pluck returns empty array).
Is it right? @takonomura
| dup_from = Kuroko2::JobDefinition.find_by(id: params[:dup_from]) | ||
| if dup_from.present? | ||
| @definition = dup_from.dup | ||
| @definition.admins = dup_from.admins.dup |
| if dup_from.present? | ||
| @definition = dup_from.dup | ||
| @definition.admins = dup_from.admins.dup | ||
| @definition.tags = dup_from.tags.dup |
| expect(response).to render_template('new') | ||
|
|
||
| expect(assigns(:definition)).to be_new_record | ||
| expect(assigns(:definition).name).to eq definition.name |
There was a problem hiding this comment.
Could you write expect expressions about tags and admins ?
| expect(assigns(:definition).admins).to match_array [admin_user, controller.current_user] | ||
| end | ||
|
|
||
| context 'with other admins' do |
There was a problem hiding this comment.
'with only anther admin user' ?
| expect(assigns(:definition).name).to eq definition.name | ||
| end | ||
|
|
||
| context 'with admins' do |
There was a problem hiding this comment.
'with current_user and another admin user'
takonomura
force-pushed
the
feature/duplicate-job-definition
branch
from
d4a8bf3 to
047f318
Compare
| expect(response).to have_http_status(:ok) | ||
| expect(response).to render_template('new') | ||
|
|
||
| expect(assigns(:definition)).to be_new_record |
There was a problem hiding this comment.
It seems better to check what a field is blank.
|
|
||
| expect(assigns(:definition)).to be_new_record | ||
| expect(assigns(:definition).name).to eq definition.name | ||
| definition.admins << controller.current_user unless definition.admins.include?(controller.current_user) |
There was a problem hiding this comment.
It seems to better for do not change definition instance in it block.
How about changing it to the below code?
expect(assigns(:definition).admins).to eq(definition.admins + controller.current_user)
takonomura
force-pushed
the
feature/duplicate-job-definition
branch
from
047f318 to
a543c3e
Compare
|
|
||
| expect(assigns(:definition)).to be_new_record | ||
| expect(assigns(:definition).name).to eq definition.name | ||
| expect(assigns(:definition).admins).to eq definition.admins + [controller.current_user] |
There was a problem hiding this comment.
[nits]
[definition.admins, controller.current_user] seems to be better.
There was a problem hiding this comment.
expect(assigns(:definition).admins).to eq [definition.admins, controller.current_user] is not working.
There was a problem hiding this comment.
oh..sorry, I misunderstood. definition.admins is array.
| expect(assigns(:definition).tags).to eq definition.tags | ||
| end | ||
|
|
||
| context 'with current_user and aother admin user' do |
There was a problem hiding this comment.
Thanks, I fixed it in 33ff102.
Add button for duplicate job definition to Job Definition Details page.
This allows us to duplicate the definition for similarly jobs.
On press the button, it open New Job Definition page with pre-filled the definition.
Before:
After: