-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[24.1] Fix input_step_parameters
missing values that don't have a label
#18405
[24.1] Fix input_step_parameters
missing values that don't have a label
#18405
Conversation
dc4374d
to
f72a14f
Compare
lib/galaxy/model/__init__.py
Outdated
step_id = security.encode_id(input_step_parameter.workflow_step_id) | ||
else: | ||
step_id = input_step_parameter.workflow_step_id | ||
input_parameters[step_id] = { |
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.
That's a breaking change, we can't do that
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.
Ok, but we still need to include input parameters without labels in the invocation.input_parameters
, so do we just use dummy labels instead? Do we pass a dummy label from the frontend or do we set one in the backend in this same function?
@@ -8921,12 +8921,13 @@ def to_dict(self, view="collection", value_mapper=None, step_details=False, lega | |||
|
|||
input_parameters = {} | |||
for input_step_parameter in self.input_step_parameters: | |||
label = input_step_parameter.workflow_step.label | |||
if not label: |
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.
You could use something like f"{input_step_parameter.workflow_step.order_index + 1}: Unnamed parameter"
. That's going to be unique enough I guess. In a followup I would suggest making the label field required in the editor.
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.
Ok, that sounds good.
Can I still make the label field required within the same PR, or should it be a new one that targets 24.1 or dev?
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.
That depends, I don't think this is straightforward as you have to check for uniqueness and nudge the user to fill in the label. I would prefer separate PRs.
`input_step_parameters` that did not have labels (label=None) were being excluded in the `WorkflowInvocation.to_dict()` result. This therefore sets the label to `n: Unnamed parameter` for null labels. Fixes galaxyproject#18366
This is a bug caused in galaxyproject#18378 which made the `SaveChangesModal` useless because the version before the save was routed to. Now, we instead append the version to the route after the user confirms the save (or doesn't) in the modal.
f72a14f
to
b5cabbf
Compare
Thank you @ahmedhamidawan! |
input_step_parameters
that did not have labels (label=None) were being excluded from theWorkflowInvocation.to_dict()
result.This therefore sets the label to
n: Unnamed parameter
for null labels.Fixes #18366
How to test the changes?
(Select all options that apply)
License