From 91497afbfeb0713ca3170e4935bb5f52c16d24a3 Mon Sep 17 00:00:00 2001 From: axolotle Date: Wed, 12 Apr 2023 13:04:55 +0200 Subject: [PATCH 01/11] form: move option asking+prompt in external function --- src/tests/test_questions.py | 2 +- src/utils/form.py | 276 ++++++++++++++++++------------------ 2 files changed, 142 insertions(+), 136 deletions(-) diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 190eb0cba..7ada38a1c 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -445,7 +445,7 @@ class BaseTest: assert option.name == id_ assert option.ask == {"en": id_} assert option.readonly is (True if is_special_readonly_option else False) - assert option.visible is None + assert option.visible is True # assert option.bind is None if is_special_readonly_option: diff --git a/src/utils/form.py b/src/utils/form.py index 12c3249c3..701632c30 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -35,6 +35,7 @@ from yunohost.utils.i18n import _value_for_locale logger = getActionLogger("yunohost.form") +Context = dict[str, Any] # ╭───────────────────────────────────────────────────────╮ # │ ┌─╴╷ ╷╭─┐╷ │ @@ -200,16 +201,14 @@ class BaseOption: def __init__( self, question: Dict[str, Any], - context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {}, ): self.name = question["name"] - self.context = context self.hooks = hooks self.type = question.get("type", "string") self.default = question.get("default", None) self.optional = question.get("optional", False) - self.visible = question.get("visible", None) + self.visible = question.get("visible", True) self.readonly = question.get("readonly", False) # Don't restrict choices if there's none specified self.choices = question.get("choices", None) @@ -241,75 +240,11 @@ class BaseOption: value = value.strip() return value - def ask_if_needed(self): - if self.visible and not evaluate_simple_js_expression( - self.visible, context=self.context - ): - # FIXME There could be several use case if the question is not displayed: - # - we doesn't want to give a specific value - # - we want to keep the previous value - # - we want the default value - self.value = self.values[self.name] = None - return self.values + def is_visible(self, context: Context) -> bool: + if isinstance(self.visible, bool): + return self.visible - for i in range(5): - # Display question if no value filled or if it's a readonly message - if Moulinette.interface.type == "cli" and os.isatty(1): - text_for_user_input_in_cli = self._format_text_for_user_input_in_cli() - if self.readonly: - Moulinette.display(text_for_user_input_in_cli) - self.value = self.values[self.name] = self.current_value - return self.values - elif self.value is None: - self._prompt(text_for_user_input_in_cli) - - # Apply default value - class_default = getattr(self, "default_value", None) - if self.value in [None, ""] and ( - self.default is not None or class_default is not None - ): - self.value = class_default if self.default is None else self.default - - try: - # Normalize and validate - self.value = self.normalize(self.value, self) - self._value_pre_validator() - except YunohostValidationError as e: - # If in interactive cli, re-ask the current question - if i < 4 and Moulinette.interface.type == "cli" and os.isatty(1): - logger.error(str(e)) - self.value = None - continue - - # Otherwise raise the ValidationError - raise - - break - - self.value = self.values[self.name] = self._value_post_validator() - - # Search for post actions in hooks - post_hook = f"post_ask__{self.name}" - if post_hook in self.hooks: - self.values.update(self.hooks[post_hook](self)) - - return self.values - - def _prompt(self, text): - prefill = "" - if self.current_value is not None: - prefill = self.humanize(self.current_value, self) - elif self.default is not None: - prefill = self.humanize(self.default, self) - self.value = Moulinette.prompt( - message=text, - is_password=self.hide_user_input_in_prompt, - confirm=False, - prefill=prefill, - is_multiline=(self.type == "text"), - autocomplete=self.choices or [], - help=_value_for_locale(self.help), - ) + return evaluate_simple_js_expression(self.visible, context=context) def _format_text_for_user_input_in_cli(self): text_for_user_input_in_cli = _value_for_locale(self.ask) @@ -396,9 +331,9 @@ class DisplayTextOption(BaseOption): argument_type = "display_text" def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} + self, question, hooks: Dict[str, Callable] = {} ): - super().__init__(question, context, hooks) + super().__init__(question, hooks) self.optional = True self.readonly = True @@ -424,13 +359,19 @@ class DisplayTextOption(BaseOption): class ButtonOption(BaseOption): argument_type = "button" - enabled = None + enabled = True def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} + self, question, hooks: Dict[str, Callable] = {} ): - super().__init__(question, context, hooks) - self.enabled = question.get("enabled", None) + super().__init__(question, hooks) + self.enabled = question.get("enabled", True) + + def is_enabled(self, context: Context) -> bool: + if isinstance(self.enabled, bool): + return self.enabled + + return evaluate_simple_js_expression(self.enabled, context=context) # ╭───────────────────────────────────────────────────────╮ @@ -452,10 +393,8 @@ class PasswordOption(BaseOption): default_value = "" forbidden_chars = "{}" - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, context, hooks) + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) self.redact = True if self.default is not None: raise YunohostValidationError( @@ -491,10 +430,8 @@ class NumberOption(BaseOption): argument_type = "number" default_value = None - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, context, hooks) + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) self.min = question.get("min", None) self.max = question.get("max", None) self.step = question.get("step", None) @@ -549,10 +486,8 @@ class BooleanOption(BaseOption): yes_answers = ["1", "yes", "y", "true", "t", "on"] no_answers = ["0", "no", "n", "false", "f", "off"] - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, context, hooks) + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) self.yes = question.get("yes", 1) self.no = question.get("no", 0) if self.default is None: @@ -716,10 +651,8 @@ class FileOption(BaseOption): argument_type = "file" upload_dirs: List[str] = [] - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, context, hooks) + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) self.accept = question.get("accept", "") @classmethod @@ -830,15 +763,16 @@ class TagsOption(BaseOption): return super()._value_post_validator() +# ─ ENTITIES ────────────────────────────────────────────── + + class DomainOption(BaseOption): argument_type = "domain" - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): + def __init__(self, question, hooks: Dict[str, Callable] = {}): from yunohost.domain import domain_list, _get_maindomain - super().__init__(question, context, hooks) + super().__init__(question, hooks) if self.default is None: self.default = _get_maindomain() @@ -864,12 +798,10 @@ class DomainOption(BaseOption): class AppOption(BaseOption): argument_type = "app" - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): + def __init__(self, question, hooks: Dict[str, Callable] = {}): from yunohost.app import app_list - super().__init__(question, context, hooks) + super().__init__(question, hooks) apps = app_list(full=True)["apps"] @@ -891,13 +823,11 @@ class AppOption(BaseOption): class UserOption(BaseOption): argument_type = "user" - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): + def __init__(self, question, hooks: Dict[str, Callable] = {}): from yunohost.user import user_list, user_info from yunohost.domain import _get_maindomain - super().__init__(question, context, hooks) + super().__init__(question, hooks) self.choices = { username: f"{infos['fullname']} ({infos['mail']})" @@ -924,12 +854,10 @@ class UserOption(BaseOption): class GroupOption(BaseOption): argument_type = "group" - def __init__( - self, question, context: Mapping[str, Any] = {}, hooks: Dict[str, Callable] = {} - ): + def __init__(self, question, hooks: Dict[str, Callable] = {}): from yunohost.user import user_group_list - super().__init__(question, context) + super().__init__(question) self.choices = list( user_group_list(short=True, include_primary_groups=False)["groups"] @@ -981,12 +909,111 @@ OPTIONS = { # ╰───────────────────────────────────────────────────────╯ +def prompt_or_validate_form( + raw_options: dict[str, Any], + prefilled_answers: dict[str, Any] = {}, + context: Context = {}, + hooks: dict[str, Callable[[], None]] = {}, +) -> list[BaseOption]: + options = [] + answers = {**prefilled_answers} + + for name, raw_option in raw_options.items(): + raw_option["name"] = name + raw_option["value"] = answers.get(name) + question_class = OPTIONS[raw_option.get("type", "string")] + option = question_class(raw_option, hooks=hooks) + + interactive = Moulinette.interface.type == "cli" and os.isatty(1) + + if isinstance(option, ButtonOption): + if option.is_enabled(context): + continue + else: + raise YunohostValidationError( + "config_action_disabled", + action=option.name, + help=_value_for_locale(option.help), + ) + + if option.is_visible(context): + for i in range(5): + # Display question if no value filled or if it's a readonly message + if interactive: + text_for_user_input_in_cli = ( + option._format_text_for_user_input_in_cli() + ) + if option.readonly: + Moulinette.display(text_for_user_input_in_cli) + option.value = option.current_value + break + elif option.value is None: + prefill = "" + if option.current_value is not None: + prefill = option.humanize(option.current_value, option) + elif option.default is not None: + prefill = option.humanize(option.default, option) + option.value = Moulinette.prompt( + message=text_for_user_input_in_cli, + is_password=option.hide_user_input_in_prompt, + confirm=False, + prefill=prefill, + is_multiline=(option.type == "text"), + autocomplete=option.choices or [], + help=_value_for_locale(option.help), + ) + + # Apply default value + class_default = getattr(option, "default_value", None) + if option.value in [None, ""] and ( + option.default is not None or class_default is not None + ): + option.value = ( + class_default if option.default is None else option.default + ) + + try: + # Normalize and validate + option.value = option.normalize(option.value, option) + option._value_pre_validator() + except YunohostValidationError as e: + # If in interactive cli, re-ask the current question + if i < 4 and interactive: + logger.error(str(e)) + option.value = None + continue + + # Otherwise raise the ValidationError + raise + + break + + option.value = option.values[option.name] = option._value_post_validator() + + # Search for post actions in hooks + post_hook = f"post_ask__{option.name}" + if post_hook in option.hooks: + option.values.update(option.hooks[post_hook](option)) + else: + # FIXME There could be several use case if the question is not displayed: + # - we doesn't want to give a specific value + # - we want to keep the previous value + # - we want the default value + option.value = option.values[option.name] = None + + answers.update(option.values) + context.update(option.values) + options.append(option) + + return options + + def ask_questions_and_parse_answers( - raw_questions: Dict, + raw_options: dict[str, Any], prefilled_answers: Union[str, Mapping[str, Any]] = {}, current_values: Mapping[str, Any] = {}, hooks: Dict[str, Callable[[], None]] = {}, -) -> List[BaseOption]: +) -> list[BaseOption]: """Parse arguments store in either manifest.json or actions.json or from a config panel against the user answers when they are present. @@ -1013,31 +1040,10 @@ def ask_questions_and_parse_answers( answers = {} context = {**current_values, **answers} - out = [] - for name, raw_question in raw_questions.items(): - raw_question["name"] = name - question_class = OPTIONS[raw_question.get("type", "string")] - raw_question["value"] = answers.get(name) - question = question_class(raw_question, context=context, hooks=hooks) - if question.type == "button": - if question.enabled is None or evaluate_simple_js_expression( # type: ignore - question.enabled, context=context # type: ignore - ): # type: ignore - continue - else: - raise YunohostValidationError( - "config_action_disabled", - action=question.name, - help=_value_for_locale(question.help), - ) - - new_values = question.ask_if_needed() - answers.update(new_values) - context.update(new_values) - out.append(question) - - return out + return prompt_or_validate_form( + raw_options, prefilled_answers=answers, context=context, hooks=hooks + ) def hydrate_questions_with_choices(raw_questions: List) -> List: From 4261317e49cc2dff36028077224006469ad8de4b Mon Sep 17 00:00:00 2001 From: axolotle Date: Wed, 12 Apr 2023 20:48:56 +0200 Subject: [PATCH 02/11] form: separate BaseOption into BaseReadonlyOption + BaseInputOption --- src/tests/test_questions.py | 7 +- src/utils/form.py | 191 ++++++++++++++++++++---------------- 2 files changed, 109 insertions(+), 89 deletions(-) diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 7ada38a1c..706645f9b 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -17,6 +17,8 @@ from yunohost import app, domain, user from yunohost.utils.form import ( OPTIONS, ask_questions_and_parse_answers, + BaseInputOption, + BaseReadonlyOption, DisplayTextOption, PasswordOption, DomainOption, @@ -377,8 +379,7 @@ def _fill_or_prompt_one_option(raw_option, intake): answers = {id_: intake} if intake is not None else {} option = ask_questions_and_parse_answers(options, answers)[0] - - return (option, option.value) + return (option, option.value if isinstance(option, BaseInputOption) else None) def _test_value_is_expected_output(value, expected_output): @@ -438,7 +439,7 @@ class BaseTest: id_ = raw_option["id"] option, value = _fill_or_prompt_one_option(raw_option, None) - is_special_readonly_option = isinstance(option, DisplayTextOption) + is_special_readonly_option = isinstance(option, BaseReadonlyOption) assert isinstance(option, OPTIONS[raw_option["type"]]) assert option.type == raw_option["type"] diff --git a/src/utils/form.py b/src/utils/form.py index 701632c30..0da3f892d 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -195,9 +195,6 @@ def evaluate_simple_js_expression(expr, context={}): class BaseOption: - hide_user_input_in_prompt = False - pattern: Optional[Dict] = None - def __init__( self, question: Dict[str, Any], @@ -206,16 +203,101 @@ class BaseOption: self.name = question["name"] self.hooks = hooks self.type = question.get("type", "string") - self.default = question.get("default", None) - self.optional = question.get("optional", False) self.visible = question.get("visible", True) self.readonly = question.get("readonly", False) - # Don't restrict choices if there's none specified - self.choices = question.get("choices", None) - self.pattern = question.get("pattern", self.pattern) self.ask = question.get("ask", self.name) if not isinstance(self.ask, dict): self.ask = {"en": self.ask} + + def is_visible(self, context: Context) -> bool: + if isinstance(self.visible, bool): + return self.visible + + return evaluate_simple_js_expression(self.visible, context=context) + + def _format_text_for_user_input_in_cli(self) -> str: + return _value_for_locale(self.ask) + + +# ╭───────────────────────────────────────────────────────╮ +# │ DISPLAY OPTIONS │ +# ╰───────────────────────────────────────────────────────╯ + + +class BaseReadonlyOption(BaseOption): + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) + self.readonly = True + + +class DisplayTextOption(BaseReadonlyOption): + argument_type = "display_text" + + +class MarkdownOption(BaseReadonlyOption): + argument_type = "markdown" + + +class AlertOption(BaseReadonlyOption): + argument_type = "alert" + + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) + self.style = question.get("style", "info") + + def _format_text_for_user_input_in_cli(self) -> str: + text = _value_for_locale(self.ask) + + if self.style in ["success", "info", "warning", "danger"]: + color = { + "success": "green", + "info": "cyan", + "warning": "yellow", + "danger": "red", + } + prompt = m18n.g(self.style) if self.style != "danger" else m18n.n("danger") + return colorize(prompt, color[self.style]) + f" {text}" + else: + return text + + +class ButtonOption(BaseReadonlyOption): + argument_type = "button" + enabled = True + + def __init__(self, question, hooks: Dict[str, Callable] = {}): + super().__init__(question, hooks) + self.help = question.get("help") + self.style = question.get("style", "success") + self.enabled = question.get("enabled", True) + + def is_enabled(self, context: Context) -> bool: + if isinstance(self.enabled, bool): + return self.enabled + + return evaluate_simple_js_expression(self.enabled, context=context) + + +# ╭───────────────────────────────────────────────────────╮ +# │ INPUT OPTIONS │ +# ╰───────────────────────────────────────────────────────╯ + + +class BaseInputOption(BaseOption): + hide_user_input_in_prompt = False + pattern: Optional[Dict] = None + + def __init__( + self, + question: Dict[str, Any], + hooks: Dict[str, Callable] = {}, + ): + super().__init__(question, hooks) + self.default = question.get("default", None) + self.optional = question.get("optional", False) + # Don't restrict choices if there's none specified + self.choices = question.get("choices", None) + self.pattern = question.get("pattern", self.pattern) self.help = question.get("help") self.redact = question.get("redact", False) self.filter = question.get("filter", None) @@ -240,14 +322,8 @@ class BaseOption: value = value.strip() return value - def is_visible(self, context: Context) -> bool: - if isinstance(self.visible, bool): - return self.visible - - return evaluate_simple_js_expression(self.visible, context=context) - - def _format_text_for_user_input_in_cli(self): - text_for_user_input_in_cli = _value_for_locale(self.ask) + def _format_text_for_user_input_in_cli(self) -> str: + text_for_user_input_in_cli = super()._format_text_for_user_input_in_cli() if self.readonly: text_for_user_input_in_cli = colorize(text_for_user_input_in_cli, "purple") @@ -322,72 +398,15 @@ class BaseOption: return self.value -# ╭───────────────────────────────────────────────────────╮ -# │ DISPLAY OPTIONS │ -# ╰───────────────────────────────────────────────────────╯ - - -class DisplayTextOption(BaseOption): - argument_type = "display_text" - - def __init__( - self, question, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, hooks) - - self.optional = True - self.readonly = True - self.style = question.get( - "style", "info" if question["type"] == "alert" else "" - ) - - def _format_text_for_user_input_in_cli(self): - text = _value_for_locale(self.ask) - - if self.style in ["success", "info", "warning", "danger"]: - color = { - "success": "green", - "info": "cyan", - "warning": "yellow", - "danger": "red", - } - prompt = m18n.g(self.style) if self.style != "danger" else m18n.n("danger") - return colorize(prompt, color[self.style]) + f" {text}" - else: - return text - - -class ButtonOption(BaseOption): - argument_type = "button" - enabled = True - - def __init__( - self, question, hooks: Dict[str, Callable] = {} - ): - super().__init__(question, hooks) - self.enabled = question.get("enabled", True) - - def is_enabled(self, context: Context) -> bool: - if isinstance(self.enabled, bool): - return self.enabled - - return evaluate_simple_js_expression(self.enabled, context=context) - - -# ╭───────────────────────────────────────────────────────╮ -# │ INPUT OPTIONS │ -# ╰───────────────────────────────────────────────────────╯ - - # ─ STRINGS ─────────────────────────────────────────────── -class StringOption(BaseOption): +class StringOption(BaseInputOption): argument_type = "string" default_value = "" -class PasswordOption(BaseOption): +class PasswordOption(BaseInputOption): hide_user_input_in_prompt = True argument_type = "password" default_value = "" @@ -426,7 +445,7 @@ class ColorOption(StringOption): # ─ NUMERIC ─────────────────────────────────────────────── -class NumberOption(BaseOption): +class NumberOption(BaseInputOption): argument_type = "number" default_value = None @@ -480,7 +499,7 @@ class NumberOption(BaseOption): # ─ BOOLEAN ─────────────────────────────────────────────── -class BooleanOption(BaseOption): +class BooleanOption(BaseInputOption): argument_type = "boolean" default_value = 0 yes_answers = ["1", "yes", "y", "true", "t", "on"] @@ -606,7 +625,7 @@ class EmailOption(StringOption): } -class WebPathOption(BaseOption): +class WebPathOption(BaseInputOption): argument_type = "path" default_value = "" @@ -647,7 +666,7 @@ class URLOption(StringOption): # ─ FILE ────────────────────────────────────────────────── -class FileOption(BaseOption): +class FileOption(BaseInputOption): argument_type = "file" upload_dirs: List[str] = [] @@ -713,7 +732,7 @@ class FileOption(BaseOption): # ─ CHOICES ─────────────────────────────────────────────── -class TagsOption(BaseOption): +class TagsOption(BaseInputOption): argument_type = "tags" default_value = "" @@ -766,7 +785,7 @@ class TagsOption(BaseOption): # ─ ENTITIES ────────────────────────────────────────────── -class DomainOption(BaseOption): +class DomainOption(BaseInputOption): argument_type = "domain" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -795,7 +814,7 @@ class DomainOption(BaseOption): return value -class AppOption(BaseOption): +class AppOption(BaseInputOption): argument_type = "app" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -820,7 +839,7 @@ class AppOption(BaseOption): self.choices.update({app["id"]: _app_display(app) for app in apps}) -class UserOption(BaseOption): +class UserOption(BaseInputOption): argument_type = "user" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -851,7 +870,7 @@ class UserOption(BaseOption): break -class GroupOption(BaseOption): +class GroupOption(BaseInputOption): argument_type = "group" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -877,8 +896,8 @@ class GroupOption(BaseOption): OPTIONS = { "display_text": DisplayTextOption, - "markdown": DisplayTextOption, - "alert": DisplayTextOption, + "markdown": MarkdownOption, + "alert": AlertOption, "button": ButtonOption, "string": StringOption, "text": StringOption, From 9e8e0497dd286ee4a0992d005bc893ea57e6ef38 Mon Sep 17 00:00:00 2001 From: axolotle Date: Wed, 12 Apr 2023 21:01:25 +0200 Subject: [PATCH 03/11] form: fix readonly prompting + + choices + tests --- src/tests/test_questions.py | 68 ++++++------------------ src/utils/configpanel.py | 10 ---- src/utils/form.py | 100 ++++++++++++++++++++++-------------- 3 files changed, 78 insertions(+), 100 deletions(-) diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 706645f9b..9e7be5db4 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -662,9 +662,7 @@ class TestString(BaseTest): (" ##value \n \tvalue\n ", "##value \n \tvalue"), ], reason=r"should fail or without `\n`?"), # readonly - *xfail(scenarios=[ - ("overwrite", "expected value", {"readonly": True, "default": "expected value"}), - ], reason="Should not be overwritten"), + ("overwrite", "expected value", {"readonly": True, "current_value": "expected value"}), ] # fmt: on @@ -701,9 +699,7 @@ class TestText(BaseTest): (r" ##value \n \tvalue\n ", r"##value \n \tvalue\n"), ], reason="Should not be stripped"), # readonly - *xfail(scenarios=[ - ("overwrite", "expected value", {"readonly": True, "default": "expected value"}), - ], reason="Should not be overwritten"), + ("overwrite", "expected value", {"readonly": True, "current_value": "expected value"}), ] # fmt: on @@ -737,9 +733,7 @@ class TestPassword(BaseTest): ("secret", FAIL), *[("supersecret" + char, FAIL) for char in PasswordOption.forbidden_chars], # FIXME maybe add ` \n` to the list? # readonly - *xpass(scenarios=[ - ("s3cr3t!!", "s3cr3t!!", {"readonly": True}), - ], reason="Should fail since readonly is forbidden"), + ("s3cr3t!!", YunohostError, {"readonly": True, "current_value": "isforbidden"}), # readonly is forbidden ] # fmt: on @@ -780,9 +774,7 @@ class TestColor(BaseTest): ("yellow", "#ffff00"), ], reason="Should work with pydantic"), # readonly - *xfail(scenarios=[ - ("#ffff00", "#fe100", {"readonly": True, "default": "#fe100"}), - ], reason="Should not be overwritten"), + ("#ffff00", "#fe100", {"readonly": True, "current_value": "#fe100"}), ] # fmt: on @@ -824,9 +816,7 @@ class TestNumber(BaseTest): (-10, -10, {"default": 10}), (-10, -10, {"default": 10, "optional": True}), # readonly - *xfail(scenarios=[ - (1337, 10000, {"readonly": True, "default": 10000}), - ], reason="Should not be overwritten"), + (1337, 10000, {"readonly": True, "current_value": 10000}), ] # fmt: on # FIXME should `step` be some kind of "multiple of"? @@ -891,9 +881,7 @@ class TestBoolean(BaseTest): "scenarios": all_fails("", "y", "n", error=AssertionError), }, # readonly - *xfail(scenarios=[ - (1, 0, {"readonly": True, "default": 0}), - ], reason="Should not be overwritten"), + (1, 0, {"readonly": True, "current_value": 0}), ] @@ -931,9 +919,7 @@ class TestDate(BaseTest): ("12-01-10", FAIL), ("2022-02-29", FAIL), # readonly - *xfail(scenarios=[ - ("2070-12-31", "2024-02-29", {"readonly": True, "default": "2024-02-29"}), - ], reason="Should not be overwritten"), + ("2070-12-31", "2024-02-29", {"readonly": True, "current_value": "2024-02-29"}), ] # fmt: on @@ -966,9 +952,7 @@ class TestTime(BaseTest): ("23:1", FAIL), ("23:005", FAIL), # readonly - *xfail(scenarios=[ - ("00:00", "08:00", {"readonly": True, "default": "08:00"}), - ], reason="Should not be overwritten"), + ("00:00", "08:00", {"readonly": True, "current_value": "08:00"}), ] # fmt: on @@ -992,9 +976,7 @@ class TestEmail(BaseTest): *nones(None, "", output=""), ("\n Abc@example.tld ", "Abc@example.tld"), # readonly - *xfail(scenarios=[ - ("Abc@example.tld", "admin@ynh.local", {"readonly": True, "default": "admin@ynh.local"}), - ], reason="Should not be overwritten"), + ("Abc@example.tld", "admin@ynh.local", {"readonly": True, "current_value": "admin@ynh.local"}), # Next examples are from https://github.com/JoshData/python-email-validator/blob/main/tests/test_syntax.py # valid email values @@ -1107,9 +1089,7 @@ class TestWebPath(BaseTest): ("https://example.com/folder", "/https://example.com/folder") ], reason="Should fail or scheme+domain removed"), # readonly - *xfail(scenarios=[ - ("/overwrite", "/value", {"readonly": True, "default": "/value"}), - ], reason="Should not be overwritten"), + ("/overwrite", "/value", {"readonly": True, "current_value": "/value"}), # FIXME should path have forbidden_chars? ] # fmt: on @@ -1134,9 +1114,7 @@ class TestUrl(BaseTest): *nones(None, "", output=""), ("http://some.org/folder/file.txt", "http://some.org/folder/file.txt"), # readonly - *xfail(scenarios=[ - ("https://overwrite.org", "https://example.org", {"readonly": True, "default": "https://example.org"}), - ], reason="Should not be overwritten"), + ("https://overwrite.org", "https://example.org", {"readonly": True, "current_value": "https://example.org"}), # rest is taken from https://github.com/pydantic/pydantic/blob/main/tests/test_networks.py # valid *unchanged( @@ -1426,9 +1404,7 @@ class TestSelect(BaseTest): ] }, # readonly - *xfail(scenarios=[ - ("one", "two", {"readonly": True, "choices": ["one", "two"], "default": "two"}), - ], reason="Should not be overwritten"), + ("one", "two", {"readonly": True, "choices": ["one", "two"], "current_value": "two"}), ] # fmt: on @@ -1476,9 +1452,7 @@ class TestTags(BaseTest): *all_fails(*([t] for t in [False, True, -1, 0, 1, 1337, 13.37, [], ["one"], {}]), raw_option={"choices": [False, True, -1, 0, 1, 1337, 13.37, [], ["one"], {}]}), *all_fails(*([str(t)] for t in [False, True, -1, 0, 1, 1337, 13.37, [], ["one"], {}]), raw_option={"choices": [False, True, -1, 0, 1, 1337, 13.37, [], ["one"], {}]}), # readonly - *xfail(scenarios=[ - ("one", "one,two", {"readonly": True, "choices": ["one", "two"], "default": "one,two"}), - ], reason="Should not be overwritten"), + ("one", "one,two", {"readonly": True, "choices": ["one", "two"], "current_value": "one,two"}), ] # fmt: on @@ -1526,9 +1500,7 @@ class TestDomain(BaseTest): ("doesnt_exist.pouet", FAIL, {}), ("fake.com", FAIL, {"choices": ["fake.com"]}), # readonly - *xpass(scenarios=[ - (domains1[0], domains1[0], {"readonly": True}), - ], reason="Should fail since readonly is forbidden"), + (domains1[0], YunohostError, {"readonly": True}), # readonly is forbidden ] }, { @@ -1625,9 +1597,7 @@ class TestApp(BaseTest): (installed_non_webapp["id"], installed_non_webapp["id"]), (installed_non_webapp["id"], FAIL, {"filter": "is_webapp"}), # readonly - *xpass(scenarios=[ - (installed_non_webapp["id"], installed_non_webapp["id"], {"readonly": True}), - ], reason="Should fail since readonly is forbidden"), + (installed_non_webapp["id"], YunohostError, {"readonly": True}), # readonly is forbidden ] }, ] @@ -1744,9 +1714,7 @@ class TestUser(BaseTest): ("", regular_username, {"default": regular_username}) ], reason="Should throw 'no default allowed'"), # readonly - *xpass(scenarios=[ - (admin_username, admin_username, {"readonly": True}), - ], reason="Should fail since readonly is forbidden"), + (admin_username, YunohostError, {"readonly": True}), # readonly is forbidden ] }, ] @@ -1821,9 +1789,7 @@ class TestGroup(BaseTest): ("", "custom_group", {"default": "custom_group"}), ], reason="Should throw 'default must be in (None, 'all_users', 'visitors', 'admins')"), # readonly - *xpass(scenarios=[ - ("admins", "admins", {"readonly": True}), - ], reason="Should fail since readonly is forbidden"), + ("admins", YunohostError, {"readonly": True}), # readonly is forbidden ] }, ] diff --git a/src/utils/configpanel.py b/src/utils/configpanel.py index 2c56eb754..355956574 100644 --- a/src/utils/configpanel.py +++ b/src/utils/configpanel.py @@ -465,20 +465,10 @@ class ConfigPanel: "max_progression", ] forbidden_keywords += format_description["sections"] - forbidden_readonly_types = ["password", "app", "domain", "user", "file"] for _, _, option in self._iterate(): if option["id"] in forbidden_keywords: raise YunohostError("config_forbidden_keyword", keyword=option["id"]) - if ( - option.get("readonly", False) - and option.get("type", "string") in forbidden_readonly_types - ): - raise YunohostError( - "config_forbidden_readonly_type", - type=option["type"], - id=option["id"], - ) return self.config diff --git a/src/utils/form.py b/src/utils/form.py index 0da3f892d..071bbaa21 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -204,7 +204,16 @@ class BaseOption: self.hooks = hooks self.type = question.get("type", "string") self.visible = question.get("visible", True) + self.readonly = question.get("readonly", False) + if self.readonly and self.type in {"password", "app", "domain", "user", "group", "file"}: + # FIXME i18n + raise YunohostError( + "config_forbidden_readonly_type", + type=self.type, + id=self.name, + ) + self.ask = question.get("ask", self.name) if not isinstance(self.ask, dict): self.ask = {"en": self.ask} @@ -328,9 +337,10 @@ class BaseInputOption(BaseOption): if self.readonly: text_for_user_input_in_cli = colorize(text_for_user_input_in_cli, "purple") if self.choices: - return ( - text_for_user_input_in_cli + f" {self.choices[self.current_value]}" - ) + choice = self.current_value + if isinstance(self.choices, dict) and choice is not None: + choice = self.choices[choice] + return f"{text_for_user_input_in_cli} {choice}" return text_for_user_input_in_cli + f" {self.humanize(self.current_value)}" elif self.choices: # Prevent displaying a shitload of choices @@ -348,7 +358,9 @@ class BaseInputOption(BaseOption): m18n.n("other_available_options", n=remaining_choices) ] - choices_to_display = " | ".join(choices_to_display) + choices_to_display = " | ".join( + str(choice) for choice in choices_to_display + ) text_for_user_input_in_cli += f" [{choices_to_display}]" @@ -946,7 +958,7 @@ def prompt_or_validate_form( interactive = Moulinette.interface.type == "cli" and os.isatty(1) if isinstance(option, ButtonOption): - if option.is_enabled(context): + if option.is_visible(context) and option.is_enabled(context): continue else: raise YunohostValidationError( @@ -955,32 +967,49 @@ def prompt_or_validate_form( help=_value_for_locale(option.help), ) - if option.is_visible(context): + # FIXME not sure why we do not append Buttons to returned options + options.append(option) + + if not option.is_visible(context): + if isinstance(option, BaseInputOption): + # FIXME There could be several use case if the question is not displayed: + # - we doesn't want to give a specific value + # - we want to keep the previous value + # - we want the default value + option.value = context[option.name] = None + + continue + + message = option._format_text_for_user_input_in_cli() + + if option.readonly: + if interactive: + Moulinette.display(message) + + if isinstance(option, BaseInputOption): + option.value = context[option.name] = option.current_value + + continue + + if isinstance(option, BaseInputOption): for i in range(5): - # Display question if no value filled or if it's a readonly message - if interactive: - text_for_user_input_in_cli = ( - option._format_text_for_user_input_in_cli() + if interactive and option.value is None: + prefill = "" + + if option.current_value is not None: + prefill = option.humanize(option.current_value, option) + elif option.default is not None: + prefill = option.humanize(option.default, option) + + option.value = Moulinette.prompt( + message=message, + is_password=isinstance(option, PasswordOption), + confirm=False, + prefill=prefill, + is_multiline=(option.type == "text"), + autocomplete=option.choices or [], + help=_value_for_locale(option.help), ) - if option.readonly: - Moulinette.display(text_for_user_input_in_cli) - option.value = option.current_value - break - elif option.value is None: - prefill = "" - if option.current_value is not None: - prefill = option.humanize(option.current_value, option) - elif option.default is not None: - prefill = option.humanize(option.default, option) - option.value = Moulinette.prompt( - message=text_for_user_input_in_cli, - is_password=option.hide_user_input_in_prompt, - confirm=False, - prefill=prefill, - is_multiline=(option.type == "text"), - autocomplete=option.choices or [], - help=_value_for_locale(option.help), - ) # Apply default value class_default = getattr(option, "default_value", None) @@ -1013,16 +1042,9 @@ def prompt_or_validate_form( post_hook = f"post_ask__{option.name}" if post_hook in option.hooks: option.values.update(option.hooks[post_hook](option)) - else: - # FIXME There could be several use case if the question is not displayed: - # - we doesn't want to give a specific value - # - we want to keep the previous value - # - we want the default value - option.value = option.values[option.name] = None - answers.update(option.values) - context.update(option.values) - options.append(option) + answers.update(option.values) + context.update(option.values) return options @@ -1070,7 +1092,7 @@ def hydrate_questions_with_choices(raw_questions: List) -> List: for raw_question in raw_questions: question = OPTIONS[raw_question.get("type", "string")](raw_question) - if question.choices: + if isinstance(question, BaseInputOption) and question.choices: raw_question["choices"] = question.choices raw_question["default"] = question.default out.append(raw_question) From 07636fe21e15ebf0fcb77aabe9a752771d1c6901 Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 02:36:18 +0200 Subject: [PATCH 04/11] form: rename text_cli_* to _get_prompt_message + message --- src/utils/form.py | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/utils/form.py b/src/utils/form.py index 071bbaa21..57d4cabcc 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -193,6 +193,8 @@ def evaluate_simple_js_expression(expr, context={}): # │ ╰─╯╵ ╵ ╶┴╴╰─╯╵╰╯╶─╯ │ # ╰───────────────────────────────────────────────────────╯ +FORBIDDEN_READONLY_TYPES = {"password", "app", "domain", "user", "group"} + class BaseOption: def __init__( @@ -206,7 +208,7 @@ class BaseOption: self.visible = question.get("visible", True) self.readonly = question.get("readonly", False) - if self.readonly and self.type in {"password", "app", "domain", "user", "group", "file"}: + if self.readonly and self.type in FORBIDDEN_READONLY_TYPES: # FIXME i18n raise YunohostError( "config_forbidden_readonly_type", @@ -224,7 +226,7 @@ class BaseOption: return evaluate_simple_js_expression(self.visible, context=context) - def _format_text_for_user_input_in_cli(self) -> str: + def _get_prompt_message(self) -> str: return _value_for_locale(self.ask) @@ -254,7 +256,7 @@ class AlertOption(BaseReadonlyOption): super().__init__(question, hooks) self.style = question.get("style", "info") - def _format_text_for_user_input_in_cli(self) -> str: + def _get_prompt_message(self) -> str: text = _value_for_locale(self.ask) if self.style in ["success", "info", "warning", "danger"]: @@ -331,17 +333,17 @@ class BaseInputOption(BaseOption): value = value.strip() return value - def _format_text_for_user_input_in_cli(self) -> str: - text_for_user_input_in_cli = super()._format_text_for_user_input_in_cli() + def _get_prompt_message(self) -> str: + message = super()._get_prompt_message() if self.readonly: - text_for_user_input_in_cli = colorize(text_for_user_input_in_cli, "purple") + message = colorize(message, "purple") if self.choices: choice = self.current_value if isinstance(self.choices, dict) and choice is not None: choice = self.choices[choice] - return f"{text_for_user_input_in_cli} {choice}" - return text_for_user_input_in_cli + f" {self.humanize(self.current_value)}" + return f"{message} {choice}" + return message + f" {self.humanize(self.current_value)}" elif self.choices: # Prevent displaying a shitload of choices # (e.g. 100+ available users when choosing an app admin...) @@ -362,9 +364,9 @@ class BaseInputOption(BaseOption): str(choice) for choice in choices_to_display ) - text_for_user_input_in_cli += f" [{choices_to_display}]" + message += f" [{choices_to_display}]" - return text_for_user_input_in_cli + return message def _value_pre_validator(self): if self.value in [None, ""] and not self.optional: @@ -590,13 +592,13 @@ class BooleanOption(BaseInputOption): def get(self, key, default=None): return getattr(self, key, default) - def _format_text_for_user_input_in_cli(self): - text_for_user_input_in_cli = super()._format_text_for_user_input_in_cli() + def _get_prompt_message(self): + message = super()._get_prompt_message() if not self.readonly: - text_for_user_input_in_cli += " [yes | no]" + message += " [yes | no]" - return text_for_user_input_in_cli + return message # ─ TIME ────────────────────────────────────────────────── @@ -980,7 +982,7 @@ def prompt_or_validate_form( continue - message = option._format_text_for_user_input_in_cli() + message = option._get_prompt_message() if option.readonly: if interactive: From f0f89d8f2a4fb9b932b2f32d8cf66754fde3a077 Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 13:58:24 +0200 Subject: [PATCH 05/11] form: restrict choices to select, tags, domain, app, user + group --- src/tests/test_questions.py | 85 +++---------------------- src/utils/form.py | 121 +++++++++++++++++++++++------------- 2 files changed, 84 insertions(+), 122 deletions(-) diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 9e7be5db4..3e7927dce 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -17,9 +17,9 @@ from yunohost import app, domain, user from yunohost.utils.form import ( OPTIONS, ask_questions_and_parse_answers, + BaseChoicesOption, BaseInputOption, BaseReadonlyOption, - DisplayTextOption, PasswordOption, DomainOption, WebPathOption, @@ -490,14 +490,12 @@ class BaseTest: option, value = _fill_or_prompt_one_option(raw_option, None) expected_message = option.ask["en"] + choices = [] - if option.choices: - choices = ( - option.choices - if isinstance(option.choices, list) - else option.choices.keys() - ) - expected_message += f" [{' | '.join(choices)}]" + if isinstance(option, BaseChoicesOption): + choices = option.choices + if choices: + expected_message += f" [{' | '.join(choices)}]" if option.type == "boolean": expected_message += " [yes | no]" @@ -507,7 +505,7 @@ class BaseTest: confirm=False, # FIXME no confirm? prefill=prefill, is_multiline=option.type == "text", - autocomplete=option.choices or [], + autocomplete=choices, help=option.help["en"], ) @@ -1972,75 +1970,6 @@ def test_question_string_input_test_ask_with_example(): assert example_text in prompt.call_args[1]["message"] -def test_question_string_with_choice(): - questions = {"some_string": {"type": "string", "choices": ["fr", "en"]}} - answers = {"some_string": "fr"} - out = ask_questions_and_parse_answers(questions, answers)[0] - - assert out.name == "some_string" - assert out.type == "string" - assert out.value == "fr" - - -def test_question_string_with_choice_prompt(): - questions = {"some_string": {"type": "string", "choices": ["fr", "en"]}} - answers = {"some_string": "fr"} - with patch.object(Moulinette, "prompt", return_value="fr"), patch.object( - os, "isatty", return_value=True - ): - out = ask_questions_and_parse_answers(questions, answers)[0] - - assert out.name == "some_string" - assert out.type == "string" - assert out.value == "fr" - - -def test_question_string_with_choice_bad(): - questions = {"some_string": {"type": "string", "choices": ["fr", "en"]}} - answers = {"some_string": "bad"} - - with pytest.raises(YunohostError), patch.object(os, "isatty", return_value=False): - ask_questions_and_parse_answers(questions, answers) - - -def test_question_string_with_choice_ask(): - ask_text = "some question" - choices = ["fr", "en", "es", "it", "ru"] - questions = { - "some_string": { - "ask": ask_text, - "choices": choices, - } - } - answers = {} - - with patch.object(Moulinette, "prompt", return_value="ru") as prompt, patch.object( - os, "isatty", return_value=True - ): - ask_questions_and_parse_answers(questions, answers) - assert ask_text in prompt.call_args[1]["message"] - - for choice in choices: - assert choice in prompt.call_args[1]["message"] - - -def test_question_string_with_choice_default(): - questions = { - "some_string": { - "type": "string", - "choices": ["fr", "en"], - "default": "en", - } - } - answers = {} - with patch.object(os, "isatty", return_value=False): - out = ask_questions_and_parse_answers(questions, answers)[0] - - assert out.name == "some_string" - assert out.type == "string" - assert out.value == "en" - - @pytest.mark.skip # we should do something with this example def test_question_password_input_test_ask_with_example(): ask_text = "some question" diff --git a/src/utils/form.py b/src/utils/form.py index 57d4cabcc..02f51b6c4 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -306,8 +306,6 @@ class BaseInputOption(BaseOption): super().__init__(question, hooks) self.default = question.get("default", None) self.optional = question.get("optional", False) - # Don't restrict choices if there's none specified - self.choices = question.get("choices", None) self.pattern = question.get("pattern", self.pattern) self.help = question.get("help") self.redact = question.get("redact", False) @@ -338,33 +336,7 @@ class BaseInputOption(BaseOption): if self.readonly: message = colorize(message, "purple") - if self.choices: - choice = self.current_value - if isinstance(self.choices, dict) and choice is not None: - choice = self.choices[choice] - return f"{message} {choice}" - return message + f" {self.humanize(self.current_value)}" - elif self.choices: - # Prevent displaying a shitload of choices - # (e.g. 100+ available users when choosing an app admin...) - choices = ( - list(self.choices.keys()) - if isinstance(self.choices, dict) - else self.choices - ) - choices_to_display = choices[:20] - remaining_choices = len(choices[20:]) - - if remaining_choices > 0: - choices_to_display += [ - m18n.n("other_available_options", n=remaining_choices) - ] - - choices_to_display = " | ".join( - str(choice) for choice in choices_to_display - ) - - message += f" [{choices_to_display}]" + return f"{message} {self.humanize(self.current_value)}" return message @@ -374,13 +346,6 @@ class BaseInputOption(BaseOption): # we have an answer, do some post checks if self.value not in [None, ""]: - if self.choices and self.value not in self.choices: - raise YunohostValidationError( - "app_argument_choice_invalid", - name=self.name, - value=self.value, - choices=", ".join(str(choice) for choice in self.choices), - ) if self.pattern and not re.match(self.pattern["regexp"], str(self.value)): raise YunohostValidationError( self.pattern["error"], @@ -746,7 +711,72 @@ class FileOption(BaseInputOption): # ─ CHOICES ─────────────────────────────────────────────── -class TagsOption(BaseInputOption): +class BaseChoicesOption(BaseInputOption): + def __init__( + self, + question: Dict[str, Any], + hooks: Dict[str, Callable] = {}, + ): + super().__init__(question, hooks) + # Don't restrict choices if there's none specified + self.choices = question.get("choices", None) + + def _get_prompt_message(self) -> str: + message = super()._get_prompt_message() + + if self.readonly: + message = message + choice = self.current_value + + if isinstance(self.choices, dict) and choice is not None: + choice = self.choices[choice] + + return f"{colorize(message, 'purple')} {choice}" + + if self.choices: + # Prevent displaying a shitload of choices + # (e.g. 100+ available users when choosing an app admin...) + choices = ( + list(self.choices.keys()) + if isinstance(self.choices, dict) + else self.choices + ) + choices_to_display = choices[:20] + remaining_choices = len(choices[20:]) + + if remaining_choices > 0: + choices_to_display += [ + m18n.n("other_available_options", n=remaining_choices) + ] + + choices_to_display = " | ".join( + str(choice) for choice in choices_to_display + ) + + return f"{message} [{choices_to_display}]" + + return message + + def _value_pre_validator(self): + super()._value_pre_validator() + + # we have an answer, do some post checks + if self.value not in [None, ""]: + if self.choices and self.value not in self.choices: + raise YunohostValidationError( + "app_argument_choice_invalid", + name=self.name, + value=self.value, + choices=", ".join(str(choice) for choice in self.choices), + ) + + +class SelectOption(BaseChoicesOption): + argument_type = "select" + default_value = "" + + +class TagsOption(BaseChoicesOption): argument_type = "tags" default_value = "" @@ -799,7 +829,7 @@ class TagsOption(BaseInputOption): # ─ ENTITIES ────────────────────────────────────────────── -class DomainOption(BaseInputOption): +class DomainOption(BaseChoicesOption): argument_type = "domain" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -828,7 +858,7 @@ class DomainOption(BaseInputOption): return value -class AppOption(BaseInputOption): +class AppOption(BaseChoicesOption): argument_type = "app" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -853,7 +883,7 @@ class AppOption(BaseInputOption): self.choices.update({app["id"]: _app_display(app) for app in apps}) -class UserOption(BaseInputOption): +class UserOption(BaseChoicesOption): argument_type = "user" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -884,7 +914,7 @@ class UserOption(BaseInputOption): break -class GroupOption(BaseInputOption): +class GroupOption(BaseChoicesOption): argument_type = "group" def __init__(self, question, hooks: Dict[str, Callable] = {}): @@ -926,7 +956,7 @@ OPTIONS = { "path": WebPathOption, "url": URLOption, "file": FileOption, - "select": StringOption, + "select": SelectOption, "tags": TagsOption, "domain": DomainOption, "app": AppOption, @@ -997,6 +1027,9 @@ def prompt_or_validate_form( for i in range(5): if interactive and option.value is None: prefill = "" + choices = ( + option.choices if isinstance(option, BaseChoicesOption) else [] + ) if option.current_value is not None: prefill = option.humanize(option.current_value, option) @@ -1009,7 +1042,7 @@ def prompt_or_validate_form( confirm=False, prefill=prefill, is_multiline=(option.type == "text"), - autocomplete=option.choices or [], + autocomplete=choices, help=_value_for_locale(option.help), ) @@ -1094,7 +1127,7 @@ def hydrate_questions_with_choices(raw_questions: List) -> List: for raw_question in raw_questions: question = OPTIONS[raw_question.get("type", "string")](raw_question) - if isinstance(question, BaseInputOption) and question.choices: + if isinstance(question, BaseChoicesOption) and question.choices: raw_question["choices"] = question.choices raw_question["default"] = question.default out.append(raw_question) From c439c47d67b5fe7e0dece709212d4d6d3b18549c Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 14:00:04 +0200 Subject: [PATCH 06/11] form: restrict filter to AppOption --- src/utils/form.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/form.py b/src/utils/form.py index 02f51b6c4..edae7717b 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -309,7 +309,6 @@ class BaseInputOption(BaseOption): self.pattern = question.get("pattern", self.pattern) self.help = question.get("help") self.redact = question.get("redact", False) - self.filter = question.get("filter", None) # .current_value is the currently stored value self.current_value = question.get("current_value") # .value is the "proposed" value which we got from the user @@ -865,6 +864,7 @@ class AppOption(BaseChoicesOption): from yunohost.app import app_list super().__init__(question, hooks) + self.filter = question.get("filter", None) apps = app_list(full=True)["apps"] From fe2761da4ab2289d5b622ab061d323ad603f9f2a Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 14:05:03 +0200 Subject: [PATCH 07/11] configpanel: fix choices --- src/utils/configpanel.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/utils/configpanel.py b/src/utils/configpanel.py index 355956574..2914ae11f 100644 --- a/src/utils/configpanel.py +++ b/src/utils/configpanel.py @@ -30,6 +30,8 @@ from moulinette.utils.log import getActionLogger from yunohost.utils.error import YunohostError, YunohostValidationError from yunohost.utils.form import ( OPTIONS, + BaseChoicesOption, + BaseInputOption, BaseOption, FileOption, ask_questions_and_parse_answers, @@ -148,9 +150,11 @@ class ConfigPanel: option["ask"] = ask question_class = OPTIONS[option.get("type", "string")] # FIXME : maybe other properties should be taken from the question, not just choices ?. - option["choices"] = question_class(option).choices - option["default"] = question_class(option).default - option["pattern"] = question_class(option).pattern + if issubclass(question_class, BaseChoicesOption): + option["choices"] = question_class(option).choices + if issubclass(question_class, BaseInputOption): + option["default"] = question_class(option).default + option["pattern"] = question_class(option).pattern else: result[key] = {"ask": ask} if "current_value" in option: From 1c7d427be0fea47d64a3abf67cf7a86b631c4d9a Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 14:28:00 +0200 Subject: [PATCH 08/11] form: remove hooks from Option's attrs --- src/utils/form.py | 68 ++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/src/utils/form.py b/src/utils/form.py index edae7717b..b455fe812 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -200,10 +200,8 @@ class BaseOption: def __init__( self, question: Dict[str, Any], - hooks: Dict[str, Callable] = {}, ): self.name = question["name"] - self.hooks = hooks self.type = question.get("type", "string") self.visible = question.get("visible", True) @@ -236,8 +234,8 @@ class BaseOption: class BaseReadonlyOption(BaseOption): - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.readonly = True @@ -252,8 +250,8 @@ class MarkdownOption(BaseReadonlyOption): class AlertOption(BaseReadonlyOption): argument_type = "alert" - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.style = question.get("style", "info") def _get_prompt_message(self) -> str: @@ -276,8 +274,8 @@ class ButtonOption(BaseReadonlyOption): argument_type = "button" enabled = True - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.help = question.get("help") self.style = question.get("style", "success") self.enabled = question.get("enabled", True) @@ -298,12 +296,8 @@ class BaseInputOption(BaseOption): hide_user_input_in_prompt = False pattern: Optional[Dict] = None - def __init__( - self, - question: Dict[str, Any], - hooks: Dict[str, Callable] = {}, - ): - super().__init__(question, hooks) + def __init__(self, question: Dict[str, Any]): + super().__init__(question) self.default = question.get("default", None) self.optional = question.get("optional", False) self.pattern = question.get("pattern", self.pattern) @@ -390,8 +384,8 @@ class PasswordOption(BaseInputOption): default_value = "" forbidden_chars = "{}" - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.redact = True if self.default is not None: raise YunohostValidationError( @@ -427,8 +421,8 @@ class NumberOption(BaseInputOption): argument_type = "number" default_value = None - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.min = question.get("min", None) self.max = question.get("max", None) self.step = question.get("step", None) @@ -483,8 +477,8 @@ class BooleanOption(BaseInputOption): yes_answers = ["1", "yes", "y", "true", "t", "on"] no_answers = ["0", "no", "n", "false", "f", "off"] - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.yes = question.get("yes", 1) self.no = question.get("no", 0) if self.default is None: @@ -648,8 +642,8 @@ class FileOption(BaseInputOption): argument_type = "file" upload_dirs: List[str] = [] - def __init__(self, question, hooks: Dict[str, Callable] = {}): - super().__init__(question, hooks) + def __init__(self, question): + super().__init__(question) self.accept = question.get("accept", "") @classmethod @@ -714,9 +708,8 @@ class BaseChoicesOption(BaseInputOption): def __init__( self, question: Dict[str, Any], - hooks: Dict[str, Callable] = {}, ): - super().__init__(question, hooks) + super().__init__(question) # Don't restrict choices if there's none specified self.choices = question.get("choices", None) @@ -831,10 +824,10 @@ class TagsOption(BaseChoicesOption): class DomainOption(BaseChoicesOption): argument_type = "domain" - def __init__(self, question, hooks: Dict[str, Callable] = {}): + def __init__(self, question): from yunohost.domain import domain_list, _get_maindomain - super().__init__(question, hooks) + super().__init__(question) if self.default is None: self.default = _get_maindomain() @@ -860,10 +853,10 @@ class DomainOption(BaseChoicesOption): class AppOption(BaseChoicesOption): argument_type = "app" - def __init__(self, question, hooks: Dict[str, Callable] = {}): + def __init__(self, question): from yunohost.app import app_list - super().__init__(question, hooks) + super().__init__(question) self.filter = question.get("filter", None) apps = app_list(full=True)["apps"] @@ -886,11 +879,11 @@ class AppOption(BaseChoicesOption): class UserOption(BaseChoicesOption): argument_type = "user" - def __init__(self, question, hooks: Dict[str, Callable] = {}): + def __init__(self, question): from yunohost.user import user_list, user_info from yunohost.domain import _get_maindomain - super().__init__(question, hooks) + super().__init__(question) self.choices = { username: f"{infos['fullname']} ({infos['mail']})" @@ -917,7 +910,7 @@ class UserOption(BaseChoicesOption): class GroupOption(BaseChoicesOption): argument_type = "group" - def __init__(self, question, hooks: Dict[str, Callable] = {}): + def __init__(self, question): from yunohost.user import user_group_list super().__init__(question) @@ -972,11 +965,14 @@ OPTIONS = { # ╰───────────────────────────────────────────────────────╯ +Hooks = dict[str, Callable[[BaseInputOption], Any]] + + def prompt_or_validate_form( raw_options: dict[str, Any], prefilled_answers: dict[str, Any] = {}, context: Context = {}, - hooks: dict[str, Callable[[], None]] = {}, + hooks: Hooks = {}, ) -> list[BaseOption]: options = [] answers = {**prefilled_answers} @@ -985,7 +981,7 @@ def prompt_or_validate_form( raw_option["name"] = name raw_option["value"] = answers.get(name) question_class = OPTIONS[raw_option.get("type", "string")] - option = question_class(raw_option, hooks=hooks) + option = question_class(raw_option) interactive = Moulinette.interface.type == "cli" and os.isatty(1) @@ -1075,8 +1071,8 @@ def prompt_or_validate_form( # Search for post actions in hooks post_hook = f"post_ask__{option.name}" - if post_hook in option.hooks: - option.values.update(option.hooks[post_hook](option)) + if post_hook in hooks: + option.values.update(hooks[post_hook](option)) answers.update(option.values) context.update(option.values) @@ -1088,7 +1084,7 @@ def ask_questions_and_parse_answers( raw_options: dict[str, Any], prefilled_answers: Union[str, Mapping[str, Any]] = {}, current_values: Mapping[str, Any] = {}, - hooks: Dict[str, Callable[[], None]] = {}, + hooks: Hooks = {}, ) -> list[BaseOption]: """Parse arguments store in either manifest.json or actions.json or from a config panel against the user answers when they are present. From e87f8ef93a417b4287c6bed3a88210b0be196569 Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 15:10:23 +0200 Subject: [PATCH 09/11] form: use Enum for Option's type --- src/utils/configpanel.py | 25 ++++--- src/utils/form.py | 154 ++++++++++++++++++++++++++------------- 2 files changed, 118 insertions(+), 61 deletions(-) diff --git a/src/utils/configpanel.py b/src/utils/configpanel.py index 2914ae11f..f5d802356 100644 --- a/src/utils/configpanel.py +++ b/src/utils/configpanel.py @@ -34,6 +34,7 @@ from yunohost.utils.form import ( BaseInputOption, BaseOption, FileOption, + OptionType, ask_questions_and_parse_answers, evaluate_simple_js_expression, ) @@ -148,7 +149,7 @@ class ConfigPanel: if mode == "full": option["ask"] = ask - question_class = OPTIONS[option.get("type", "string")] + question_class = OPTIONS[option.get("type", OptionType.string)] # FIXME : maybe other properties should be taken from the question, not just choices ?. if issubclass(question_class, BaseChoicesOption): option["choices"] = question_class(option).choices @@ -158,7 +159,7 @@ class ConfigPanel: else: result[key] = {"ask": ask} if "current_value" in option: - question_class = OPTIONS[option.get("type", "string")] + question_class = OPTIONS[option.get("type", OptionType.string)] result[key]["value"] = question_class.humanize( option["current_value"], option ) @@ -243,7 +244,7 @@ class ConfigPanel: self.filter_key = "" self._get_config_panel() for panel, section, option in self._iterate(): - if option["type"] == "button": + if option["type"] == OptionType.button: key = f"{panel['id']}.{section['id']}.{option['id']}" actions[key] = _value_for_locale(option["ask"]) @@ -425,7 +426,7 @@ class ConfigPanel: subnode["name"] = key # legacy subnode.setdefault("optional", raw_infos.get("optional", True)) # If this section contains at least one button, it becomes an "action" section - if subnode.get("type") == "button": + if subnode.get("type") == OptionType.button: out["is_action_section"] = True out.setdefault(sublevel, []).append(subnode) # Key/value are a property @@ -500,13 +501,13 @@ class ConfigPanel: # Hydrating config panel with current value for _, section, option in self._iterate(): if option["id"] not in self.values: - allowed_empty_types = [ - "alert", - "display_text", - "markdown", - "file", - "button", - ] + allowed_empty_types = { + OptionType.alert, + OptionType.display_text, + OptionType.markdown, + OptionType.file, + OptionType.button, + } if section["is_action_section"] and option.get("default") is not None: self.values[option["id"]] = option["default"] @@ -587,7 +588,7 @@ class ConfigPanel: section["options"] = [ option for option in section["options"] - if option.get("type", "string") != "button" + if option.get("type", OptionType.string) != OptionType.button or option["id"] == action ] diff --git a/src/utils/form.py b/src/utils/form.py index b455fe812..62750657b 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -23,7 +23,8 @@ import re import shutil import tempfile import urllib.parse -from typing import Any, Callable, Dict, List, Mapping, Optional, Union +from enum import Enum +from typing import Any, Callable, Dict, List, Literal, Mapping, Optional, Union from moulinette import Moulinette, m18n from moulinette.interfaces.cli import colorize @@ -193,7 +194,50 @@ def evaluate_simple_js_expression(expr, context={}): # │ ╰─╯╵ ╵ ╶┴╴╰─╯╵╰╯╶─╯ │ # ╰───────────────────────────────────────────────────────╯ -FORBIDDEN_READONLY_TYPES = {"password", "app", "domain", "user", "group"} + +class OptionType(str, Enum): + # display + display_text = "display_text" + markdown = "markdown" + alert = "alert" + # action + button = "button" + # text + string = "string" + text = "text" + password = "password" + color = "color" + # numeric + number = "number" + range = "range" + # boolean + boolean = "boolean" + # time + date = "date" + time = "time" + # location + email = "email" + path = "path" + url = "url" + # file + file = "file" + # choice + select = "select" + tags = "tags" + # entity + domain = "domain" + app = "app" + user = "user" + group = "group" + + +FORBIDDEN_READONLY_TYPES = { + OptionType.password, + OptionType.app, + OptionType.domain, + OptionType.user, + OptionType.group, +} class BaseOption: @@ -202,7 +246,7 @@ class BaseOption: question: Dict[str, Any], ): self.name = question["name"] - self.type = question.get("type", "string") + self.type = question.get("type", OptionType.string) self.visible = question.get("visible", True) self.readonly = question.get("readonly", False) @@ -240,15 +284,15 @@ class BaseReadonlyOption(BaseOption): class DisplayTextOption(BaseReadonlyOption): - argument_type = "display_text" + type: Literal[OptionType.display_text] = OptionType.display_text class MarkdownOption(BaseReadonlyOption): - argument_type = "markdown" + type: Literal[OptionType.markdown] = OptionType.markdown class AlertOption(BaseReadonlyOption): - argument_type = "alert" + type: Literal[OptionType.alert] = OptionType.alert def __init__(self, question): super().__init__(question) @@ -271,7 +315,7 @@ class AlertOption(BaseReadonlyOption): class ButtonOption(BaseReadonlyOption): - argument_type = "button" + type: Literal[OptionType.button] = OptionType.button enabled = True def __init__(self, question): @@ -373,14 +417,21 @@ class BaseInputOption(BaseOption): # ─ STRINGS ─────────────────────────────────────────────── -class StringOption(BaseInputOption): - argument_type = "string" +class BaseStringOption(BaseInputOption): default_value = "" +class StringOption(BaseStringOption): + type: Literal[OptionType.string] = OptionType.string + + +class TextOption(BaseStringOption): + type: Literal[OptionType.text] = OptionType.text + + class PasswordOption(BaseInputOption): + type: Literal[OptionType.password] = OptionType.password hide_user_input_in_prompt = True - argument_type = "password" default_value = "" forbidden_chars = "{}" @@ -407,7 +458,8 @@ class PasswordOption(BaseInputOption): assert_password_is_strong_enough("user", self.value) -class ColorOption(StringOption): +class ColorOption(BaseStringOption): + type: Literal[OptionType.color] = OptionType.color pattern = { "regexp": r"^#[ABCDEFabcdef\d]{3,6}$", "error": "config_validate_color", # i18n: config_validate_color @@ -418,7 +470,7 @@ class ColorOption(StringOption): class NumberOption(BaseInputOption): - argument_type = "number" + type: Literal[OptionType.number, OptionType.range] = OptionType.number default_value = None def __init__(self, question): @@ -472,7 +524,7 @@ class NumberOption(BaseInputOption): class BooleanOption(BaseInputOption): - argument_type = "boolean" + type: Literal[OptionType.boolean] = OptionType.boolean default_value = 0 yes_answers = ["1", "yes", "y", "true", "t", "on"] no_answers = ["0", "no", "n", "false", "f", "off"] @@ -562,7 +614,8 @@ class BooleanOption(BaseInputOption): # ─ TIME ────────────────────────────────────────────────── -class DateOption(StringOption): +class DateOption(BaseStringOption): + type: Literal[OptionType.date] = OptionType.date pattern = { "regexp": r"^\d{4}-\d\d-\d\d$", "error": "config_validate_date", # i18n: config_validate_date @@ -580,7 +633,8 @@ class DateOption(StringOption): raise YunohostValidationError("config_validate_date") -class TimeOption(StringOption): +class TimeOption(BaseStringOption): + type: Literal[OptionType.time] = OptionType.time pattern = { "regexp": r"^(?:\d|[01]\d|2[0-3]):[0-5]\d$", "error": "config_validate_time", # i18n: config_validate_time @@ -590,7 +644,8 @@ class TimeOption(StringOption): # ─ LOCATIONS ───────────────────────────────────────────── -class EmailOption(StringOption): +class EmailOption(BaseStringOption): + type: Literal[OptionType.email] = OptionType.email pattern = { "regexp": r"^.+@.+", "error": "config_validate_email", # i18n: config_validate_email @@ -598,7 +653,7 @@ class EmailOption(StringOption): class WebPathOption(BaseInputOption): - argument_type = "path" + type: Literal[OptionType.path] = OptionType.path default_value = "" @staticmethod @@ -628,7 +683,8 @@ class WebPathOption(BaseInputOption): return "/" + value.strip().strip(" /") -class URLOption(StringOption): +class URLOption(BaseStringOption): + type: Literal[OptionType.url] = OptionType.url pattern = { "regexp": r"^https?://.*$", "error": "config_validate_url", # i18n: config_validate_url @@ -639,7 +695,7 @@ class URLOption(StringOption): class FileOption(BaseInputOption): - argument_type = "file" + type: Literal[OptionType.file] = OptionType.file upload_dirs: List[str] = [] def __init__(self, question): @@ -764,12 +820,12 @@ class BaseChoicesOption(BaseInputOption): class SelectOption(BaseChoicesOption): - argument_type = "select" + type: Literal[OptionType.select] = OptionType.select default_value = "" class TagsOption(BaseChoicesOption): - argument_type = "tags" + type: Literal[OptionType.tags] = OptionType.tags default_value = "" @staticmethod @@ -822,7 +878,7 @@ class TagsOption(BaseChoicesOption): class DomainOption(BaseChoicesOption): - argument_type = "domain" + type: Literal[OptionType.domain] = OptionType.domain def __init__(self, question): from yunohost.domain import domain_list, _get_maindomain @@ -851,7 +907,7 @@ class DomainOption(BaseChoicesOption): class AppOption(BaseChoicesOption): - argument_type = "app" + type: Literal[OptionType.app] = OptionType.app def __init__(self, question): from yunohost.app import app_list @@ -877,7 +933,7 @@ class AppOption(BaseChoicesOption): class UserOption(BaseChoicesOption): - argument_type = "user" + type: Literal[OptionType.user] = OptionType.user def __init__(self, question): from yunohost.user import user_list, user_info @@ -908,7 +964,7 @@ class UserOption(BaseChoicesOption): class GroupOption(BaseChoicesOption): - argument_type = "group" + type: Literal[OptionType.group] = OptionType.group def __init__(self, question): from yunohost.user import user_group_list @@ -932,29 +988,29 @@ class GroupOption(BaseChoicesOption): OPTIONS = { - "display_text": DisplayTextOption, - "markdown": MarkdownOption, - "alert": AlertOption, - "button": ButtonOption, - "string": StringOption, - "text": StringOption, - "password": PasswordOption, - "color": ColorOption, - "number": NumberOption, - "range": NumberOption, - "boolean": BooleanOption, - "date": DateOption, - "time": TimeOption, - "email": EmailOption, - "path": WebPathOption, - "url": URLOption, - "file": FileOption, - "select": SelectOption, - "tags": TagsOption, - "domain": DomainOption, - "app": AppOption, - "user": UserOption, - "group": GroupOption, + OptionType.display_text: DisplayTextOption, + OptionType.markdown: MarkdownOption, + OptionType.alert: AlertOption, + OptionType.button: ButtonOption, + OptionType.string: StringOption, + OptionType.text: StringOption, + OptionType.password: PasswordOption, + OptionType.color: ColorOption, + OptionType.number: NumberOption, + OptionType.range: NumberOption, + OptionType.boolean: BooleanOption, + OptionType.date: DateOption, + OptionType.time: TimeOption, + OptionType.email: EmailOption, + OptionType.path: WebPathOption, + OptionType.url: URLOption, + OptionType.file: FileOption, + OptionType.select: SelectOption, + OptionType.tags: TagsOption, + OptionType.domain: DomainOption, + OptionType.app: AppOption, + OptionType.user: UserOption, + OptionType.group: GroupOption, } @@ -1122,7 +1178,7 @@ def hydrate_questions_with_choices(raw_questions: List) -> List: out = [] for raw_question in raw_questions: - question = OPTIONS[raw_question.get("type", "string")](raw_question) + question = OPTIONS[raw_question.get("type", OptionType.string)](raw_question) if isinstance(question, BaseChoicesOption) and question.choices: raw_question["choices"] = question.choices raw_question["default"] = question.default From c1f0ac04c7597467b8f179fdaaff30d36f3ae0ce Mon Sep 17 00:00:00 2001 From: axolotle Date: Thu, 13 Apr 2023 15:54:56 +0200 Subject: [PATCH 10/11] rename Option.name to Option.id --- src/app.py | 20 +++++++------- src/tests/test_questions.py | 10 +++---- src/utils/configpanel.py | 6 ++--- src/utils/form.py | 54 ++++++++++++++++++------------------- 4 files changed, 45 insertions(+), 45 deletions(-) diff --git a/src/app.py b/src/app.py index 97227ed0c..c64a5d860 100644 --- a/src/app.py +++ b/src/app.py @@ -1099,7 +1099,7 @@ def app_install( raw_questions = manifest["install"] questions = ask_questions_and_parse_answers(raw_questions, prefilled_answers=args) args = { - question.name: question.value + question.id: question.value for question in questions if question.value is not None } @@ -1147,7 +1147,7 @@ def app_install( if question.type == "password": continue - app_settings[question.name] = question.value + app_settings[question.id] = question.value _set_app_settings(app_instance_name, app_settings) @@ -1202,16 +1202,16 @@ def app_install( # Reinject user-provider passwords which are not in the app settings # (cf a few line before) if question.type == "password": - env_dict[question.name] = question.value + env_dict[question.id] = question.value # We want to hav the env_dict in the log ... but not password values env_dict_for_logging = env_dict.copy() for question in questions: # Or should it be more generally question.redact ? if question.type == "password": - del env_dict_for_logging[f"YNH_APP_ARG_{question.name.upper()}"] - if question.name in env_dict_for_logging: - del env_dict_for_logging[question.name] + del env_dict_for_logging[f"YNH_APP_ARG_{question.id.upper()}"] + if question.id in env_dict_for_logging: + del env_dict_for_logging[question.id] operation_logger.extra.update({"env": env_dict_for_logging}) @@ -2358,17 +2358,17 @@ def _set_default_ask_questions(questions, script_name="install"): ), # i18n: app_manifest_install_ask_init_admin_permission ] - for question_name, question in questions.items(): - question["name"] = question_name + for question_id, question in questions.items(): + question["id"] = question_id # If this question corresponds to a question with default ask message... if any( - (question.get("type"), question["name"]) == question_with_default + (question.get("type"), question["id"]) == question_with_default for question_with_default in questions_with_default ): # The key is for example "app_manifest_install_ask_domain" question["ask"] = m18n.n( - f"app_manifest_{script_name}_ask_{question['name']}" + f"app_manifest_{script_name}_ask_{question['id']}" ) # Also it in fact doesn't make sense for any of those questions to have an example value nor a default value... diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 3e7927dce..7ef678d19 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -33,7 +33,7 @@ from yunohost.utils.error import YunohostError, YunohostValidationError """ Argument default format: { - "the_name": { + "the_id": { "type": "one_of_the_available_type", // "sting" is not specified "ask": { "en": "the question in english", @@ -50,7 +50,7 @@ Argument default format: } User answers: -{"the_name": "value", ...} +{"the_id": "value", ...} """ @@ -443,7 +443,7 @@ class BaseTest: assert isinstance(option, OPTIONS[raw_option["type"]]) assert option.type == raw_option["type"] - assert option.name == id_ + assert option.id == id_ assert option.ask == {"en": id_} assert option.readonly is (True if is_special_readonly_option else False) assert option.visible is True @@ -1913,7 +1913,7 @@ def test_options_query_string(): ) def _assert_correct_values(options, raw_options): - form = {option.name: option.value for option in options} + form = {option.id: option.value for option in options} for k, v in results.items(): if k == "file_id": @@ -1945,7 +1945,7 @@ def test_question_string_default_type(): out = ask_questions_and_parse_answers(questions, answers)[0] - assert out.name == "some_string" + assert out.id == "some_string" assert out.type == "string" assert out.value == "some_value" diff --git a/src/utils/configpanel.py b/src/utils/configpanel.py index f5d802356..42a030cbc 100644 --- a/src/utils/configpanel.py +++ b/src/utils/configpanel.py @@ -521,7 +521,7 @@ class ConfigPanel: f"Config panel question '{option['id']}' should be initialized with a value during install or upgrade.", raw_msg=True, ) - value = self.values[option["name"]] + value = self.values[option["id"]] # Allow to use value instead of current_value in app config script. # e.g. apps may write `echo 'value: "foobar"'` in the config file (which is more intuitive that `echo 'current_value: "foobar"'` @@ -600,14 +600,14 @@ class ConfigPanel: prefilled_answers.update(self.new_values) questions = ask_questions_and_parse_answers( - {question["name"]: question for question in section["options"]}, + {question["id"]: question for question in section["options"]}, prefilled_answers=prefilled_answers, current_values=self.values, hooks=self.hooks, ) self.new_values.update( { - question.name: question.value + question.id: question.value for question in questions if question.value is not None } diff --git a/src/utils/form.py b/src/utils/form.py index 62750657b..57cb1cd5b 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -245,7 +245,7 @@ class BaseOption: self, question: Dict[str, Any], ): - self.name = question["name"] + self.id = question["id"] self.type = question.get("type", OptionType.string) self.visible = question.get("visible", True) @@ -255,10 +255,10 @@ class BaseOption: raise YunohostError( "config_forbidden_readonly_type", type=self.type, - id=self.name, + id=self.id, ) - self.ask = question.get("ask", self.name) + self.ask = question.get("ask", self.id) if not isinstance(self.ask, dict): self.ask = {"en": self.ask} @@ -379,14 +379,14 @@ class BaseInputOption(BaseOption): def _value_pre_validator(self): if self.value in [None, ""] and not self.optional: - raise YunohostValidationError("app_argument_required", name=self.name) + raise YunohostValidationError("app_argument_required", name=self.id) # we have an answer, do some post checks if self.value not in [None, ""]: if self.pattern and not re.match(self.pattern["regexp"], str(self.value)): raise YunohostValidationError( self.pattern["error"], - name=self.name, + name=self.id, value=self.value, ) @@ -440,7 +440,7 @@ class PasswordOption(BaseInputOption): self.redact = True if self.default is not None: raise YunohostValidationError( - "app_argument_password_no_default", name=self.name + "app_argument_password_no_default", name=self.id ) def _value_pre_validator(self): @@ -496,7 +496,7 @@ class NumberOption(BaseInputOption): option = option.__dict__ if isinstance(option, BaseOption) else option raise YunohostValidationError( "app_argument_invalid", - name=option.get("name"), + name=option.get("id"), error=m18n.n("invalid_number"), ) @@ -508,14 +508,14 @@ class NumberOption(BaseInputOption): if self.min is not None and int(self.value) < self.min: raise YunohostValidationError( "app_argument_invalid", - name=self.name, + name=self.id, error=m18n.n("invalid_number_min", min=self.min), ) if self.max is not None and int(self.value) > self.max: raise YunohostValidationError( "app_argument_invalid", - name=self.name, + name=self.id, error=m18n.n("invalid_number_max", max=self.max), ) @@ -554,7 +554,7 @@ class BooleanOption(BaseInputOption): raise YunohostValidationError( "app_argument_choice_invalid", - name=option.get("name"), + name=option.get("id"), value=value, choices="yes/no", ) @@ -594,7 +594,7 @@ class BooleanOption(BaseInputOption): raise YunohostValidationError( "app_argument_choice_invalid", - name=option.get("name"), + name=option.get("id"), value=strvalue, choices="yes/no", ) @@ -663,7 +663,7 @@ class WebPathOption(BaseInputOption): if not isinstance(value, str): raise YunohostValidationError( "app_argument_invalid", - name=option.get("name"), + name=option.get("id"), error="Argument for path should be a string.", ) @@ -676,7 +676,7 @@ class WebPathOption(BaseInputOption): elif option.get("optional") is False: raise YunohostValidationError( "app_argument_invalid", - name=option.get("name"), + name=option.get("id"), error="Option is mandatory", ) @@ -725,7 +725,7 @@ class FileOption(BaseInputOption): ): raise YunohostValidationError( "app_argument_invalid", - name=self.name, + name=self.id, error=m18n.n("file_does_not_exist", path=str(self.value)), ) @@ -740,7 +740,7 @@ class FileOption(BaseInputOption): FileOption.upload_dirs += [upload_dir] - logger.debug(f"Saving file {self.name} for file question into {file_path}") + logger.debug(f"Saving file {self.id} for file question into {file_path}") def is_file_path(s): return isinstance(s, str) and s.startswith("/") and os.path.exists(s) @@ -813,7 +813,7 @@ class BaseChoicesOption(BaseInputOption): if self.choices and self.value not in self.choices: raise YunohostValidationError( "app_argument_choice_invalid", - name=self.name, + name=self.id, value=self.value, choices=", ".join(str(choice) for choice in self.choices), ) @@ -853,13 +853,13 @@ class TagsOption(BaseChoicesOption): if self.choices: raise YunohostValidationError( "app_argument_choice_invalid", - name=self.name, + name=self.id, value=self.value, choices=", ".join(str(choice) for choice in self.choices), ) raise YunohostValidationError( "app_argument_invalid", - name=self.name, + name=self.id, error=f"'{str(self.value)}' is not a list", ) @@ -949,7 +949,7 @@ class UserOption(BaseChoicesOption): if not self.choices: raise YunohostValidationError( "app_argument_invalid", - name=self.name, + name=self.id, error="You should create a YunoHost user first.", ) @@ -1033,9 +1033,9 @@ def prompt_or_validate_form( options = [] answers = {**prefilled_answers} - for name, raw_option in raw_options.items(): - raw_option["name"] = name - raw_option["value"] = answers.get(name) + for id_, raw_option in raw_options.items(): + raw_option["id"] = id_ + raw_option["value"] = answers.get(id_) question_class = OPTIONS[raw_option.get("type", "string")] option = question_class(raw_option) @@ -1047,7 +1047,7 @@ def prompt_or_validate_form( else: raise YunohostValidationError( "config_action_disabled", - action=option.name, + action=option.id, help=_value_for_locale(option.help), ) @@ -1060,7 +1060,7 @@ def prompt_or_validate_form( # - we doesn't want to give a specific value # - we want to keep the previous value # - we want the default value - option.value = context[option.name] = None + option.value = context[option.id] = None continue @@ -1071,7 +1071,7 @@ def prompt_or_validate_form( Moulinette.display(message) if isinstance(option, BaseInputOption): - option.value = context[option.name] = option.current_value + option.value = context[option.id] = option.current_value continue @@ -1123,10 +1123,10 @@ def prompt_or_validate_form( break - option.value = option.values[option.name] = option._value_post_validator() + option.value = option.values[option.id] = option._value_post_validator() # Search for post actions in hooks - post_hook = f"post_ask__{option.name}" + post_hook = f"post_ask__{option.id}" if post_hook in hooks: option.values.update(hooks[post_hook](option)) From 4df7e4681dcdca089a269bb9bb63ce6355b19896 Mon Sep 17 00:00:00 2001 From: axolotle Date: Sun, 30 Apr 2023 17:15:40 +0200 Subject: [PATCH 11/11] form: force option type to 'select' if there's 'choices' + add test --- src/tests/test_questions.py | 15 +++++++++++++++ src/utils/form.py | 23 ++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/tests/test_questions.py b/src/tests/test_questions.py index 7ef678d19..7737c4546 100644 --- a/src/tests/test_questions.py +++ b/src/tests/test_questions.py @@ -1950,6 +1950,21 @@ def test_question_string_default_type(): assert out.value == "some_value" +def test_option_default_type_with_choices_is_select(): + questions = { + "some_choices": {"choices": ["a", "b"]}, + # LEGACY (`choices` in option `string` used to be valid) + # make sure this result as a `select` option + "some_legacy": {"type": "string", "choices": ["a", "b"]} + } + answers = {"some_choices": "a", "some_legacy": "a"} + + options = ask_questions_and_parse_answers(questions, answers) + for option in options: + assert option.type == "select" + assert option.value == "a" + + @pytest.mark.skip # we should do something with this example def test_question_string_input_test_ask_with_example(): ask_text = "some question" diff --git a/src/utils/form.py b/src/utils/form.py index 57cb1cd5b..1ca03373e 100644 --- a/src/utils/form.py +++ b/src/utils/form.py @@ -1014,6 +1014,22 @@ OPTIONS = { } +def hydrate_option_type(raw_option: dict[str, Any]) -> dict[str, Any]: + type_ = raw_option.get( + "type", OptionType.select if "choices" in raw_option else OptionType.string + ) + # LEGACY (`choices` in option `string` used to be valid) + if "choices" in raw_option and type_ == OptionType.string: + logger.warning( + f"Packagers: option {raw_option['id']} has 'choices' but has type 'string', use 'select' instead to remove this warning." + ) + type_ = OptionType.select + + raw_option["type"] = type_ + + return raw_option + + # ╭───────────────────────────────────────────────────────╮ # │ ╷ ╷╶┬╴╶┬╴╷ ╭─╴ │ # │ │ │ │ │ │ ╰─╮ │ @@ -1036,8 +1052,8 @@ def prompt_or_validate_form( for id_, raw_option in raw_options.items(): raw_option["id"] = id_ raw_option["value"] = answers.get(id_) - question_class = OPTIONS[raw_option.get("type", "string")] - option = question_class(raw_option) + raw_option = hydrate_option_type(raw_option) + option = OPTIONS[raw_option["type"]](raw_option) interactive = Moulinette.interface.type == "cli" and os.isatty(1) @@ -1178,7 +1194,8 @@ def hydrate_questions_with_choices(raw_questions: List) -> List: out = [] for raw_question in raw_questions: - question = OPTIONS[raw_question.get("type", OptionType.string)](raw_question) + raw_question = hydrate_option_type(raw_question) + question = OPTIONS[raw_question["type"]](raw_question) if isinstance(question, BaseChoicesOption) and question.choices: raw_question["choices"] = question.choices raw_question["default"] = question.default