🗃️ [#2041] Added hard database constraints on User's identifier/login_type fields #1074

stevenbal · 2024-03-04T13:55:10Z

Based on #990

src/open_inwoner/accounts/models.py

codecov-commenter · 2024-03-11T16:23:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.04%. Comparing base (761f3e2) to head (39a45be).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1074      +/-   ##
===========================================
+ Coverage    95.03%   95.04%   +0.01%     
===========================================
  Files          911      913       +2     
  Lines        32038    32124      +86     
===========================================
+ Hits         30446    30532      +86     
  Misses        1592     1592

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

src/open_inwoner/accounts/models.py

src/open_inwoner/openklant/tests/data.py

pi-sigma

Not sure if something needs to be done about DuplicateEmailRegistrationTest; take a quick look to confirm.

src/open_inwoner/accounts/tests/test_user_constraints.py

pi-sigma · 2024-03-13T08:23:44Z

src/open_inwoner/cms/cases/tests/test_contactform.py

@@ -442,6 +443,9 @@ def test_form_success_with_api_eherkenning_user(self, m):
            with self.subTest(
                use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter
            ):
+                # FIXME for some reason creating the user outside of the loop
+                # makes the second iteration fail?
+                User.objects.filter(kvk="12345678").delete()


Same/similar issue as in https://github.com/maykinmedia/open-inwoner/blob/develop/src/open_inwoner/accounts/tests/test_profile_views.py#L547, where the mock has to be recreated because it is overriden somehow? Something's up with our mock machinery that needs to be fixed, though not sure if this can be done as part of your ticket; perhaps create a new one for this.

Agree these delete()'s are a bit smelly.

Still unsure why only the second iteration failed: apparently it wasn't due to the mocks, but it's fixed if I give the User the same email as the klant created in the mock data? 🙈

Bartvaderkin

Some small notes, and I wonder if we covered the eenmanszaken situation from an earlier comment (if we always have kvk and rsin, or if eenmanszaken can have a bsn but not kvk or rsin).

src/open_inwoner/accounts/tests/test_user_constraints.py

src/open_inwoner/accounts/tests/test_profile_views.py

Bartvaderkin · 2024-03-18T09:50:26Z

src/open_inwoner/accounts/tests/test_profile_views.py

+        # reset noise from signals
+        m.reset_mock()
+        self.clearTimelineLogs()


We don't need these lines anymore as they were there to reset the subtests.

I removed the reset_mock, but clearTimelineLogs is actually still needed (it's also used in other tests in this file that don't have subtests)

Existing code may or may not be perfect.

Could be cool to see why it happens but if you tested it fails if we remove it I'll believe it (could be because the code earlier in the tests generates similar logs)

Bartvaderkin · 2024-03-18T10:52:41Z

src/open_inwoner/accounts/tests/test_profile_views.py

+        # reset noise from signals
+        m.reset_mock()
+        self.clearTimelineLogs()


Same as above

Bartvaderkin · 2024-03-19T09:09:31Z

@stevenbal Are we sure about the check_bsn_only_set_when_login_digid rule? In relation to the eherkenning & eenmanszaken from earlier?

stevenbal · 2024-03-19T09:15:55Z

Are we sure about the check_bsn_only_set_when_login_digid rule? In relation to the eherkenning & eenmanszaken from earlier?

I'll double check with Alex, apparently Groningen registers eenmanszaken bij KVK number and vestigingsnummer, but if we only allow that, it would still not work for eenmanszaken if we use Open Zaak. In case of Open Zaak we might have to force users that want to log in for an eenmanszaak to log in with DigiD

alextreme · 2024-04-15T08:42:18Z

The suggestion (OpenZaak + RSIN + eHerkenning -> dont allow eenmanszaken to log in via eHerkenning) is sufficient for now and is already in the project. For eenmanszaken BSN == RSIN, so logging in via DigiD would give them the same information.

…pe fields

stevenbal · 2024-04-15T14:41:47Z

@Bartvaderkin I rebased this on master, can you re-review/approve this? :)

alextreme · 2024-07-08T12:24:27Z

@swrichards could you take a look at this PR somewhere in the following weeks? No rush but now it is languishing

swrichards · 2024-07-09T08:09:33Z

@swrichards could you take a look at this PR somewhere in the following weeks? No rush but now it is languishing

👍

stevenbal marked this pull request as draft March 4, 2024 13:55

alextreme added the release 1.15 label Mar 11, 2024

stevenbal commented Mar 11, 2024

View reviewed changes

src/open_inwoner/accounts/models.py Outdated Show resolved Hide resolved

stevenbal marked this pull request as ready for review March 11, 2024 15:49

stevenbal force-pushed the fix/2041-user-db-constraints branch from 8b236ff to edc7d43 Compare March 11, 2024 15:50

stevenbal requested review from Bartvaderkin and pi-sigma March 11, 2024 15:50

Bartvaderkin suggested changes Mar 12, 2024

View reviewed changes

src/open_inwoner/accounts/models.py Outdated Show resolved Hide resolved

