[#929] Refactor contacts #348

vaszig · 2022-11-24T16:01:49Z

Removes Contact model
Adds user_contacts and contacts_for_approval (field for invitations) to the User model
Removes phonenumber from contact creation and the ability to modify a contact

src/open_inwoner/accounts/admin.py

src/open_inwoner/accounts/migrations/0048_auto_20221124_1515.py

alextreme · 2022-11-24T16:37:01Z

Nice refactor! Went over it early to ensure we're on the same page, think this is going in the right direction 👍

alextreme

Nice work! Big PR but this was imho as discussed, only a few minor comments

src/open_inwoner/accounts/views/contacts.py

src/open_inwoner/plans/admin.py

alextreme · 2022-11-29T12:24:49Z

src/open_inwoner/plans/managers.py

-        return self.filter(
-            Q(contacts__contact_user=user) | Q(contacts__created_by=user)
-        ).distinct()
+        return self.filter(Q(created_by=user) | Q(plan_contacts__id=user.id)).distinct()


Maybe it makes sense to auto-add the creating user also to plan_contacts?

Agree with Alex, it will simplify code in several places

vaszig · 2022-11-29T14:44:57Z

I have processed @alextreme 's reccomendations and I will push the changes as soon as the other guys have reviewed as well (I don't want to push new commits in case they have started their reviews).

@alextreme can I have a bit of explanation for the third comment?About adding the creating user to the plan_contacts

alextreme · 2022-11-29T15:25:56Z

@alextreme can I have a bit of explanation for the third comment?About adding the creating user to the plan_contacts

Well, if we want to simplify the Plans further we could make it so that the User creating the Plan is both registered in created_by and in the plan_contacts M2M. This way you no longer would need to check both the created_by FK and the plan_contacts M2M, you would only have to check plan_contacts.

Up to you, Bart and Anna to determine if this makes sense, but as we're refactoring the Plans significantly it would make sense to also do this if you three agree with me. Otherwise you can leave it as-is.

Bartvaderkin · 2022-11-29T16:31:07Z

@vaszig I haven't started on this today so I don't mind new commits.

codecov-commenter · 2022-11-30T07:39:53Z

Codecov Report

Merging #348 (b89f152) into develop (348165a) will increase coverage by 0.06%.
The diff coverage is 99.78%.

@@             Coverage Diff             @@
##           develop     #348      +/-   ##
===========================================
+ Coverage    96.38%   96.45%   +0.06%     
===========================================
  Files          442      451       +9     
  Lines        13917    13906      -11     
===========================================
- Hits         13414    13413       -1     
+ Misses         503      493      -10

Impacted Files	Coverage Δ
src/open_inwoner/accounts/managers.py	`88.88% <ø> (+2.35%)`	⬆️
src/open_inwoner/accounts/urls.py	`100.00% <ø> (ø)`
..._inwoner/plans/management/commands/plans_expire.py	`100.00% <ø> (ø)`
src/open_inwoner/utils/tests/test_migrations.py	`95.23% <95.23%> (ø)`
src/open_inwoner/accounts/admin.py	`92.06% <100.00%> (-0.59%)`	⬇️
src/open_inwoner/accounts/forms.py	`98.86% <100.00%> (+1.02%)`	⬆️
...ner/accounts/migrations/0047_auto_20221130_1609.py	`100.00% <100.00%> (ø)`
...ner/accounts/migrations/0048_auto_20221130_1610.py	`100.00% <100.00%> (ø)`
...ner/accounts/migrations/0049_auto_20221130_1614.py	`100.00% <100.00%> (ø)`
..._0047_action_is_deleted_0049_auto_20221130_1614.py	`100.00% <100.00%> (ø)`
... and 29 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Bartvaderkin

I've added some code style feedback.

But the most important thing I'm missing is a test for the significant data migration.

Here you can find a base class TestMigrations and some usage examples for a migration test: https://github.com/open-formulieren/open-forms/search?q=TestMigrations

Also I ran the migrations on my local database and I get some styling issues (but note the names were messy test data so the gaps might not mean anything)

src/open_inwoner/utils/tests/test_timeline_logger.py

src/open_inwoner/accounts/models.py

src/open_inwoner/accounts/query.py

src/open_inwoner/accounts/tests/test_auth.py

src/open_inwoner/templates/pages/profile/contacts/edit.html

src/open_inwoner/templates/pages/profile/contacts/list.html

src/open_inwoner/utils/tests/test_timeline_logger.py

src/open_inwoner/accounts/tests/test_contact_views.py

vaszig · 2022-11-30T16:08:43Z

Changes according to comments have been made
Tests for data migrations have been added but one is failing, I will investigate this with @Bartvaderkin
Concerning the styling, since we don't have a design for the creator's contactlist page, I am only showing the contacts' data (first name, last name) if they have been approved.

…g Contact

annashamray

Good job!
It's much neater and more readable when before! The simplified querysets have brought a joy to my heart :)

src/open_inwoner/accounts/models.py

src/open_inwoner/accounts/views/contacts.py

src/open_inwoner/accounts/models.py

src/open_inwoner/accounts/query.py

