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

Allow end user to select a choice different from list of available choices in profile_list #735

Merged
merged 39 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
53f98f0
Refs #699 - allow user to specify free text image location. This commit:
batpad May 18, 2023
4cfe73e
use free_form object for configuration, handle frontend validation wi…
batpad May 18, 2023
62fb537
renames: free_form -> other_choice and match_regex -> validation_matc…
batpad May 20, 2023
f76918c
slightly messy but working implementation
batpad Jun 23, 2023
08dd78d
slightly ugly hack to ensure un-needed fields are not submitted with …
batpad Jun 23, 2023
f6ee8f8
add tests for handling choices and other-choice
batpad Jun 27, 2023
b09ce98
add validation for other-choice by checking against the validation re…
batpad Jun 27, 2023
cf4ed30
add some comments to the code
batpad Jun 27, 2023
2f64ab5
rename validation_match_regex to validation_regex
batpad Jun 28, 2023
29f5df5
convert -other-choice to --other-choice in form submission to prevent…
batpad Jun 28, 2023
eabd145
change value of other to other-choice in select dropdown
batpad Jun 28, 2023
4ae4009
fix error message for POST
batpad Jun 28, 2023
bfd51d4
fix test
batpad Jun 28, 2023
ed4c9ff
Merge branch 'allow-other' of github.com:batpad/kubespawner into allo…
batpad Jun 28, 2023
16f16b5
fix validation regex in config, clarify doc
batpad Jun 28, 2023
b9c74e0
fix bug where free form input would not show if Other was selected wh…
batpad Jun 28, 2023
872ad3e
allow validation_regex to be optional, add test
batpad Jun 28, 2023
930d838
Address PR feedback from @yuvipanda:
batpad Jul 1, 2023
f55e323
make other_choice_form_key a variable
batpad Jul 1, 2023
d15e3bf
Rename other-choice to unlisted-choice
yuvipanda Jul 12, 2023
116ad0c
Move options validation code into a function
yuvipanda Jul 12, 2023
68a71f9
Format with black
yuvipanda Jul 12, 2023
07fc3a9
Fix typo
yuvipanda Jul 12, 2023
6862aa0
Explicitly populate defaults of profile_list
yuvipanda Jul 19, 2023
a47f67e
Move code to get a profile from a slug into own function
yuvipanda Jul 19, 2023
36db652
Stop using self._profile_list
yuvipanda Jul 19, 2023
59507de
Document why the options_form default is the way it is
yuvipanda Jul 19, 2023
b1dba27
Remove extra call to _populate_profile_list_defaults
yuvipanda Jul 19, 2023
1763083
Clarify that the profile_list function is idempotent
yuvipanda Jul 19, 2023
3534920
Split code that applies overrides to its own function
yuvipanda Jul 19, 2023
6eb6e8a
Remove extra comment
yuvipanda Jul 19, 2023
96067e8
Actually write comment explaining default choice selection
yuvipanda Jul 19, 2023
cea4107
Reduce extra complexity in choosing default choice
yuvipanda Jul 19, 2023
dcbd152
Fix find profile by slug tests
yuvipanda Jul 19, 2023
783f240
Run pre-commit
yuvipanda Jul 19, 2023
a88f875
Add missing await to async function call
yuvipanda Jul 19, 2023
ff4216c
Handle empty profile_list
yuvipanda Jul 19, 2023
e8e8ceb
Handle profile_list being a function in a single place
yuvipanda Jul 19, 2023
243a5d7
Handle case of slug being None
yuvipanda Jul 19, 2023
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
7 changes: 7 additions & 0 deletions jupyterhub_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@
'profile_options': {
'image': {
'display_name': 'Image',
'unlisted_choice': {
'enabled': True,
'display_name': 'Image Location',
'validation_regex': '^pangeo/.*$',
'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$',
'kubespawner_override': {'image': '{value}'},
},
'choices': {
'pytorch': {
'display_name': 'Python 3 Training Notebook',
Expand Down
105 changes: 86 additions & 19 deletions kubespawner/spawner.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import asyncio
import ipaddress
import os
import re
import string
import sys
import warnings
Expand Down Expand Up @@ -1537,6 +1538,17 @@ def _validate_image_pull_secrets(self, proposal):
and the value is a dictionary with the following keys:

- `display_name`: Name used to identify this particular option
- `unlisted_choice`: Object to specify if there should be a free-form field if the user
selected "Other" as a choice:
- `enabled`: Boolean, whether the free form input should be enabled
- `display_name`: String, label for input field
- `validation_regex`: Optional, regex that the free form input should match - eg. ^pangeo/.*$
- `validation_message`: Optional, validation message for the regex. Should describe the required
input format in a human-readable way.
- `kubespawner_override`: Object specifying what key:values should be over-ridden
batpad marked this conversation as resolved.
Show resolved Hide resolved
with the value of the free form input, using `{value}` for the value to be substituted with
the user POSTed value in the `unlisted_choice` input field. eg:
- some_config_key: some_value-with-{value}-substituted-with-what-user-wrote
- `choices`: A dictionary containing list of choices for the user to choose from
to set the value for this particular option. The key is an identifier for this
choice, and the value is a dictionary with the following possible keys:
Expand Down Expand Up @@ -1573,6 +1585,15 @@ def _validate_image_pull_secrets(self, proposal):
'profile_options': {
'image': {
'display_name': 'Image',
'unlisted_choice': {
'enabled': true,
'display_name': 'Image Location',
'validation_regex': '^pangeo/.*$',
'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$',
'kubespawner_override': {
'image': '{value}'
}
},
'choices': {
'pytorch': {
'display_name': 'Python 3 Training Notebook',
Expand Down Expand Up @@ -2991,6 +3012,48 @@ def _options_from_form(self, formdata):

return options

def _validate_posted_profile_options(self, profile, selected_options):
"""
Validate posted user options against the selected profile

The default form is rendered in such a way that each option specified in
the profile *must* have a value in the POST body. Extra options in the
POST body are ignored. We only honor options that are defined
in the selected profile *and* are in the form data
posted. This prevents users who may be authorized to only use
one profile from being able to access options set for other
profiles
"""
for option_name, option in profile.get('profile_options').items():
unlisted_choice_form_key = f'{option_name}--unlisted-choice'
if option_name not in selected_options:
# unlisted_choice is enabled:
if option.get('unlisted_choice', {}).get('enabled', False):
if unlisted_choice_form_key not in selected_options:
raise ValueError(
f'Expected option {option_name} for profile {profile["slug"]} or {unlisted_choice_form_key}, not found in posted form'
)
unlisted_choice = selected_options[unlisted_choice_form_key]

# Validate value of 'unlisted_choice' against validation regex
if profile.get('profile_options')[option_name][
'unlisted_choice'
].get('validation_regex', False):
unlisted_choice_validation_regex = profile.get(
'profile_options'
)[option_name]['unlisted_choice']['validation_regex']
if not re.match(
unlisted_choice_validation_regex, unlisted_choice
):
raise ValueError(
f'Value of {unlisted_choice_form_key} does not match validation regex.'
)
# unlisted_choice is Disabled
else:
raise ValueError(
f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form'
)

async def _load_profile(self, slug, selected_profile_user_options):
"""Load a profile by name

Expand Down Expand Up @@ -3041,24 +3104,12 @@ async def _load_profile(self, slug, selected_profile_user_options):
setattr(self, k, v)

if profile.get('profile_options'):
# each option specified here *must* have a value in our POST, as we
# render our HTML such that there's always something selected.

# We only honor options that are defined in the selected profile *and*
# are in the form data posted. This prevents users who may be authorized
# to only use one profile from being able to access options set for other
# profiles
for user_selected_option_name in selected_profile_user_options.keys():
if (
user_selected_option_name
not in profile.get('profile_options').keys()
):
raise ValueError(
f'Expected option {user_selected_option_name} for profile {profile["slug"]}, not found in posted form'
)

self._validate_posted_profile_options(
profile, selected_profile_user_options
)
# Get selected options or default to the first option if none is passed
for option_name, option in profile.get('profile_options').items():
unlisted_choice_form_key = f'{option_name}--unlisted-choice'
chosen_option = selected_profile_user_options.get(option_name, None)
# If none was selected get the default
if not chosen_option:
Expand All @@ -3069,9 +3120,25 @@ async def _load_profile(self, slug, selected_profile_user_options):
default_option = choice_name
chosen_option = default_option

chosen_option_overrides = option['choices'][chosen_option][
'kubespawner_override'
]
# Handle override for unlisted_choice free text specified by user
if (
option.get('unlisted_choice', {}).get('enabled', False)
and unlisted_choice_form_key in selected_profile_user_options
):
chosen_option_overrides = option['unlisted_choice'][
'kubespawner_override'
]
for k, v in chosen_option_overrides.items():
chosen_option_overrides[k] = v.format(
value=selected_profile_user_options[
unlisted_choice_form_key
]
)
else:
chosen_option_overrides = option['choices'][chosen_option][
'kubespawner_override'
]

for k, v in chosen_option_overrides.items():
if callable(v):
v = await maybe_future(v(self))
Expand Down
72 changes: 63 additions & 9 deletions kubespawner/templates/form.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,33 @@ <h3>{{ profile.display_name }}</h3>
{%- if profile.profile_options %}
<div>
{%- for k, option in profile.profile_options.items() %}
<div class="option">
<label for="profile-option-{{ profile.slug }}-{{ k }}"
class="js-profile-option-label">{{ option.display_name }}</label>
<select name="profile-option-{{ profile.slug }}-{{ k }}"
class="form-control js-profile-option-select">
{%- for k, choice in option['choices'].items() %}
<option value="{{ k }}" {% if choice.default %}selected{% endif %}>{{ choice.display_name }}</option>
{%- endfor %}
</select>
<div class="js-options-container">
<div class="option">
<label for="profile-option-{{ profile.slug }}-{{ k }}"
class="js-profile-option-label">{{ option.display_name }}</label>
<select name="profile-option-{{ profile.slug }}-{{ k }}"
class="form-control js-profile-option-select">
{%- for k, choice in option['choices'].items() %}
<option value="{{ k }}" {% if choice.default %}selected{% endif %}>{{ choice.display_name }}</option>
{%- endfor %}
{%- if option['unlisted_choice'] and option['unlisted_choice']['enabled'] %}
<option value="unlisted-choice">Other...</option>
{%- endif %}
</select>
</div>
{%- if option['unlisted_choice'] and option['unlisted_choice']['enabled'] %}
<div class="option hidden js-other-input-container">
<label for="profile-option-{{ profile.slug }}-{{ k }}--unlisted-choice">
{{ option['unlisted_choice']['display_name'] }}
</label>
<input data-name="profile-option-{{ profile.slug }}-{{ k }}--unlisted-choice"
{%- if option['unlisted_choice']['validation_regex'] %}
pattern="{{ option['unlisted_choice']['validation_regex'] }}"
{%- endif %}
title="{{ option['unlisted_choice']['validation_message'] }}"
class="form-control js-other-input" />
</div>
{%- endif %}
</div>
{%- endfor %}
</div>
Expand All @@ -41,4 +59,40 @@ <h3>{{ profile.display_name }}</h3>
.find('input[type=radio]')
.prop('checked', true);
});

// If the user selects "Other" in the dropdown, show the free text field
$('.js-profile-option-select').change(function() {
const selectionValue = $(this).val();
const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container');
const $otherInput = $(this).parents('.js-options-container').find('.js-other-input');
if (selectionValue === 'unlisted-choice') {

// if the Select box is selected with a value that is not 'unlisted-choice',
// we remove the "name" attribute for the unlisted-choice input
// and vice-versa, so that we only submit valid form values
// and don't leave the complexity of figuring out which value
// to use to the back-end.

// This can probably be done cleaner by completely refactoring
// how we send values from the frontend.
$(this).data('name', $(this).attr('name'));
$(this).removeAttr('name');
$otherInput.attr('name', $otherInput.data('name'));
$otherInputContainer.removeClass('hidden');
} else {
$otherInput.removeAttr('name');
$(this).attr('name', $(this).data('name'));
$otherInputContainer.addClass('hidden');
}
});

// wrapping in a document.ready to make clear this is executed once
// on page load
$(document).ready(() => {
// trigger the `change` event on the select inputs,
// so that if "Other" is selected on page load, the corresponding
// free-text input shows
$('.js-profile-option-select').trigger('change');
});

</script>
Loading