Removing single backend registration config from form #4867

robinmolen · 2024-11-28T14:16:07Z

Part of #3283

Changes

The deprecated fields registration_backend and registration_backend_options have been removed. The field registration_backends should now be used in all situations.

These fields have also been removed from the import and export. This means, when you import a form using the registration_backend config, the registration will no longer be converted to registration_backends and the form will be imported without registration configurations.

Checklist

Check off the items that are completed or not relevant.

Impact on features
- Checked copying a form
- Checked import/export of a form
- Config checks in the configuration overview admin page
- Problem detection in the admin email digest is handled
Release management
- I have labelled the PR as "needs-backport" accordingly
I have updated the translations assets (you do NOT need to provide translations)
- Ran ./bin/makemessages_js.sh
- Ran ./bin/compilemessages_js.sh
Commit hygiene
- Commit messages refer to the relevant Github issue
- Commit messages explain the "why" of change, not the how

codecov · 2024-11-28T15:38:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (f678921) to head (9bdf87c).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4867      +/-   ##
==========================================
- Coverage   96.61%   96.60%   -0.02%     
==========================================
  Files         752      752              
  Lines       25848    25809      -39     
  Branches     3422     3411      -11     
==========================================
- Hits        24974    24932      -42     
- Misses        612      613       +1     
- Partials      262      264       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/installation/upgrade-300.rst

vaszig

I am a bit confused here and maybe I am missing something..

I see registration_backend and registration_backend_options are still used in a lot of places, shouldn't we remove them from everywhere now that we deprecated these two? There are factories as well that make use of them so they need updating and then the tests themselves.

Like here for instance:
https://github.com/open-formulieren/open-forms/blob/master/src/openforms/forms/tests/factories.py#L50

https://github.com/open-formulieren/open-forms/blob/master/src/openforms/registrations/contrib/email/tests/test_email_rendering.py#L14

**And a general comment..It helps a lot if you add next to the subtask (in cases like meta ticket 3283) the link for the pull request

vaszig · 2024-12-04T13:08:59Z

src/openforms/forms/tests/test_copy.py

@@ -33,7 +33,10 @@ def test_form_copy_with_reusable_definition(self):
        self.assertNotEqual(copied_form.pk, form.pk)
        self.assertNotEqual(copied_form.uuid, str(form.uuid))
        self.assertEqual(copied_form.active, form.active)
-        self.assertEqual(copied_form.registration_backend, form.registration_backend)
+        self.assertEqual(


Maybe we should test the exact backends and not the number of them(since we are testing the copy functionality)?Like we do with all the assertions above.

I've done a number comparison because both have 0 backends :)
But yeah, its a bit ambiguous. I'll make it more explicit 👍

let's make it very clear what the expected result is, both the old and new assertion confuse me in what the actual initial and final state are

vaszig · 2024-12-04T13:09:25Z

src/openforms/forms/tests/test_import_export.py

        self.assertEqual(
-            imported_form.registration_backend_options,
-            form.registration_backend_options,
+            imported_form.registration_backends.count(),


I would say the same like the comment for the copy.

robinmolen · 2024-12-04T13:51:17Z

I see registration_backend and registration_backend_options are still used in a lot of places, shouldn't we remove them from everywhere now that we deprecated these two? There are factories as well that make use of them so they need updating and then the tests themselves.

Yeah this is something i discussed with Sergei, keeping the registration_backend and registration_backend_options in the FormFactory, to keep the registration simple for the test. But yeah, maybe best to completely remove this way of working.. I'll take a crack at it :)

**And a general comment..It helps a lot if you add next to the subtask (in cases like meta ticket 3283) the link for the pull request

Ah check, yeah will do 👍

sergei-maertens · 2024-12-04T18:09:50Z

src/openforms/submissions/admin.py

@@ -361,7 +361,7 @@ def successfully_processed(self, obj) -> bool | None:

    @admin.display(description=_("Registration backend"))
    def get_registration_backend(self, obj):
-        return obj.form.registration_backend or "-"
+        return backend if (backend := obj.form.registration_backends.first()) else "-"


looks like this was bugged in the old code, it should really use submission.registration_backend so it takes into account the dynamic backend selection from logic rules (!)

Check, the obj.registration_backend is now used for this admin display. I've also added a test that makes sure that the admin correctly shows the registration_backend of the submission

sergei-maertens · 2024-12-04T18:15:02Z

I see registration_backend and registration_backend_options are still used in a lot of places, shouldn't we remove them from everywhere now that we deprecated these two? There are factories as well that make use of them so they need updating and then the tests themselves.

Yeah this is something i discussed with Sergei, keeping the registration_backend and registration_backend_options in the FormFactory, to keep the registration simple for the test. But yeah, maybe best to completely remove this way of working.. I'll take a crack at it :)

**And a general comment..It helps a lot if you add next to the subtask (in cases like meta ticket 3283) the link for the pull request

Ah check, yeah will do 👍

quick search on master branch for registration_backend= usage in test_*.py files:

90 matches across 17 files

which will turn the currently one liner into more lines because a separate factory needs to be called. On a PR with 11 changed files, I expect this to make the PR twice as large. IMO the convenience in the factory (which is precisely the reason why we use factories) is not a problem, it's just a shortcut, like how SubmissionFactory(auth_info__identifier="123") is a shortcut that makes the tests a lot more readable and maintainable.

So, I suggest not to rip this out of the factories.

The deprecated fields `registration_backend` and `registration_backend_options` have been removed. The field `registration_backends` should now be used in all situations. These fields have also been removed from the import and export. This means, when you import a form using the `registration_backend` config, the registration will no longer be converted to `registration_backends`.

sergei-maertens · 2024-12-05T14:33:11Z

src/openforms/submissions/tests/form_logic/test_submission_logic.py

@@ -369,6 +372,114 @@ def test_check_logic_hide_with_default_value(self):
            data["step"]["formStep"]["configuration"]["components"][0]["hidden"],
        )

+    def test_check_logic_with_registration_backend_correctly_displayed_in_admin(self):


can you please put the tests in src/openforms/submissions/tests/test_admin.py with the rest of the admin tests, and use webtest?

These tests here are tests for API endpoints

Hmm yeah i wasn't sure about the placement of this test.

On the one hand, it is about the admin, so should be placed with the admin tests. On the other hand, this test heavily focuses on the form logic, and the effects it has..

But I'll move it to the test_admin 👍

On the one hand, it is about the admin, so should be placed with the admin tests. On the other hand, this test heavily focuses on the form logic, and the effects it has..

It's testing what is displayed on the admin page and that's what the initial comment was about :-) that the logic engine needs to be used is an implementation detail, but the original test case was APITestCase and that's definitely the wrong location for admin tests 😛

