Functional prototype for static values in Scripts #3059

abujeda · 2023-09-20T08:42:50Z

Prototype to implement fixes values within new Job Composer scripts.

I am not sure about the UI as the combination of the dropdown + Add/Remove actions + Set as static are all linked.
I am not sure either of using the SmartAttributes fixed property to implement this feature, but seems like the right approach.

This PR shows what I had to do to make it work with SmartAttributes and the fixed property, but it is more a hack than a PR.

@johrstrom what is your idea for the UI?
And for the backend? Should the fixed property be use for this feature?

abujeda · 2023-09-20T08:44:48Z

apps/dashboard/app/helpers/scripts_helper.rb

@@ -4,6 +4,8 @@
 module ScriptsHelper
  def create_editable_widget(form, attrib, format: nil)
    widget = attrib.widget
+    name = "#{form.object_name}[#{attrib.id}][value]"
+    #attrib.html_options = { class: 'real-field', autocomplete: 'off', name: name }


Please ignore this part, I was doing some testing on submitting the form parameters as a hash to avoid the field_property notation used for exclude/min/max/fixed

johrstrom · 2023-09-20T13:22:55Z

@johrstrom what is your idea for the UI?

Probably a checkbox and we can use the currently selected/input value as the value we fix to (saying the same in a help message).

And for the backend? Should the fixed property be use for this feature?

Yes - though I think it's just a boolean.

johrstrom · 2023-09-20T13:26:00Z

@johrstrom what is your idea for the UI?

Probably a checkbox and we can use the currently selected/input value as the value we fix to (saying the same in a help message).

Checking your demo in the other ticket - the UI you show is about what I was thinking.

johrstrom · 2023-09-20T15:17:36Z

apps/dashboard/app/controllers/scripts_controller.rb

+    :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_min, :bc_num_slots_max,
    :bc_num_hours, :bc_num_hours_min, :bc_num_hours_max


We can start with fixing bc_num_slots and bc_num_hours instead if it's easier to start with number fields instead of select fields.

johrstrom · 2023-09-20T15:20:58Z

This PR shows what I had to do to make it work with SmartAttributes and the fixed property, but it is more a hack than a PR.

I can start to comment on this if you're ready. I think yes, we should use the fixed API and as to the UI - if it's clunky now we can fix it up later, there's a lot of UI related work to do, I'm mostly interested in functionality for the initial release.

abujeda · 2023-09-20T16:03:21Z

OK - thanks. Let me improve a bit the implementation and then it will bre ready for review

abujeda · 2023-09-21T14:52:49Z

@johrstrom this is ready for a review

apps/dashboard/app/helpers/batch_connect/session_contexts_helper.rb

apps/dashboard/app/helpers/scripts_helper.rb

abujeda · 2023-09-22T16:49:34Z

I have improve a bit the code, added the comment and added a check in the jobs_app_test

apps/dashboard/app/views/scripts/editable_form_fields/_editable_number.html.erb

apps/dashboard/app/controllers/scripts_controller.rb

apps/dashboard/app/models/script.rb

abujeda · 2023-09-25T11:55:12Z

I have implemented the changes. This is the flow with fixed values now:

apps/dashboard/app/javascript/packs/script_edit.js

apps/dashboard/config/locales/en.yml

apps/dashboard/test/system/jobs_app_test.rb

johrstrom · 2023-09-26T18:55:40Z

Tests are currently failing for another reason - I'm fixing that shortly.

johrstrom · 2023-09-26T18:59:27Z

apps/dashboard/app/javascript/packs/script_edit.js

