Allow dropdown text for unlisted choice to be configurable

[batpad]

PR Summary

The profile's in profile_list can provide profile_options, for example to choose a pre-defined image from a list or opt to specify an unlisted_choice - not part of this list. This pr adds the unlisted_choice.display_name_in_choices configuration to specify how the option/choice added to the list of pre-defined entries should be presented.

Before, it looked like:

After, it can be configured to look like:

---

cc @yuvipanda @consideRatio @GeorgianaElena

#757 should be a bit more straightforward than #756, so made this a separate smaller PR, correctly updated with latest main.

Here we just add an option to the unlisted_choice options dictionary and output that in the select instead of Other... if present.

I have called that option other_text, but that doesn't sound great to me, so if anyone has a better recommendation for naming, please let know / go ahead and make the change.

I'm not sure if I missed adding documentation for this new option somewhere.

Many apologies for the delay getting to this, please let me know if this looks fine. I'll make a separate PR toward #756.

Thank you!

[consideRatio]

Wieeee this looks nice!

Config naming

There is a coupling between the new config option and choices.*.display_name and I'd love if that is captured directly by the naming of the config. In words, I'm describing the option as "the display_name of an entry added to choices to choose an unlisted choice". That makes me lean towards a naming like:

• display_name_in_choices
• choices_entry_display_name

I think it sounds fine with display_name_in_choices.

For reference, this is how the config may look

                    'profile_options': {
                        'image': {
                            'display_name': 'Image',
                            'choices': {
                                'base': {
                                    'display_name': 'jupyter/base-notebook:latest',
                                    'kubespawner_override': {
                                        'image': 'jupyter/base-notebook:latest'
                                    },
                                },
                                'minimal': {
                                    'display_name': 'jupyter/minimal-notebook:latest',
                                    'default': True,
                                    'kubespawner_override': {
                                        'image': 'jupyter/minimal-notebook:latest'
                                    },
                                },
                            },
                            'unlisted_choice': {
                                'enabled': True,
                                'display_name': 'Other image',
                                'display_name_in_choices': 'Enter image manually',
                                'validation_regex': '^jupyter/.+:.+$',
                                'validation_message': 'Must be an image matching ^jupyter/<name>:<tag>$',
                                'kubespawner_override': {'image': '{value}'},
                            },
                        },
                    },

Initialized profile_list

Maybe we should reduce complexity like below in the jinja2 template parsing a profile_list variable by initializing profile_list more thoroughly in _get_initialized_profile_list.

                        {%- if option['unlisted_choice']['other_text'] %}
                          {{ option['unlisted_choice']['other_text'] }}
                        {%- else %}
                          Other...
                        {%- endif %}

If we in _get_initialized_profile_list provide a default for each profile_options entry with unlisted_choice.enabled=False and unlisted_choice.display_name_in_choices="Other..." we would be able to reduce the complexity quite a bit.

                        {{ option['unlisted_choice']['other_text'] }}

[consideRatio]

(I fixed a mixup between #756 and #757 in the title and PR description)

[batpad]

(I fixed a mixup between #756 and #757 in the title and PR description)

Yikes, so sorry about that, and thanks for fixing. Apologies the branch is still named 756-custom-other-text - let me know if you would like me to make a new PR with a new branch name there.

I think it sounds fine with display_name_in_choices

Sounds good to me! Pushed the change.

If we in _get_initialized_profile_list provide a default for each profile_options entry with unlisted_choice.enabled=False and unlisted_choice.display_name_in_choices="Other..." we would be able to reduce the complexity quite a bit.

Have tried to do this with the latest commit. It seems to work, but would be great to get your eyes on those bits and if it lines up with how you were envisioning this.

Thanks so much for the quick and thoughtful review!

[batpad]

Am taking a look at the failing test.

[consideRatio]

let me know if you would like me to make a new PR with a new branch name there.

No no worries, I think that shouldn't matter for anyone in any way. You could do git rebase -i main and reword the first commit if you want, I can see how that could possibly cause confusion.

Have tried to do this with the latest commit. It seems to work, but would be great to get your eyes on those bits and if it lines up with how you were envisioning this.

Thanks! This section was added, and its something like that I was thinking.

                if option_config.get('unlisted_choice'):
                    if not 'display_name_in_choices' in option_config.get(
                        'unlisted_choice'
                    ):
                        option_config['unlisted_choice'][
                            'display_name_in_choices'
                        ] = "Other..."

I think its important that we initialize unlisted_choice as a dictionary if needed as well, and I think we will check enabled from the template quite often, so then that should probably be initialized at least also.

                if option_config.get("unlisted_choice") is None:
                    option_config["unlisted_choice"] = {}
                if option_config["unlisted_choice"].get("enabled") is None:
                    option_config["unlisted_choice"]["enabled"] = False
                if option_config["unlisted_choice"].get("display_name_in_choices") is None:
                    option_config["unlisted_choice"]["display_name_in_choices"] = "Other..."

I remembered now that there is a smoother way of writing this also, I suggest going with this!

                unlisted_choice = option_config.setdefault("unlisted_choice", {})
                unlisted_choice.setdefault("enabled", False)
                unlisted_choice.setdefault("display_name_in_choices", "Other...")

There is also a comment above where this logic resides that should be updated to reflect that default values for unlisted_choice is populated as well.

[batpad]

Looking into failing tests.

[batpad]

@consideRatio made a small change to the setdefault stuff to only set display_name_in_choices if unlisted_choice is enabled. It seemed a bit awkward to get that set even when unlisted_choice was disabled (and made the test a bit confusing) - but there's no strong opinion here, also happy to change the test to expect display_name_in_choices to be Other... after setting of defaults even when unlisted_choice.enabled is False.

[consideRatio]

Nice, I added a change suggestion also updating an outdated reference to other_text - with that this LGTM!

[consideRatio]

On your end @batpad, is this ready to be merged? If so, I suggest we go for it!