Skip to content

Boutiques portal task#1629

Draft
natacha-beck wants to merge 8 commits into
aces:masterfrom
natacha-beck:boutiques_portal_task
Draft

Boutiques portal task#1629
natacha-beck wants to merge 8 commits into
aces:masterfrom
natacha-beck:boutiques_portal_task

Conversation

@natacha-beck
Copy link
Copy Markdown
Contributor

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.

@natacha-beck natacha-beck marked this pull request as draft May 15, 2026 16:05
Copy link
Copy Markdown
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ||
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Boutiques disabled input issue in before_form and after_form

2 participants