Set default values for min/max handler #4627

ahmed-mgd · 2025-09-24T15:07:40Z

johrstrom · 2025-09-24T15:36:05Z

apps/dashboard/app/javascript/dynamic_forms.js

+    } else {
+      const defaultVal = dim == 'min' ? 1 : 999;
+      changeElement.attr(dim, defaultVal);


I love the precision here in making just a small enough change, but sadly, I feel like it's not enough.

It seems to me that we should embed the min or max to fall back on. Take this for example, we should fall back to these values if they're defined.

some_element: widget: number_field min: 2 max: 4

If we can't fallback to any values, then we just don't. I think the conservative route of just doing nothing is better than choosing 1 or 999. But we can document that these are fallback values when you use data-min or data-max sparingly.

johrstrom · 2025-10-29T18:40:26Z

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

+      :div, 
+      id: [form.object_name, attrib.id, 'wrapper'].join('_'), 
+      class: attrib.hide_by_default? ? 'd-none' : '',
+      data: data


Seems like we need should have some sort of helper function as a member function of the smart attribute to generate this hash. That way it can deal with all the edge cases and it encapsulates that logic within that object.

@Bubballoo3 FYI - let me know if a member function on the smart attribute parent class is to your liking. I think it makes sense there as it'll likely need all sorts of toggles based on the widget type.

Yeah I think that would be good. We should try to keep this as small as possible though

If it were me, I'd likely put it all in a module that you include so that it's all encapsulated in one single file. In that way, it won't be mixed in with other functionality when you try to read and understand it. It's one file that serves one purpose, even if it has many lines. But that's just me.

Wouldn't we only want a module if we need to use it elsewhere in ruby code? I might be misunderstanding; are you talking about just the data hash or would it do more?

Wouldn't we only want a module if we need to use it elsewhere in ruby code? I might be misunderstanding; are you talking about just the data hash or would it do more?
React

No it wouldn't do more, but my thinking is it may be nice to just to have all this functionality & logic contained in 1 file instead of being spread out amongst all the other functionality and logic in the existing file.

It's not about reuse it's more about separating functionality.

Bubballoo3 · 2025-10-29T21:21:41Z

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

+      :div, 
+      id: [form.object_name, attrib.id, 'wrapper'].join('_'), 
+      class: attrib.hide_by_default? ? 'd-none' : '',
+      data: data


Yeah I think that would be good. We should try to keep this as small as possible though

Bubballoo3 · 2025-10-29T21:37:02Z

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

+    min_val = attrib.opts[:min]
+    max_val = attrib.opts[:max]
+    data[:min_default] = min_val if min_val
+    data[:max_default] = max_val if max_val


I think storing the min/max here might add a bit more complexity than we need, as it requires us to keep conventions between server-side and client-side code, which is difficult as we can't share any methods between ruby and javascript. We can expand this initial hash whenever necessary ofc, but things like mins and maxes shouldn't need to be collected at this stage since we know the widget will be ok when it renders. IMO it would be much easier to manage if all of this was handled in dynamic_forms.js, and we load the default values into the data hash whenever we bind the min/max-for listener. This also prevents us from expanding the data hash for static widgets (without min/max-for directives), where this info is unnecessary bloat.

Just to justify the existing items under this criteria, widget_type is necessary because we don't have any other way of connecting the complex form items (like resolution_fields) to a meaningful name. The hide_by_default attribute is less necessary admittedly, but ok because we absolutely have to check hide_by_default? when we add the d-none class, and it is simpler to just get it from the attrib object here than detect it from the d-none class later.

Let me know what you guys think of this, obviously the wrapper is new so we could go several ways with what it becomes, and this current approach does reduce complexity in dynamic_forms, which is definitely already complex enough. Overall though I think trying to avoid creating conventions that have to be followed in two places will save us some headache as we use this more.

We can expand this initial hash whenever necessary ofc, but things like mins and maxes shouldn't need to be collected at this stage since we know the widget will be ok when it renders. IMO it would be much easier to manage if all of this was handled in dynamic_forms.js, and we load the default values into the data hash whenever we bind the min/max-for listener.

I don't think I follow you. It needs to be rendered in the HTML on the server side somewhere at some point. But I could be completely missing what you're trying to say.

I am saying that unless the data information needs to be rendered server-side for some reason, we should store and retrieve that data exclusively in the javascript, just to keep that storing and retrieval in a single place. Since we can get the min and max information from the html post-rendering, this would be a place where we could just as effectively gather and store min_default and max_default in addMinMaxForHandler and set the element's data in the same way, allowing us to only store data on widgets that we are going to change dynamically later, and keep min/max-for functionality entirely withing dynamic_forms.js.

Oh I think I got you - because mins and maxes will be in the HTML before any of our javascript runs that's when & where we can extract it. I.e., the defaults are there before we modify them when our handlers run. Is that it?

Yeah. And even though the resulting data attribute will be the same by the time we start changing the mins/maxes, we should still try and set this at the latest possible moment, since that is the closest to when the data will be used/accessed (same principle as initializing a variable close to where it's used). The most immediate impact is reducing the splash radius of this change to only dynamic_forms.js and not having to touch session_contexts_helper.rb

This reverts commit 9f28346.

Bubballoo3 · 2025-11-07T21:07:39Z

apps/dashboard/app/javascript/dynamic_forms.js

+  if (!wrapper) return;
+
+  ['max', 'min'].forEach((dim) => {
+    const defaultVal = element.getAttribute(dim);


I like the structure of this method and its error resistance, but I am not sure why you pass every form element through here when most do not have min/max defined. IMO it makes more sense to call this method in addMinMaxForHandlers so that

Every element you try to store should have a max and min

You only store elements that might be changed in the future

Since addMinMaxForHandlers calls toggleMinMax at the very end, it ends up changing the default values for the "second" element. If/when we iterate to that second element later, we don't store the actual defaults for that element but those mutated values. I'm not sure if there's an easy way to work around this.

Just to use the language from the method to make this a little more clear, I'll refer to the subject (the element with the directive) and the object (element the directive modifies). In this case the object is the only thing we need to store data on, since that is the element that toggleMinMax is going to change. The secondEle variable refers to a second subject, so in the case of data-max-num-cores-for-cluster-oakley: 28 the object is num-cores and the secondEle is cluster. So we assign a second event listener to cluster, but that listener can still only modify num-cores. So as long as you storeDefaults(num-cores) before those change handlers are added, I think it should all work out. If you tried it and it didn't work, or you have a specific example in mind that I am not considering, I'd definitely be open to discussing and getting to the bottom of it.

Oh I see what you are talking about now, and the easiest solution is to only collect the min/maxDefault one time for each element. So inside here you just need to check that wrapper.dataset['${dim}Default'] === undefined before wrapper.dataset['${dim}Default'] = n;

ahmed-mgd · 2025-11-07T22:12:00Z

It looks like some test cases are still failing since it expects different min/max values now. I'll look into those.

Bubballoo3 · 2025-11-07T22:51:52Z

Just a quick note on the tests, everything with visit new_batch_connect_session_context_url('sys/bc_jupyter') uses the form fixture https://github.com/OSC/ondemand/blob/0b26bcc08c110b1ecfbb8429fc6af772804b3a57/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/form.yml and you can throw that directly into a dev app to manually work through the tests. You may have already figured that out, but I remember being kind of thrown trying to connect the fixtures to the app names as there are several bc_jupyters in the fixtures.

johrstrom · 2025-11-13T17:41:33Z

the latest update I think is good on the surface - but there seems to be an issue where multiple events are firing and while it does change the min & max correctly when the first event fires - subsequent events overwrite this. Not sure at a glance/off the top of my head to solve for this in a nice way.

moffatsadeghi · 2025-11-21T20:07:55Z

the latest update I think is good on the surface - but there seems to be an issue where multiple events are firing and while it does change the min & max correctly when the first event fires - subsequent events overwrite this. Not sure at a glance/off the top of my head to solve for this in a nice way.

@ahmed-mgd we are wrapping up 4.1 development here soon. What do you need from us to help you with this issue? At a quick glance, this PR shows a lengthy, complex discussion.

ahmed-mgd · 2025-11-21T20:22:33Z

@ahmed-mgd we are wrapping up 4.1 development here soon. What do you need from us to help you with this issue? At a quick glance, this PR shows a lengthy, complex discussion.

We resolved most of the problems in this discussion and now I just need to work out a quirk with the toggleMinMax function. I've kinda put this off, but with the code freeze coming up I'll prioritize this ticket since it's dragged on.

Set default values for min and max

2cb2fed

github-project-automation bot added this to PR Review Pipeline Sep 24, 2025

github-project-automation bot moved this to Awaiting Review in PR Review Pipeline Sep 24, 2025

osc-bot added the component/dashboard label Sep 24, 2025

johrstrom reviewed Sep 24, 2025

View reviewed changes

Bubballoo3 moved this from Awaiting Review to Changes Requested in PR Review Pipeline Oct 2, 2025

Bubballoo3 mentioned this pull request Oct 24, 2025

data-label undefined when initial is not set #4595

Open

ahmed-mgd added 3 commits October 29, 2025 09:05

Revert default value fallback

74793dd

Merge branch 'master' into min-max-handlers-1446

9d847ca

Fall back on default min/max values from wrapper

9f28346

johrstrom reviewed Oct 29, 2025

View reviewed changes

Bubballoo3 reviewed Oct 29, 2025

View reviewed changes

ahmed-mgd added 3 commits November 5, 2025 13:51

Revert "Fall back on default min/max values from wrapper"

98ac95b

This reverts commit 9f28346.

Get default values before attaching form handlers

fe20726

Remove debug log

0b26bcc

Bubballoo3 requested changes Nov 7, 2025

View reviewed changes

Set default values for min/max handler #4627

Are you sure you want to change the base?

Set default values for min/max handler #4627

Uh oh!

Conversation

ahmed-mgd commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubballoo3 Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubballoo3 Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubballoo3 Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubballoo3 Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmed-mgd commented Nov 7, 2025

Uh oh!

Bubballoo3 commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johrstrom commented Nov 13, 2025

Uh oh!

moffatsadeghi commented Nov 21, 2025

Uh oh!

ahmed-mgd commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Bubballoo3 Oct 31, 2025 •

edited

Loading

Bubballoo3 Oct 30, 2025 •

edited

Loading

Bubballoo3 Oct 30, 2025 •

edited

Loading

Bubballoo3 Nov 7, 2025 •

edited

Loading

Bubballoo3 commented Nov 7, 2025 •

edited

Loading