Boutiques portal task#1629
Conversation
prioux
left a comment
There was a problem hiding this comment.
Review my suggestions, it's possible my code examples have bugs.
| "#{iname} #{ioptional}\n" | ||
| }.join("") | ||
|
|
||
| # List of file that can be disable (requested or optional) |
There was a problem hiding this comment.
So this new piece of code is complicated to read. First, the name of the variable. input_files_can_be_disabled is not really about how they CAN be disabled, but how they COULD. And it's a true/false value that turns true if any input file is listed as a potentially disabled in any other input. I'm not convinced that useful until we actually verify if it's the case.
For the code, I see a complicated structure of three any? embedded in each other. That's unreadable. Also, the input being scanned is accessed using the [] operator instead of the method. Here's my attempt to rewrite it, but please double check it too:
all_file_ids = needed_files_ids | optional_files_ids # union of all IDs
input_files_can_be_disabled = descriptor.inputs
.select { |input| input.value_disables.present? } # subset of inputs that disables other inputs
.any? { |input| (input.value_disables.values.flatten & all_file_ids).present? } # are there at least one of those that disables any of the file inputs| if num_in_files < num_needed_inputs || num_in_files > num_needed_inputs+num_opt_inputs | ||
| if (! descriptor.qualified_to_launch_multiple_tasks? || num_in_files == 0) | ||
| if num_in_files < num_needed_inputs || num_in_files > num_needed_inputs + num_opt_inputs | ||
| if (! descriptor.qualified_to_launch_multiple_tasks? || num_in_files == 0) && !input_files_can_be_disabled |
There was a problem hiding this comment.
Again, like I said in my previous comment, we're not going to get the message if there are any POTENTIAL disabling, bot ACTUAL disabling. I don't think that's right.
| # Disabled via disables-inputs | ||
| # Eg: | ||
| # "disables-inputs": [ "mask" ] | ||
| (other_input.disables_inputs || []).include?(input.id) || |
There was a problem hiding this comment.
So lines 491 and 500 make up this huge expression like this:
(other_input.disables_inputs || []).include?(input.id) || ((other_input['value-disables'] || {})[invoke_params[other_input.id].to_s] || []).include?(input.id)This is unreadable, even with the comments. You are currently within a Ruby any? loop, so please use the loop shortcuts to evaluate each clause separately and break out of the loop early. e.g.
next true if (other_input.disables_inputs || []).include?(input.id)
current_value = invoke_params[other_input.id].to_s
disable_map = other_input.value_disables || {}
disable_ids = disable_map[current_value] || []
next true if disable_ids.include?(input.id) # the "next true if" is not really needed here
You could also shortcut even earlier with a variant:
disable_map = other_input.value_disables || next false
disable_ids = disable_map[current_value] || next false
but this is tricky if we ever add more conditions within the loop.
Fixed #1628
By bypass the verification in before form in case of disabled input files.
By adding logic to after form in order to have the same patterns as it is for isInactive.