-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Projects' add new dropdown #1643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would also be good if @fbacall could check the workflow bits
@@ -11,6 +11,7 @@ module WorkflowSupport | |||
def create_ro_crate | |||
@crate_builder = Legacy::WorkflowCrateBuilder.new(legacy_ro_crate_params) | |||
@workflow.workflow_class = @crate_builder.workflow_class = WorkflowClass.find_by_id(params[:workflow_class_id]) | |||
@workflow.project_ids |= params[:workflow] ? params[:workflow][:project_ids] : params[:project_ids] || [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is in the right place, it doesn't belong as part of create_ro_crate
and should have been set already.
I think you just need to add workflow[project_ids]
to the permitted params in legacy_ro_crate_params and also possibly update legacy_set_workflow
for what reason do we need to handle params[:project_ids]
, rather than it always being workflow[project_ids]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @stuzart here. Also for the actions in WorkflowsController.
It's possible to do it all via the params, but unfortunately you have to do something slightly different in each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be fine to have it always as params[:project_ids]... I've modified the code to include this, and moved the parameter of project ids to be added in the legacy params. I believe it now all works again, but please do test it out.
See 614e01f.
@@ -31,6 +32,7 @@ def create_content_blob | |||
if handle_upload_data && @workflow.content_blob.save | |||
@content_blob = @workflow.content_blob | |||
@workflow = workflow | |||
@workflow.project_ids |= params[:workflow] ? params[:workflow][:project_ids] : params[:project_ids] || [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as for create_ro_crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 614e01f.
Includes all assets that can be linked to projects in the "add new" dropdown menu.
Resolves #1614.