Hmpf refactor moar stuff but does this ever ends

src/yunohost/app.py

@@ -34,6 +34,7 @@ import subprocess
 import glob
 import tempfile
 from collections import OrderedDict
+from typing import List
 
 from moulinette import Moulinette, m18n
 from moulinette.core import MoulinetteError
@@ -55,6 +56,7 @@ from yunohost.utils import packages
 from yunohost.utils.config import (
     ConfigPanel,
     ask_questions_and_parse_answers,
+    Question,
     DomainQuestion,
     PathQuestion
 )
@@ -899,11 +901,13 @@ def app_install(
         app_instance_name = app_id
 
     # Retrieve arguments list for install script
-    questions = manifest.get("arguments", {}).get("install", {})
+    raw_questions = manifest.get("arguments", {}).get("install", {})
-    args = ask_questions_and_parse_answers(questions, prefilled_answers=args)
+    questions = ask_questions_and_parse_answers(raw_questions, prefilled_answers=args)
+    args = {question.name: question.value for question in questions if question.value is not None}
 
     # Validate domain / path availability for webapps
-    _validate_and_normalize_webpath(args, extracted_app_folder)
+    path_requirement = _guess_webapp_path_requirement(questions, extracted_app_folder)
+    _validate_webpath_requirement(questions, path_requirement)
 
     # Attempt to patch legacy helpers ...
     _patch_legacy_helpers(extracted_app_folder)
@@ -972,9 +976,10 @@ def app_install(
     env_dict["YNH_APP_BASEDIR"] = extracted_app_folder
 
     env_dict_for_logging = env_dict.copy()
-    for arg_name, arg_value_and_type in args.items():
+    for question in questions:
-        if arg_value_and_type[1] == "password":
+        # Or should it be more generally question.redact ?
-            del env_dict_for_logging["YNH_APP_ARG_%s" % arg_name.upper()]
+        if question.type == "password":
+            del env_dict_for_logging["YNH_APP_ARG_%s" % question.name.upper()]
 
     operation_logger.extra.update({"env": env_dict_for_logging})
 
@@ -1635,8 +1640,9 @@ def app_action_run(operation_logger, app, action, args=None):
     action_declaration = actions[action]
 
     # Retrieve arguments list for install script
-    questions = actions[action].get("arguments", {})
+    raw_questions = actions[action].get("arguments", {})
-    args = ask_questions_and_parse_answers(questions, prefilled_answers=args)
+    questions = ask_questions_and_parse_answers(raw_questions, prefilled_answers=args)
+    args = {question.name: question.value for question in questions if question.value is not None}
 
     tmp_workdir_for_app = _make_tmp_workdir_for_app(app=app)
 
@@ -2371,35 +2377,20 @@ def _check_manifest_requirements(manifest, app_instance_name):
             )
 
 
-def _validate_and_normalize_webpath(args_dict, app_folder):
+def _guess_webapp_path_requirement(questions: List[Question], app_folder: str) -> str:
 
     # If there's only one "domain" and "path", validate that domain/path
     # is an available url and normalize the path.
 
-    domain_args = [
+    domain_questions = [question for question in questions if question.type == "domain"]
-        (name, value[0]) for name, value in args_dict.items() if value[1] == "domain"
+    path_questions = [question for question in questions if question.type == "path"]
-    ]
-    path_args = [
-        (name, value[0]) for name, value in args_dict.items() if value[1] == "path"
-    ]
 
-    if len(domain_args) == 1 and len(path_args) == 1:
+    if len(domain_questions) == 0 and len(path_questions) == 0:
-
+        return None
-        domain = domain_args[0][1]
+    if len(domain_questions) == 1 and len(path_questions) == 1:
-        path = path_args[0][1]
+        return "domain_and_path"
-
+    if len(domain_questions) == 1 and len(path_questions) == 0:
-        domain = DomainQuestion.normalize(domain)
+        # This is likely to be a full-domain app...
-        path = PathQuestion.normalize(path)
-
-        # Check the url is available
-        _assert_no_conflicting_apps(domain, path)
-
-        # (We save this normalized path so that the install script have a
-        # standard path format to deal with no matter what the user inputted)
-        args_dict[path_args[0][0]] = (path, "path")
-
-    # This is likely to be a full-domain app...
-    elif len(domain_args) == 1 and len(path_args) == 0:
 
         # Confirm that this is a full-domain app This should cover most cases
         # ...  though anyway the proper solution is to implement some mechanism
@@ -2409,18 +2400,33 @@ def _validate_and_normalize_webpath(args_dict, app_folder):
 
         # Full-domain apps typically declare something like path_url="/" or path=/
         # and use ynh_webpath_register or yunohost_app_checkurl inside the install script
-        install_script_content = open(
+        install_script_content = read_file(os.path.join(app_folder, "scripts/install"))
-            os.path.join(app_folder, "scripts/install")
-        ).read()
 
         if re.search(
-            r"\npath(_url)?=[\"']?/[\"']?\n", install_script_content
+            r"\npath(_url)?=[\"']?/[\"']?", install_script_content
         ) and re.search(
-            r"(ynh_webpath_register|yunohost app checkurl)", install_script_content
+            r"ynh_webpath_register", install_script_content
         ):
+            return "full_domain"
 
-            domain = domain_args[0][1]
+    return "?"
-            _assert_no_conflicting_apps(domain, "/", full_domain=True)
+
+
+def _validate_webpath_requirement(questions: List[Question], path_requirement: str) -> None:
+
+    domain_questions = [question for question in questions if question.type == "domain"]
+    path_questions = [question for question in questions if question.type == "path"]
+
+    if path_requirement == "domain_and_path":
+
+        domain = domain_questions[0].value
+        path = path_questions[0].value
+        _assert_no_conflicting_apps(domain, path, full_domain=True)
+
+    elif path_requirement == "full_domain":
+
+        domain = domain_questions[0].value
+        _assert_no_conflicting_apps(domain, "/", full_domain=True)
 
 
 def _get_conflicting_apps(domain, path, ignore_app=None):
@@ -2499,10 +2505,8 @@ def _make_environment_for_app_script(app, args={}, args_prefix="APP_ARG_"):
         "YNH_APP_MANIFEST_VERSION": manifest.get("version", "?"),
     }
 
