Skip to content

Boutiques portal task #1629

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.

Sign up for GitHub

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

Draft
natacha-beck wants to merge 8 commits into
base: master
Choose a base branch
from
+58 −23
Draft

Boutiques portal task #1629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cae5e79
[FIX] show_table render when re-render the form with a subclass
Aug 5, 2025
7d2e0de
Merge branch 'dev' of github.com:aces/cbrain into dev
Sep 29, 2025
f3bd8de
Merge branch 'dev' of github.com:aces/cbrain into dev
Oct 17, 2025
4028bb9
Merge branch 'dev' of github.com:aces/cbrain into dev
Dec 18, 2025
4ec746a
Merge branch 'dev' of github.com:aces/cbrain into dev
Mar 11, 2026
536ea46
Merge branch 'dev' of github.com:aces/cbrain into dev
Apr 13, 2026
3d9c39a
Merge branch 'dev' of github.com:aces/cbrain into dev
May 12, 2026
b9562b7
[FIX] before_form and after_form in case of disabled value
May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 50 additions & 17 deletions BrainPortal/app/models/boutiques_portal_task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,13 @@ def default_launch_args #:nodoc:
end

def before_form
descriptor = self.descriptor_for_before_form
descriptor = self.descriptor_for_before_form

num_needed_inputs = descriptor.required_file_inputs.size
num_opt_inputs = descriptor.optional_file_inputs.size
num_in_files = Userfile.where(:id => (self.params[:interface_userfile_ids] || [])).count
needed_files_ids = descriptor.required_file_inputs.map(&:id)
num_needed_inputs = needed_files_ids.size
optional_files_ids = descriptor.optional_file_inputs.map(&:id)
num_opt_inputs = optional_files_ids.size
num_in_files = Userfile.where(:id => (self.params[:interface_userfile_ids] || [])).count

# This is a message describing briefly all file inputs
input_infos = descriptor.file_inputs
Expand All @@ -113,10 +115,17 @@ def before_form
"#{iname} #{ioptional}\n"
}.join("")

# List of file that can be disable (requested or optional)
Copy link
Copy Markdown
Member

@prioux prioux May 26, 2026

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

input_files_can_be_disabled = descriptor.inputs.any? do |input|
(input['value-disables'] || {}).any? do |_, disabled_inputs|
(needed_files_ids & disabled_inputs).any? || (optional_files_ids & disabled_inputs).any?
end
end

# return "Warning: you selected more files than this task requires, so you won't be able to assign them all."
# Not available in case of descriptor qualified to launch multiple task
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

@prioux prioux May 26, 2026

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.

message = "This task requires #{num_needed_inputs} mandatory file(s) and #{num_opt_inputs} optional file(s)\n" +
input_infos
cb_error message
Expand All @@ -138,6 +147,8 @@ def after_form

# Required parameters
descriptor.required_inputs.each do |input|
# skip if it is disabled
next if isDisabled(input)
# skip if the input is the sole mandatory file and if
# the task is qualified to launch multiple tasks
# only if no params was provide
Expand All @@ -150,7 +161,7 @@ def after_form

# Optional parameters
descriptor.optional_inputs.each do |input|
sanitize_param(input) unless isInactive(input)
sanitize_param(input) unless isInactive(input) || isDisabled(input)
end

# ---------------------------------------------------------------
Expand All @@ -165,15 +176,15 @@ def after_form
# ---------------------------------------------------------------

descriptor.inputs.select(&:value_choices).each do |input|
check_enum_param(input) unless isInactive(input)
check_enum_param(input) unless isInactive(input) || isDisabled(input)
end

# -------------------------------------------------------------------------------
# Check that number parameters with contraints have been given permissible values
# -------------------------------------------------------------------------------

descriptor.inputs.select { |input| input.type == 'Number' }.each do |input|
check_number_param(input) unless isInactive(input)
check_number_param(input) unless isInactive(input) || isDisabled(input)
end

# ----------------------------------------
Expand All @@ -182,7 +193,7 @@ def after_form

descriptor.list_inputs.each do |input|
next if input.type == 'File' # these are special
check_list_param(input) unless isInactive(input)
check_list_param(input) unless isInactive(input) || isDisabled(input)
end

# ---------------------------------
Expand All @@ -191,7 +202,7 @@ def after_form

descriptor.inputs.each do |input|

next if isInactive(input)
next if isInactive(input) || isDisabled(input)

# Inputs that want other inputs to NOT BE provided
(input.disables_inputs || []).each do |dontneed| # loop on IDs
Expand Down Expand Up @@ -469,6 +480,27 @@ def isInactive(input)
)
end

def isDisabled(input)
descriptor = self.descriptor_for_after_form
descriptor.inputs.any? do |other_input|
# An inactive input cannot disable an other one
next false if isInactive(other_input)
# Disabled via disables-inputs
# Eg:
# "disables-inputs": [ "mask" ]
(other_input.disables_inputs || []).include?(input.id) ||
Copy link
Copy Markdown
Member