src/open_inwoner/openklant/tests/data.py Outdated Show resolved Hide resolved

stevenbal marked this pull request as draft March 12, 2024 15:06

pi-sigma requested changes Mar 13, 2024

View reviewed changes

Bartvaderkin removed the release 1.15 label Mar 15, 2024

stevenbal force-pushed the fix/2041-user-db-constraints branch from edc7d43 to 7048b7f Compare March 15, 2024 08:41

stevenbal marked this pull request as ready for review March 15, 2024 08:41

stevenbal requested review from pi-sigma and Bartvaderkin March 15, 2024 08:43

stevenbal force-pushed the fix/2041-user-db-constraints branch from 7048b7f to 8f3f271 Compare March 15, 2024 10:21

pi-sigma approved these changes Mar 15, 2024

View reviewed changes

Bartvaderkin suggested changes Mar 15, 2024

View reviewed changes

src/open_inwoner/accounts/tests/test_user_constraints.py Outdated Show resolved Hide resolved

src/open_inwoner/accounts/tests/test_profile_views.py Outdated Show resolved Hide resolved

stevenbal force-pushed the fix/2041-user-db-constraints branch from 8f3f271 to 67c28a6 Compare March 15, 2024 14:07

stevenbal requested a review from Bartvaderkin March 15, 2024 14:07

stevenbal force-pushed the fix/2041-user-db-constraints branch from 67c28a6 to 5508218 Compare March 18, 2024 07:57

Bartvaderkin suggested changes Mar 18, 2024

View reviewed changes

stevenbal force-pushed the fix/2041-user-db-constraints branch from 5508218 to 39a45be Compare March 18, 2024 11:13

stevenbal requested a review from Bartvaderkin March 18, 2024 11:13

alextreme added release 1.16 and removed release 1.16 labels Apr 15, 2024

Bart van der Schoor and others added 3 commits April 15, 2024 15:27

[#2041] Added hard database constraints on User's identifier/login_ty…

b07716b

…pe fields

🔀 [#2041] Fixes after rebase

ec550ab

✅ [#2041] Fix tests for user DB constraints

ed82dc7

stevenbal marked this pull request as draft April 15, 2024 13:37

👌 [#2041] PR feedback

2b8b97c

stevenbal force-pushed the fix/2041-user-db-constraints branch from 39a45be to 2b8b97c Compare April 15, 2024 13:52

stevenbal marked this pull request as ready for review April 15, 2024 13:53

alextreme requested review from swrichards and removed request for Bartvaderkin May 31, 2024 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🗃️ [#2041] Added hard database constraints on User's identifier/login_type fields #1074

🗃️ [#2041] Added hard database constraints on User's identifier/login_type fields #1074

stevenbal commented Mar 4, 2024 •

edited

Loading

codecov-commenter commented Mar 11, 2024 •

edited

Loading

pi-sigma left a comment

pi-sigma Mar 13, 2024

Bartvaderkin Mar 14, 2024

stevenbal Mar 15, 2024 •

edited

Loading

Bartvaderkin left a comment

Bartvaderkin Mar 18, 2024

stevenbal Mar 18, 2024

Bartvaderkin Mar 18, 2024

Bartvaderkin Mar 18, 2024

Bartvaderkin commented Mar 19, 2024

stevenbal commented Mar 19, 2024 •

edited

Loading

alextreme commented Apr 15, 2024

stevenbal commented Apr 15, 2024

alextreme commented Jul 8, 2024

swrichards commented Jul 9, 2024

🗃️ [#2041] Added hard database constraints on User's identifier/login_type fields #1074

Are you sure you want to change the base?

🗃️ [#2041] Added hard database constraints on User's identifier/login_type fields #1074

Conversation

stevenbal commented Mar 4, 2024 • edited Loading

codecov-commenter commented Mar 11, 2024 • edited Loading

Codecov Report

pi-sigma left a comment

Choose a reason for hiding this comment

pi-sigma Mar 13, 2024

Choose a reason for hiding this comment

Bartvaderkin Mar 14, 2024

Choose a reason for hiding this comment

stevenbal Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Bartvaderkin left a comment

Choose a reason for hiding this comment

Bartvaderkin Mar 18, 2024

Choose a reason for hiding this comment

stevenbal Mar 18, 2024

Choose a reason for hiding this comment

Bartvaderkin Mar 18, 2024

Choose a reason for hiding this comment

Bartvaderkin Mar 18, 2024

Choose a reason for hiding this comment

Bartvaderkin commented Mar 19, 2024

stevenbal commented Mar 19, 2024 • edited Loading

alextreme commented Apr 15, 2024

stevenbal commented Apr 15, 2024

alextreme commented Jul 8, 2024

swrichards commented Jul 9, 2024

stevenbal commented Mar 4, 2024 •

edited

Loading

codecov-commenter commented Mar 11, 2024 •

edited

Loading

stevenbal Mar 15, 2024 •

edited

Loading

stevenbal commented Mar 19, 2024 •

edited

Loading