-    for arg_name, arg_value_and_type in args.items():
+    for arg_name, arg_value in args.items():
-        env_dict["YNH_%s%s" % (args_prefix, arg_name.upper())] = str(
+        env_dict["YNH_%s%s" % (args_prefix, arg_name.upper())] = str(arg_value)
-            arg_value_and_type[0]
-        )
 
     return env_dict
 

src/yunohost/tests/test_app_config.py

@@ -2,9 +2,11 @@ import glob
 import os
 import shutil
 import pytest
+from mock import patch
 
 from .conftest import get_test_apps_dir
 
+from moulinette import Moulinette
 from moulinette.utils.filesystem import read_file
 
 from yunohost.domain import _get_maindomain
@@ -146,7 +148,9 @@ def test_app_config_regular_setting(config_app):
     assert app_config_get(config_app, "main.components.boolean") == "1"
     assert app_setting(config_app, "boolean") == "1"
 
-    with pytest.raises(YunohostValidationError):
+    with pytest.raises(YunohostValidationError), \
+         patch.object(os, "isatty", return_value=False), \
+         patch.object(Moulinette, "prompt", return_value="pwet"):
         app_config_set(config_app, "main.components.boolean", "pwet")
 
 

src/yunohost/utils/config.py

@@ -380,14 +380,13 @@ class ConfigPanel:
                 display_header(f"\n# {name}")
 
             # Check and ask unanswered questions
-            self.new_values.update(
+            questions = ask_questions_and_parse_answers(section["options"], self.args)
-                ask_questions_and_parse_answers(section["options"], self.args)
+            self.new_values.update({
-            )
+                question.name: question.value
-        self.new_values = {
+                for question in questions
-            key: value[0]
+                if question.value is not None
-            for key, value in self.new_values.items()
+            })
-            if not value[0] is None
+
-        }
         self.errors = None
 
     def _get_default_values(self):
@@ -538,9 +537,10 @@ class Question(object):
                 raise
 
             break
+
         self.value = self._post_parse_value()
 
-        return (self.value, self.argument_type)
+        return self.value
 
     def _prevalidate(self):
         if self.value in [None, ""] and not self.optional:
@@ -1058,7 +1058,7 @@ ARGUMENTS_TYPE_PARSERS = {
 }
 
 
-def ask_questions_and_parse_answers(questions: Dict, prefilled_answers: Union[str, Mapping[str, Any]] = {}) -> Mapping[str, Any]:
+def ask_questions_and_parse_answers(questions: Dict, prefilled_answers: Union[str, Mapping[str, Any]] = {}) -> List[Question]:
     """Parse arguments store in either manifest.json or actions.json or from a
     config panel against the user answers when they are present.
 
@@ -1071,17 +1071,24 @@ def ask_questions_and_parse_answers(questions: Dict, prefilled_answers: Union[st
     """
 
     if isinstance(prefilled_answers, str):
-        prefilled_answers = dict(urllib.parse.parse_qs(prefilled_answers or "", keep_blank_values=True))
+        # FIXME FIXME : this is not uniform with config_set() which uses parse.qs (no l)
+        # parse_qsl parse single values
+        # whereas parse.qs return list of values (which is useful for tags, etc)
+        # For now, let's not migrate this piece of code to parse_qs
+        # Because Aleks believes some bits of the app CI rely on overriding values (e.g. foo=foo&...&foo=bar)
+        prefilled_answers = dict(urllib.parse.parse_qsl(prefilled_answers or "", keep_blank_values=True))
 
-    out = OrderedDict()
+    if not prefilled_answers:
+        prefilled_answers = {}
+
+    out = []
 
     for question in questions:
         question_class = ARGUMENTS_TYPE_PARSERS[question.get("type", "string")]
         question["value"] = prefilled_answers.get(question["name"])
         question = question_class(question)
 
-        answer = question.ask_if_needed()
+        question.ask_if_needed()
-        if answer is not None:
+        out.append(question)
-            out[question.name] = answer
 
     return out