form: restrict choices to select, tags, domain, app, user + group

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:
+            if isinstance(option, BaseChoicesOption):
-                choices = (
+                choices = option.choices
-                    option.choices
+                if choices:
-                    if isinstance(option.choices, list)
+                    expected_message += f" [{' | '.join(choices)}]"
-                    else option.choices.keys()
-                )
-                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"

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:
+            return f"{message} {self.humanize(self.current_value)}"
-                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 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)