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

Refs #756: Handle no choices provided if unlisted_choice is present #778

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

batpad
Copy link
Contributor

@batpad batpad commented Sep 2, 2023

Allows choices to be empty if unlisted_choice is present - handles hiding the select dropdown if choices is empty.

Because of the toggling we do for the hidden / inactive state of the unlisted_choice input, some of the frontend code is a bit awkward. I can think about this, but I think a more elegant solution might require a more complete refactor / moving to a more JS-based approach of handling options and rendering.

I think this works, and I tried to add a basic test for this case, but I do worry a bit about edge cases - this probably needs a bit of thorough testing.

cc @consideRatio @yuvipanda @GeorgianaElena

@consideRatio
Copy link
Member

consideRatio commented Sep 2, 2023

The option["display_name"] is no longer used when choices is unset. Not sure what to do about this, but it should be documented and the test cases / documented examples using unlisted_choice without choices shouldn't configure it.

        'profile_options': {
            'image': {
                'display_name': 'Image',
                'unlisted_choice': {
                    'enabled': True,
                    'display_name': 'Image Location',
                    'kubespawner_override': {'image': '{value}'},
                }
            },
        },

The key question in my mind is about handling what it means to submit an unlisted_choice without providing a value. I'm not sure what I think should be allowed and should happen.

Hmm.... In an example like above where image is configured, it really doesn't make sense for a submission to be allowed - but that is the responsibility of the admin configuring this I think.

The question becomes if kubespawner_override ever allow a blank submission of an unlisted_choice, and when that happens, what should be expected to happen?

Scenario

Consider if admin admin wants to default to jupyter/base-notebook:latest, unless the user specifies something else via unlisted_choice specifically.

I can see that be relevant. How would we go about supporting that? I think the options are:

  1. let unlisted_choice have a default value
  2. let kubespawner do nothing if an empty unlisted_choice value is submitted, meaning that the unlisted_choice.kubespawner_override isn't applied when that happens. Then, the default image could be configured on a higher level, and be overridden if the user has specified something.

I'm not sure what I think works out best. I think currently, we the behavior corresponds to the second option.

@batpad
Copy link
Contributor Author

batpad commented Sep 2, 2023

The option["display_name"] is no longer used when choices is unset. Not sure what to do about this, but it should be documented and the test cases / documented examples using unlisted_choice without choices shouldn't configure it.

Good catch.

Consider if admin admin wants to default to jupyter/base-notebook:latest, unless the user specifies something else via unlisted_choice specifically.

Hmmm. What if we:

  • Make the unlisted_choice text field a required field if either there are no choices or a user selects "Other" in the drop-down and leaves the input blank.
  • Allow a default value for unlisted_choice which we use to pre-populate the unlisted_choice field

Admittedly, I've not fully thought this through, but that seems like it might be expected user behaviour, and if we expect the field to have a value, we should just validate that on the frontend?

@consideRatio
Copy link
Member

consideRatio commented Sep 4, 2023

Implementation proposal

I suggest we add unlisted_choice.default_value (string / Unicode) config and how it work like this.

  1. When unlisted_choice.default_value is None
    • require a value in the frontend and backend
      • frontend: add HTML attribute required to the input tag
      • backend: update logic
  2. When unlisted_choice.default_value is not None
    • don't require a value in the frontend
    • add a HTML attribute placeholder=<the default value> to the input tag
    • use the default value when needed
      • the unlisted_choice.default_value should only be used if there are no other pre-defined choices, or if there are other pre-defined choices but unlisted_choice was explicitly chosen in the dropdown list

Motivation

Let's consider these three situations with only unlisted_choice used.

  1. users are required to specify something
    • Python config: unlisted_choice.default_value is None
    • HTML form: the required attribute is added
  2. users are not required to specify something, a default value is used
    • Python config: unlisted_choice.default_value is not None
    • HTML form: placeholder=<default value> attribute is added
  3. users are not required to specify something, nothing should happen
    • Python config: I'm not sure how a user should configure this.
    • HTML form: no extra attributes.

Are there use cases for all of these situations strong enought to motivate the added complexity of supporting them? I think yes for situation 1 and 2, but for situation 3? I don't think so currently.

If a jupyterhub admin provides a way to configure something via an unlisted_choice, they know what that something they want to configure is. Knowing what that something is, it will be fine in almost all situations to make them to fall back to configuring an explicit default value instead of doing nothing. The edge case I can imagine is that you may want to not explicitly configure something as a default.

I'm more concerned about providing an implementation to support situation 3 now than not. If we do it, we add complexity for a hypotetical use case, not just for maintainers, but for users that needs to understand our docs. Due to this, I propose we disregard situation 3 for now.

Let's finally consider the unlisted_choice.default_value interactions with choices configured:

  1. choices a (default=True), b, and unlisted_choice (default_value="yes")
    • Expectation: the a choice is the explicit default choice, so the unlisted_choice's default value should be adopted only if the "Other..." choice was selected.
  2. choices a, b, and unlisted_choice (default_value="yes")
    • Expectation: the a choice is the implicit default choice, so the unlisted_choice's default value should be adopted only if the "Other..." choice was selected.otherwise the a choice should be the implicit default.

@batpad
Copy link
Contributor Author

batpad commented Sep 5, 2023

@consideRatio this all sounds good.

Am afraid that I may not have the capacity to take this on this month :( - I thought I could get it done, but am at a team week this week and then vacation for 2 weeks.

Just don't want to hold up a release, etc. - if someone else is able to take this on, that's amazing - else I can definitely come back to this toward the end of the month.

Big apologies!

@yuvipanda
Copy link
Collaborator

yuvipanda commented Sep 5, 2023

My suggestion @consideRatio is to not block 6.1.0 release on this, and let @batpad come back to this after his vacation. I'm hopeful to grow contributor base :)

@batpad
Copy link
Contributor Author

batpad commented Oct 31, 2023

Trying to pick this up:

I forget a bit larger context here. But from the comment above, it seems like the idea is to add an unlisted_choice.default_value configuration option, with the behaviour described in your note above @consideRatio .

I'm a bit worried about what this might involve in terms of backend validation, but I can find some time to work on this this week.

@yuvipanda @consideRatio do you know / remember if there's anything else to do here to close this out?

Thank you!

@yuvipanda
Copy link
Collaborator

Got any more time to work on this, @batpad?

@batpad batpad force-pushed the handle-no-choices branch from 9efac4f to b829054 Compare January 9, 2024 04:49
@batpad
Copy link
Contributor Author

batpad commented Jan 9, 2024

@yuvipanda - this PR should now correctly hide the Select drop-down if there is only an unlisted_choice and no choices specified. It probably needs a bit of testing, but this should "work" to visually hide that drop-down if there is only an unlisted_choice .

What this does not add yet is the default_value handling per comments above. I feel like I'm not able to fully make sense of the trade-offs of not including that or including that as a separate follow-up PR. That does seem like a bit more involved work including backend with tests - I could dig into it but would really much rather prioritize the much-needed larger refactor of this frontend code. Happy to chat!

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

Successfully merging this pull request may close these issues.

3 participants