#1778 frontend: use correct auto_prune default when creating via API
Merged 2 years ago by frostyx. Opened 2 years ago by frostyx.
copr/ frostyx/copr wrong-defaults-in-api  into  main

@@ -195,3 +195,36 @@ 

              )

          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

@@ -1,6 +1,7 @@ 

  import flask

- from . import query_params, get_copr, pagination, Paginator, GET, POST, PUT, DELETE

- 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
@@ -121,7 +122,9 @@ 

  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 @@ 

              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,

@@ -66,10 +66,10 @@ 

  

      @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 @@ 

          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/"],

Fix #1747

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/<NAME>.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

1 new commit added

  • frontend: fix unrelated pylint warnings
2 years ago

Build succeeded.

I do not object against this PR... But there's the get_form_compatible_data method that calls without_empty_fields. Can it be related? If not, I'm +1.

But there's the get_form_compatible_data method that calls without_empty_fields. Can it be related?

I just tested it and it doesn't seem to be related. This takes the user input directly, without any modifications like without_empty_fields.

ipdb> form = form_class(MultiDict(flask.request.json), meta={'csrf': False})                                           
ipdb> form.auto_prune.data                                 
False

Metadata Update from @frostyx:
- Pull-request tagged with: wip

2 years ago

rebased onto 95c82c8121f34d2e7d8d482a22ef6b6074052a84

2 years ago

Is this fixing #1746 as well ?

Indeed it does, I updated the commit description. Thank you for pointing that out.

Metadata Update from @frostyx:
- Pull-request untagged with: wip

2 years ago

Build succeeded.

Thank you for the review, merging.

rebased onto c01e5be

2 years ago

Pull-Request has been merged by frostyx

2 years ago

Build succeeded.