@prioux prioux May 26, 2026

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.

# Disabled via value-disables for the current chosen value
# Eg:
# "value-disables": {
# "setup": [
# "working_dir",
# "PAR_demuxalot_genotype_names"
# ]
# }
((other_input['value-disables'] || {})[invoke_params[other_input.id].to_s] || []).include?(input.id)
end
end

# Checks that the cbcsv is the correct type
# Current implementation will output an error here if a person uploads a cbcsv
# but forgets to change its type to cbcsv. I.e. we assume it is an error to use
Expand Down Expand Up @@ -697,7 +729,7 @@ def check_list_param(input)

def check_mutex_group(group, descriptor = self.descriptor_for_after_form)
members = group.members
are_set = members.select { |inputid| ! isInactive(descriptor.input_by_id inputid) }
are_set = members.select { |inputid| ! isInactive(descriptor.input_by_id inputid) && ! isDisabled(descriptor.input_by_id inputid) }
return if are_set.size <= 1
are_set.each do |inputid|
params_errors.add(group.name, " can have at most one parameter set")
Expand All @@ -708,16 +740,17 @@ def check_mutex_group(group, descriptor = self.descriptor_for_after_form)

def check_oneisrequired_group(group, descriptor = self.descriptor_for_after_form)
members = group.members
are_set = members.select { |inputid| ! isInactive(descriptor.input_by_id inputid) }
are_set = members.select { |inputid| ! isInactive(descriptor.input_by_id inputid) && ! isDisabled(descriptor.input_by_id inputid) }
return if are_set.size > 0
params_errors.add(group.name, " need at least one parameter set")
end

def check_allornone_group(group, descriptor = self.descriptor_for_after_form)
members = group.members
num_unset = members.count { |inputid| isInactive(descriptor.input_by_id inputid) }
return if num_unset == 0 || num_unset == members.size # all, or none
params_errors.add(group.name, " need either all the parameters set or neither")
members = group.members
active_members = members.reject { |inputid| isDisabled(descriptor.input_by_id(inputid)) }
num_unset = active_members.count { |inputid| isInactive(descriptor.input_by_id(inputid)) }
# In case of disabled value use active_members to check
return if num_unset == 0 || num_unset == active_members.size
end

# MAYBE IN COMMON
Expand Down
14 changes: 8 additions & 6 deletions ...tal/cbrain_plugins/cbrain-plugins-base/cbrain_task/boutiques_task/views/_form_js.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,13 @@ makeReceiverWithValueActionMap = lambda do |action|
doesMap[id] = doesMap[id] || {};
if (doesMap[id] && doesMap[id][disable_by])
doesMap[id][disable_by].push(value)
elsif (doesMap[id])
elsif (doesMap[id])
doesMap[id][disable_by] = [value]
end
end
end
end

doesMap
end

Expand Down Expand Up @@ -171,8 +171,8 @@ $(function () {
var isActiveParamWithValues = function(disable_by, disabled_by_values) {
var targ = $( 'li.' + ("btq-" + disable_by) + '.tsk-prm' );

var current_values = ($(targ[0].querySelector('select')) && $(targ[0].querySelector('select')).val()) ||
[targ[0].querySelector('input[type="hidden"]') && targ[0].querySelector('input[type="hidden"]').value];
var current_values = ($(targ[0].querySelector('select')) && $(targ[0].querySelector('select')).val()) ||
[targ[0].querySelector('input[type="hidden"]') && targ[0].querySelector('input[type="hidden"]').value];
current_values = (current_values && current_values.filter(k => k !== "")) || [];

return current_values.filter(value => disabled_by_values.includes(value));
Expand Down Expand Up @@ -402,6 +402,8 @@ $(function () {
handleNumberConstraints();
})

/*** Need to call it when the form is displayed (fix bug when reload form)
handleDisablesAndRequires();

/*** Optional parameters ***/
parameters.delegate('.tsk-prm-sel', 'click', function(){
Expand All @@ -414,7 +416,7 @@ $(function () {
})

/* Clicking on the parameter's checkbox toggles the parameter's state */
parameters.delegate('.tsk-prm-opt, .tsk-prm-chk, .tsk-prm-sel, .tsk-prm-sel-mult', 'change activate.tsk-prm', function () {
parameters.delegate('.tsk-prm-opt, .tsk-prm-chk, .tsk-prm-sel, .tsk-prm-sel-mult', 'change activate.tsk-prm', function () {
var opt = $(this),
param = opt.parent();

Expand Down Expand Up @@ -584,7 +586,7 @@ $(function () {
// Helper for extracting value lists
var getVals = function( id, join ) {
var vals = $('li.' + ('btq-' + id) + '.tsk-prm').find('.tsk-prm-in').map( function(){ return $(this).val(); } );

if (vals.length === 0 ) {
var li = $('li.' + ('btq-' + id));
vals = li.find(":selected").map(idx => li.find(":selected")[idx]['value']);
Expand Down
Loading