src/open_inwoner/utils/tests/test_migrations.py

annashamray · 2022-12-02T14:16:37Z

src/open_inwoner/plans/managers.py

-        return self.filter(
-            Q(contacts__contact_user=user) | Q(contacts__created_by=user)
-        ).distinct()
+        return self.filter(Q(created_by=user) | Q(plan_contacts__id=user.id)).distinct()


Agree with Alex, it will simplify code in several places

src/open_inwoner/plans/forms.py

src/open_inwoner/accounts/tests/test_contact_views.py

Bartvaderkin

Noticed this was waiting for me.

Comments have been resolved and this makes our life easier

vaszig marked this pull request as draft November 24, 2022 16:02

alextreme reviewed Nov 24, 2022

View reviewed changes

src/open_inwoner/accounts/admin.py Show resolved Hide resolved

alextreme reviewed Nov 24, 2022

View reviewed changes

src/open_inwoner/accounts/migrations/0048_auto_20221124_1515.py Outdated Show resolved Hide resolved

vaszig force-pushed the task-929-remove-contact-model branch 2 times, most recently from 194c656 to 42c61ff Compare November 29, 2022 11:49

vaszig marked this pull request as ready for review November 29, 2022 12:08

vaszig requested review from alextreme, annashamray and Bartvaderkin November 29, 2022 12:08

alextreme requested changes Nov 29, 2022

View reviewed changes

Bartvaderkin suggested changes Nov 30, 2022

View reviewed changes

vaszig changed the title ~~[#929] Refactor contacts~~ [WIP][#929] Refactor contacts Nov 30, 2022

vaszig added 9 commits December 1, 2022 07:55

[#929] Added necessary fields to User,Invite,Plan models, for removin…

17a99a8

…g Contact

[#929] Added contacts tests,started refactoring with the plans

eb11a0c

[#929] Refactoring after contact model removal

94d3bd4

[#929] Isorting

3752abc

[#929] Fixed inbox page tests

47808c5

[#929] Updated delete contact view

7eeb20f

[#929] Removed unnecessary admin field

50d6289

[#929] Processed PR review

20064f8

[#] Rebased on develop

8afd4f0

vaszig force-pushed the task-929-remove-contact-model branch from 098274b to 8afd4f0 Compare December 1, 2022 07:23

vaszig and others added 2 commits December 1, 2022 08:34

[#929] Fixed tests after rebase

b13c301

Fixed migration test issue

2394f1b

[#929] Removed redundant variables

b89f152

vaszig changed the title ~~[WIP][#929] Refactor contacts~~ [#929] Refactor contacts Dec 1, 2022

vaszig requested a review from Bartvaderkin December 1, 2022 09:45

annashamray mentioned this pull request Dec 2, 2022

[#899] Reselect template and contact on Plan-create validation error #349

Merged

annashamray previously requested changes Dec 2, 2022

View reviewed changes

alextreme approved these changes Dec 5, 2022

View reviewed changes

[929] PR review fixes

3558bf5

vaszig requested a review from annashamray December 6, 2022 10:30

vaszig mentioned this pull request Dec 6, 2022

[#897] Feature/contacts approval flow #363

Merged

Bartvaderkin approved these changes Dec 6, 2022

View reviewed changes

alextreme merged commit 14b8735 into develop Dec 6, 2022

alextreme deleted the task-929-remove-contact-model branch December 6, 2022 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#929] Refactor contacts #348

[#929] Refactor contacts #348

vaszig commented Nov 24, 2022 •

edited

Loading

alextreme commented Nov 24, 2022

alextreme left a comment

alextreme Nov 29, 2022

annashamray Dec 2, 2022

vaszig commented Nov 29, 2022

alextreme commented Nov 29, 2022

Bartvaderkin commented Nov 29, 2022

codecov-commenter commented Nov 30, 2022 •

edited

Loading

Bartvaderkin left a comment •

edited

Loading

vaszig commented Nov 30, 2022

annashamray left a comment

annashamray Dec 2, 2022

Bartvaderkin left a comment

[#929] Refactor contacts #348

[#929] Refactor contacts #348

Conversation

vaszig commented Nov 24, 2022 • edited Loading

alextreme commented Nov 24, 2022

alextreme left a comment

Choose a reason for hiding this comment

alextreme Nov 29, 2022

Choose a reason for hiding this comment

annashamray Dec 2, 2022

Choose a reason for hiding this comment

vaszig commented Nov 29, 2022

alextreme commented Nov 29, 2022

Bartvaderkin commented Nov 29, 2022

codecov-commenter commented Nov 30, 2022 • edited Loading

Codecov Report

Bartvaderkin left a comment • edited Loading

Choose a reason for hiding this comment

vaszig commented Nov 30, 2022

annashamray left a comment

Choose a reason for hiding this comment

annashamray Dec 2, 2022

Choose a reason for hiding this comment

Bartvaderkin left a comment

Choose a reason for hiding this comment

vaszig commented Nov 24, 2022 •

edited

Loading

codecov-commenter commented Nov 30, 2022 •

edited

Loading

Bartvaderkin left a comment •

edited

Loading