Skip to content
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

Functional prototype for static values in Scripts #3059

Merged
merged 4 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
10 changes: 6 additions & 4 deletions apps/dashboard/app/controllers/scripts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ class ScriptsController < ApplicationController
before_action :find_script, only: [:show, :edit, :destroy, :submit, :save]

SAVE_SCRIPT_KEYS = [
:cluster, :auto_accounts, :auto_accounts_exclude, :auto_scripts, :auto_scripts_exclude,
:auto_queues, :auto_queues_exclude, :auto_batch_clusters, :auto_batch_clusters_exclude,
:bc_num_slots, :bc_num_slots_min, :bc_num_slots_max,
:bc_num_hours, :bc_num_hours_min, :bc_num_hours_max
:cluster, :auto_accounts, :auto_accounts_exclude, :auto_accounts_fixed,
:auto_scripts, :auto_scripts_exclude, :auto_scripts_fixed,
:auto_queues, :auto_queues_exclude, :auto_queues_fixed,
:auto_batch_clusters, :auto_batch_clusters_exclude, :auto_batch_clusters_fixed,
:bc_num_slots, :bc_num_slots_fixed, :bc_num_slots_min, :bc_num_slots_max,
:bc_num_hours, :bc_num_hours_fixed, :bc_num_hours_min, :bc_num_hours_max
].freeze

def new
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
# Helper for creating new batch connect sessions.
module BatchConnect::SessionContextsHelper
def create_widget(form, attrib, format: nil, hide_excludable: true)
return '' if attrib.fixed?
def create_widget(form, attrib, format: nil, hide_excludable: true, hide_fixed: true)
return '' if hide_fixed && attrib.fixed?
johrstrom marked this conversation as resolved.
Show resolved Hide resolved
return '' if attrib.hide_when_empty? && attrib.value.blank?

widget = attrib.widget
field_options = attrib.field_options(fmt: format)
all_options = attrib.all_options(fmt: format)

if attrib.fixed?
return render :partial => "batch_connect/session_contexts/fixed", :locals => { form: form, attrib: attrib, field_options: field_options, format: format }
end

case widget
when 'select'
form.select(attrib.id, attrib.select_choices(hide_excludable: hide_excludable), field_options, attrib.html_options)
Expand Down
6 changes: 5 additions & 1 deletion apps/dashboard/app/helpers/scripts_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ module ScriptsHelper
def create_editable_widget(form, attrib, format: nil)
widget = attrib.widget
attrib.html_options = { class: 'real-field', autocomplete: 'off' }
locals = { form: form, attrib: attrib, format: format }
# For editable script elements, we want the standard render form even when they are fixed.
# We need to reset the fixed attribute to avoid being render as a read only text field.
fixed_attribute = attrib.fixed?
attrib.opts[:fixed] = false
johrstrom marked this conversation as resolved.
Show resolved Hide resolved
locals = { form: form, attrib: attrib, format: format, fixed: fixed_attribute }

case widget
when 'number_field'
Expand Down
41 changes: 41 additions & 0 deletions apps/dashboard/app/javascript/packs/script_edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,33 @@ function addInProgressField(event) {
justAdded.find('[data-select-toggler]')
.on('click', (event) => { enableOrDisableSelectOption(event) });

justAdded.find('[data-fixed-toggler]')
.on('click', (event) => { toggleFixed(event) });

const entireDiv = event.target.parentElement.parentElement.parentElement;
entireDiv.remove();
}