+}
+
+function toggleFixed(event) {
+  event.target.disabled = true;


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

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.

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.

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

johrstrom · 2023-09-26T19:00:29Z

apps/dashboard/app/javascript/packs/script_edit.js

+  dataElement.disabled = true;
+  const input = $('<input>').attr('type','hidden').attr('name', dataElement.name).attr('value', dataElement.value);
+  $(checkbox).after(input);


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

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

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.

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.

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?

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

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.

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.

johrstrom

LGTM, thanks!

osc-bot added the component/dashboard label Sep 20, 2023

abujeda commented Sep 20, 2023

View reviewed changes

johrstrom reviewed Sep 20, 2023

View reviewed changes

johrstrom reviewed Sep 21, 2023

View reviewed changes

apps/dashboard/app/helpers/batch_connect/session_contexts_helper.rb Show resolved Hide resolved

abujeda force-pushed the 2948_static_values branch 2 times, most recently from c1e2ecd to 2127fc1 Compare September 22, 2023 10:19

abujeda commented Sep 22, 2023

View reviewed changes

apps/dashboard/app/helpers/scripts_helper.rb Show resolved Hide resolved

abujeda force-pushed the 2948_static_values branch from 2127fc1 to c17ce11 Compare September 22, 2023 16:47

johrstrom reviewed Sep 22, 2023

View reviewed changes

apps/dashboard/app/views/scripts/editable_form_fields/_editable_number.html.erb Outdated Show resolved Hide resolved

johrstrom reviewed Sep 22, 2023

View reviewed changes

apps/dashboard/app/controllers/scripts_controller.rb Outdated Show resolved Hide resolved

johrstrom reviewed Sep 22, 2023

View reviewed changes

apps/dashboard/app/models/script.rb Outdated Show resolved Hide resolved

abujeda added 2 commits September 25, 2023 16:40

Added support for static values to scripts forms

3b233d7

Improved implementation for fixed values in Script forms

6e0f2b6

abujeda force-pushed the 2948_static_values branch from 5900c45 to 6e0f2b6 Compare September 25, 2023 15:40

johrstrom closed this Sep 26, 2023

johrstrom reopened this Sep 26, 2023

johrstrom reviewed Sep 26, 2023

View reviewed changes

apps/dashboard/app/javascript/packs/script_edit.js Outdated Show resolved Hide resolved

johrstrom reviewed Sep 26, 2023

View reviewed changes

apps/dashboard/config/locales/en.yml Outdated Show resolved Hide resolved

johrstrom reviewed Sep 26, 2023

View reviewed changes

apps/dashboard/test/system/jobs_app_test.rb Show resolved Hide resolved

Improved script edit JS for fixed fields

03656c3

johrstrom reviewed Sep 26, 2023

View reviewed changes

Improved comments for fixed fields in edit script JS

e5187a6

johrstrom self-requested a review September 27, 2023 19:16

johrstrom approved these changes Sep 27, 2023

View reviewed changes

johrstrom merged commit b24a37b into OSC:master Sep 27, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functional prototype for static values in Scripts #3059

Functional prototype for static values in Scripts #3059

abujeda commented Sep 20, 2023

abujeda Sep 20, 2023

johrstrom commented Sep 20, 2023

johrstrom commented Sep 20, 2023

johrstrom Sep 20, 2023

johrstrom commented Sep 20, 2023

abujeda commented Sep 20, 2023

abujeda commented Sep 21, 2023

abujeda commented Sep 22, 2023

abujeda commented Sep 25, 2023

johrstrom commented Sep 26, 2023

johrstrom Sep 26, 2023

abujeda Sep 26, 2023

johrstrom Sep 26, 2023

abujeda Sep 26, 2023

johrstrom Sep 26, 2023

abujeda Sep 26, 2023 •

edited

Loading

johrstrom Sep 26, 2023

johrstrom Sep 26, 2023

johrstrom Sep 26, 2023

abujeda Sep 26, 2023

abujeda Sep 26, 2023

johrstrom left a comment

Functional prototype for static values in Scripts #3059

Functional prototype for static values in Scripts #3059

Conversation

abujeda commented Sep 20, 2023

Choose a reason for hiding this comment

johrstrom commented Sep 20, 2023

johrstrom commented Sep 20, 2023

Choose a reason for hiding this comment

johrstrom commented Sep 20, 2023

abujeda commented Sep 20, 2023

abujeda commented Sep 21, 2023

abujeda commented Sep 22, 2023

abujeda commented Sep 25, 2023

johrstrom commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abujeda Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johrstrom left a comment

Choose a reason for hiding this comment

abujeda Sep 26, 2023 •

edited

Loading