The submission admin display used to show the first registration_backend of the form. Due to form logic, the registration_backend on the submission could be different. The admin now shows the exact registration_backend that was used by the submission

Stale

robinmolen force-pushed the task/3283-removing-form-single-registration branch 2 times, most recently from d23397b to 0ddb6e4 Compare November 28, 2024 15:30

sergei-maertens reviewed Nov 28, 2024

View reviewed changes

docs/installation/upgrade-300.rst Outdated Show resolved Hide resolved

robinmolen force-pushed the task/3283-removing-form-single-registration branch from 0ddb6e4 to 0bfa4db Compare December 2, 2024 11:26

robinmolen marked this pull request as ready for review December 2, 2024 11:52

robinmolen force-pushed the task/3283-removing-form-single-registration branch 2 times, most recently from 9200ae3 to f519038 Compare December 2, 2024 13:47

robinmolen requested a review from vaszig December 4, 2024 10:05

vaszig previously requested changes Dec 4, 2024

View reviewed changes

sergei-maertens reviewed Dec 4, 2024

View reviewed changes

robinmolen force-pushed the task/3283-removing-form-single-registration branch 2 times, most recently from e1b4020 to e4afb5e Compare December 5, 2024 07:48

robinmolen force-pushed the task/3283-removing-form-single-registration branch from f808ca8 to eddead9 Compare December 5, 2024 14:28

sergei-maertens requested changes Dec 5, 2024

View reviewed changes

robinmolen force-pushed the task/3283-removing-form-single-registration branch from eddead9 to af94e5c Compare December 5, 2024 14:46

sergei-maertens approved these changes Dec 5, 2024

View reviewed changes

robinmolen force-pushed the task/3283-removing-form-single-registration branch 2 times, most recently from 8b4f7b9 to 48ad130 Compare December 5, 2024 15:44

robinmolen force-pushed the task/3283-removing-form-single-registration branch from 48ad130 to 9bdf87c Compare December 5, 2024 16:01

sergei-maertens merged commit d1513d0 into master Dec 5, 2024
33 of 34 checks passed

sergei-maertens deleted the task/3283-removing-form-single-registration branch December 5, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing single backend registration config from form #4867

Removing single backend registration config from form #4867

robinmolen commented Nov 28, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

vaszig left a comment •

edited

Loading

vaszig Dec 4, 2024

robinmolen Dec 4, 2024

sergei-maertens Dec 4, 2024

vaszig Dec 4, 2024

robinmolen commented Dec 4, 2024

sergei-maertens Dec 4, 2024

robinmolen Dec 5, 2024

sergei-maertens commented Dec 4, 2024

sergei-maertens Dec 5, 2024

robinmolen Dec 5, 2024 •

edited

Loading

robinmolen Dec 5, 2024

sergei-maertens Dec 5, 2024

Removing single backend registration config from form #4867

Removing single backend registration config from form #4867

Conversation

robinmolen commented Nov 28, 2024

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

vaszig left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinmolen commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-maertens commented Dec 4, 2024

Choose a reason for hiding this comment

robinmolen Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2024 •

edited

Loading

vaszig left a comment •

edited

Loading

robinmolen Dec 5, 2024 •

edited

Loading