function fixedFieldEnabled(checkbox, dataElement) {
dataElement.disabled = true;
const input = $('<input>').attr('type','hidden').attr('name', dataElement.name).attr('value', dataElement.value);
$(checkbox).after(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change the HTML tag type or can we just toggle the readonly attribute?

Copy link
Contributor Author

@abujeda abujeda Sep 26, 2023

Choose a reason for hiding this comment

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

Select elements do not have readonly state, so to get the current behaviour we need to disable and add hidden field.

Do we need to change the HTML tag

Not sure if this means to change from select to input type=text to make it readonly, but it seems a more complex solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Select elements do not have readonly state, so to get the current behaviour we need to disable and add hidden field.

Editing the page on FF seems to respect the readonly - though MDN docs say it shouldn't. Further reading suggests it was deprecated in html5? But it's unclear how to find official RFC for the same.

image

Not sure if this means to change from select to input type=text to make it readonly, but it seems a more complex solution

I thought that's what this was doing - but now see it adds a hidden element instead of replacing the original. I guess this is because the browser won't send a disabled form element? Seems like it'll send readonly - but I cannot actually find if it's actually been deprecated on select fields or what.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - I think what you have is what we should go with. That is (if I'm reading it right),

  • disabling the original element
  • creating a new hidden element with the same name so that it'll be passed in the form

and of course the inverse too.

Maybe there's a way to clean this up so it's a little more apparent what's going on? It's created in one place and removed in another. Maybe just colocate these?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - I think what you have is what we should go with

I think this because I cannot find any docs on why my example shown works. The currrent RFC (below) for html5 doesn't specify it and MDN docs (also below) explicitly call it out as not working.

Which is to say - relying on adding readonly to a select widget is likely unstable (not to mention how other browsers react, I only looked doing this on firefox).

https://www.w3.org/TR/html5-author/the-select-element.html#the-select-element
https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is disabling and adding a hidden field with the same name as the disabled field will not be sent in the request.
I tried the readonly first as it was more intuitive, but in my Firefox did not work. Reading later I came across that select elements do not have a readonly and I found the recommended way of adding the hidden field when disabled.

I will colocate the methods and add comments to make it more obvious what is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some comments. The add/remove code is close to each other. Not in the same method to reuse the add in the initialization code.

}

function toggleFixed(event) {
event.target.disabled = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we disabling the event target here just to re-enable it afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is usually a good practice to avoid double submitting/processing while the first click is being handled.

In this particular case might too much. I can remove if you think it is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is usually a good practice to avoid double submitting/processing while the first click is being handled.

First click? Are you saying this event is triggered twice by 1 click (click up and click down events?) or do you mean double clicking?

We can keep it if it serves some purpose, as you say as a good practice, I'm just curious mostly now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant double clicking. But I think in this particular case even a double click will have no side effects.

const elementId = event.target.dataset.fixedToggler;
const dataElement = document.getElementById(elementId);
if (event.target.checked) {
fixedFieldEnabled(event.target, dataElement)
} else {
dataElement.disabled = false;
$(`input[type=hidden][name="${dataElement.name}"]`).remove();
}

event.target.disabled = false;
}

function enableOrDisableSelectOption(event) {
const toggleAction = event.target.dataset.selectToggler;
const li = event.target.parentElement;
Expand Down Expand Up @@ -199,6 +222,20 @@ function initSelectFields(){
});
}


function initFixedFields(){
const fixedCheckboxes = Array.from($('[data-fixed-toggler]'));

// find all the enabled 'fixed' checkboxes
fixedCheckboxes.filter((fixedFieldCheckbox) => {
return fixedFieldCheckbox.checked;
// now disable the select field
}).map((fixedFieldCheckbox) => {
const dataElement = document.getElementById(fixedFieldCheckbox.dataset.fixedToggler);
fixedFieldEnabled(fixedFieldCheckbox, dataElement)
});
}

jQuery(() => {
newFieldTemplate = $('#new_field_template');
$('#add_new_field_button').on('click', (event) => { addNewField(event) });
Expand All @@ -215,5 +252,9 @@ jQuery(() => {
$('[data-select-toggler]')
.on('click', (event) => { enableOrDisableSelectOption(event) });

$('[data-fixed-toggler]')
.on('click', (event) => { toggleFixed(event) });

initSelectFields();
initFixedFields();
});
5 changes: 3 additions & 2 deletions apps/dashboard/app/models/script.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def respond_to_missing?(method_name, include_private = false)
end

def original_parameter(string)
match = /([\w_]+)_(?:min|max|exclude)/.match(string)
match = /([\w_]+)_(?:min|max|exclude|fixed)/.match(string)
match[1]
end

Expand Down Expand Up @@ -201,7 +201,7 @@ def self.script_form_file(script_path)
# parameters you got from the controller that affect the attributes, not form.
# i.e., mins & maxes you set in the form but get serialized to the 'attributes' section.
def attribute_parameter?(name)
name.end_with?('_min') || name.end_with?('_max') || name.end_with?('_exclude')
['min', 'max', 'exclude', 'fixed'].any? { |postfix| name && name.end_with?("_#{postfix}") }
end

# update the 'form' portion of the yaml file given 'params' from the controller.
Expand All @@ -222,6 +222,7 @@ def update_attributes(params)
orig_param = original_parameter(key).to_sym
self[orig_param].min = value if key.end_with?('_min') && !value.to_s.empty?
self[orig_param].max = value if key.end_with?('_max') && !value.to_s.empty?
self[orig_param].opts[:fixed] = true if key.end_with?('_fixed')

if key.end_with?('_exclude')
exclude_list = value.split(',').to_a
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<%-
all_options = attrib.all_options(fmt: format)
all_options[:readonly] = true
%>

<%= form.text_field(attrib.id, all_options) %>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<%-
field_id = "#{form.object_name}_#{attrib.id}"

fixed_id = "#{field_id}_fixed"
fixed_name = "#{form.object_name}[#{attrib.id}_fixed]"
-%>
<div class="list-group col-md-4">
<div class="list-group-item mb-3">
<input type="checkbox" id="<%= fixed_id %>" name="<%= fixed_name %>" value="true" <%= fixed ? 'checked' : '' %> data-fixed-toggler="<%= field_id %>">
<label class="form-check-label" for="<%= fixed_id %>"><%= t('dashboard.jobs_scripts_fixed_field') %></label>
</div>
</div>
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%-
<%-
field_id = "#{form.object_name}_#{attrib.id}"
min_edit_id = "#{field_id}_min"
min_edit_name = "#{form.object_name}[#{attrib.id}_min]"
Expand All @@ -8,13 +8,18 @@

current_min = attrib.field_options[:min]
current_max = attrib.field_options[:max]

fixed_id = "#{field_id}_fixed"
fixed_name = "#{form.object_name}[#{attrib.id}_fixed]"
-%>

<div class="editable-form-field">
<%= create_widget(form, attrib, format: format) %>
<%= create_widget(form, attrib, format: format, hide_fixed: false) %>

<div class="d-none edit-group mb-3">

<%= render(partial: 'scripts/editable_form_fields/edit_fixed_field', locals: { form: form, attrib: attrib, fixed: fixed }) %>

<label for="<%= min_edit_id %>">Minimum</label>
<input min="1" step="1"
class="form-control edit-field" type="number" value="<%= current_min %>"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
-%>

<div class="editable-form-field">
<%= create_widget(form, attrib, format: format, hide_excludable: false) %>
<%= create_widget(form, attrib, format: format, hide_excludable: false, hide_fixed: false) %>

<div class="d-none edit-group">


<%= render(partial: 'scripts/editable_form_fields/edit_fixed_field', locals: { form: form, attrib: attrib, fixed: fixed }) %>

<ol class="list-group text-center col-md-4 mb-3">
<%- attrib.select_choices(hide_excludable: false).each do |select_data| %>
<%-
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
-%>

<div class="editable-form-field">
<%= create_widget(form, attrib, format: format) %>
<%= create_widget(form, attrib, format: format, hide_fixed: false) %>

<div class="d-none edit-group">
</div>
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/views/scripts/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<%= bootstrap_form_for(@script, url: submit_project_script_path) do |f| %>
<% @script.smart_attributes.each do |attrib| %>
<%# TODO generate render_format %>
<%= create_widget(f, attrib, format: nil) %>
<%= create_widget(f, attrib, format: nil, hide_fixed: false) %>
<% end %>

<%= f.submit t('dashboard.batch_connect_form_launch'), class: "btn btn-primary btn-block" %>
Expand Down
1 change: 1 addition & 0 deletions apps/dashboard/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ en:
jobs_scripts_deleted: "Script successfully deleted!"
jobs_scripts_submitted: "Successfully submited job %{job_id}."
jobs_scripts_delete_script_confirmation: "Delete all contents of script?"
jobs_scripts_fixed_field: "Fixed Value"

settings_updated: "Settings updated"

Expand Down
10 changes: 10 additions & 0 deletions apps/dashboard/test/models/script_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
require 'test_helper'

class ScriptTest < ActiveSupport::TestCase
test 'supported field postfix' do
target = Script.new({ project_dir: '/path/project', id: 1234, title: 'Test Script' })
refute target.send('attribute_parameter?', nil)
refute target.send('attribute_parameter?', '')
refute target.send('attribute_parameter?', 'account_notsupported')
assert target.send('attribute_parameter?', 'account_min')
assert target.send('attribute_parameter?', 'account_max')
assert target.send('attribute_parameter?', 'account_exclude')
assert target.send('attribute_parameter?', 'account_fixed')
end
test 'creates script' do
Dir.mktmpdir do |tmp|
projects_path = Pathname.new(tmp)
Expand Down
2 changes: 2 additions & 0 deletions apps/dashboard/test/system/jobs_app_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ def add_bc_num_hours(project_id, script_id)
fill_in('script_bc_num_hours', with: 42)
fill_in('script_bc_num_hours_min', with: 20)
fill_in('script_bc_num_hours_max', with: 101)
find('#script_bc_num_hours_fixed').click
find('#save_script_bc_num_hours').click

# correctly saves
Expand Down Expand Up @@ -439,6 +440,7 @@ def add_bc_num_hours(project_id, script_id)
min: 20
step: 1
value: '42'
fixed: true
johrstrom marked this conversation as resolved.
Show resolved Hide resolved
max: 101
label: Number of hours
help: ''
Expand Down
Loading