From c01e5be627690f1ac9a11b0c068a6438507c2a21 Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Apr 21 2021 15:22:31 +0000 Subject: [PATCH 1/2] frontend: use correct auto_prune default when creating via API Fix #1747 Fix #1746 The `test_update_copr_api3` test used web UI to create a new project. I changed it to API, which revealed that `follow_fedora_branching` and `runtime_dependencies` values weren't set at all, so I fixed it and updated the test. The behavior of `default` form values is terribly confusing to me, I described my observations in the `set_defaults` function. It might be wrong though. Also, please note that the #1747 issue cannot be reproduced via copr-cli create test-1 --chroot fedora-33-x86_64 and neither it can't be reproduced via from copr.v3 import Client Client.create_from_config_file() client.project_proxy.add("frostyx", "test-2", ["fedora-33-x86_64"]) because both of them send a complete `CoprForm` data. It can be reproduced only the way this test works, i.e. sending a subset of `CoprForm` to `/api_3/project/add/`. --- diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py index 61ae22a..5db8b0c 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py @@ -195,3 +195,36 @@ def editable_copr(f): ) return f(copr, **kwargs) return wrapper + + +def set_defaults(formdata, form_class): + """ + Take a `formdata` which can be `flask.request.form` or an output from + `get_form_compatible_data`, and `form_class` (not instance). This function + then goes through all fields defaults and update the `formdata` if those + fields weren't set from a user. + + I don't understand why this is necessary, I would expect a `form.foo.data` + to return the request value or `default` but it doesn't seem to work like + that. See the `wtforms.fields.Field` documentation: + + > default – The default value to assign to the field, if no form or object + > input is provided. May be a callable. + + The **if no form or object input is provided** is really unexpected. We + always initialize our forms with request data, i.e. the defaults are never + used? IMHO a more reasonable approach would be doing this per field. + + A field `default` value seems to be used only when rendering an empty form, + which is good enough for rendering an empty form into HTML (even though it + forces us to render all fields, which IMHO shouldn't be necessary). This + doesn't work at all for API where users send only a subset of form fields + and expect reasonable defaults for the rest. + """ + form = form_class(ImmutableMultiDict()) + for field in form: + if field.default is None: + continue + if field.name in formdata.keys(): + continue + formdata[field.name] = field.default diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py index f45f69b..57ef030 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py @@ -1,5 +1,6 @@ import flask -from . import query_params, get_copr, pagination, Paginator, GET, POST, PUT, DELETE +from . import (query_params, get_copr, pagination, Paginator, GET, POST, PUT, + DELETE, set_defaults) from .json2form import get_form_compatible_data, get_input_dict from coprs import db, models, forms from coprs.views.misc import api_login_required @@ -121,7 +122,9 @@ def search_projects(query, **kwargs): def add_project(ownername): user, group = owner2tuple(ownername) data = rename_fields(get_form_compatible_data(preserve=["chroots"])) - form = forms.CoprFormFactory.create_form_cls(user=user, group=group)(data, meta={'csrf': False}) + form_class = forms.CoprFormFactory.create_form_cls(user=user, group=group) + set_defaults(data, form_class) + form = form_class(data, meta={'csrf': False}) if not form.validate_on_submit(): raise BadRequest(form.errors) @@ -158,6 +161,8 @@ def add_project(ownername): multilib=form.multilib.data, module_hotfixes=form.module_hotfixes.data, fedora_review=form.fedora_review.data, + follow_fedora_branching=form.follow_fedora_branching.data, + runtime_dependencies=form.runtime_dependencies.data, ) db.session.commit() except (DuplicateException, diff --git a/frontend/coprs_frontend/tests/test_apiv3/test_projects.py b/frontend/coprs_frontend/tests/test_apiv3/test_projects.py index 39db723..91b9d8f 100644 --- a/frontend/coprs_frontend/tests/test_apiv3/test_projects.py +++ b/frontend/coprs_frontend/tests/test_apiv3/test_projects.py @@ -66,10 +66,10 @@ class TestApiv3Projects(CoprsTestCase): @pytest.mark.usefixtures("f_u1_ts_client", "f_mock_chroots", "f_db") def test_update_copr_api3(self): - self.web_ui.new_project("test", ["fedora-rawhide-i386"], - bootstrap="default", isolation="simple", - contact="somebody@redhat.com", - homepage="https://github.com/fedora-copr") + self.api3.new_project("test", ["fedora-rawhide-i386"], + bootstrap="default", isolation="simple", + contact="somebody@redhat.com", + homepage="https://github.com/fedora-copr") old_data = self._get_copr_id_data(1) # When new arguments are added to the Copr model, we should update this @@ -87,7 +87,8 @@ class TestApiv3Projects(CoprsTestCase): assert old_data["fedora_review"] is False assert old_data["repos"] == '' assert old_data["runtime_dependencies"] == "" - assert old_data["auto_prune"] is False # TODO: issue 1747 + assert old_data["auto_prune"] is True + assert old_data["follow_fedora_branching"] is True self.api3.modify_project( "test", delete_after_days=5, enable_net=True, devel_mode=True, repos=["http://example/repo/", "http://another/"], From bf066b4c5b21e19bd0d5b191f42be27f28e20a22 Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Apr 21 2021 15:22:31 +0000 Subject: [PATCH 2/2] frontend: fix unrelated pylint warnings --- diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py index 57ef030..69fd2d9 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py @@ -1,7 +1,7 @@ import flask -from . import (query_params, get_copr, pagination, Paginator, GET, POST, PUT, - DELETE, set_defaults) -from .json2form import get_form_compatible_data, get_input_dict +from coprs.views.apiv3_ns import (query_params, get_copr, pagination, Paginator, + GET, POST, PUT, DELETE, set_defaults) +from coprs.views.apiv3_ns.json2form import get_form_compatible_data, get_input_dict from coprs import db, models, forms from coprs.views.misc import api_login_required from coprs.views.apiv3_ns import apiv3_ns