[#897] Feature/contacts approval flow #363

vaszig · 2022-12-06T14:36:12Z

No description provided.

codecov-commenter · 2022-12-07T07:58:57Z

Codecov Report

Merging #363 (525843d) into develop (2ae8276) will increase coverage by 0.02%.
The diff coverage is 99.13%.

@@             Coverage Diff             @@
##           develop     #363      +/-   ##
===========================================
+ Coverage    96.48%   96.50%   +0.02%     
===========================================
  Files          450      457       +7     
  Lines        14034    14495     +461     
===========================================
+ Hits         13541    13989     +448     
- Misses         493      506      +13

Impacted Files	Coverage Δ
src/open_inwoner/accounts/urls.py	`100.00% <ø> (ø)`
src/open_inwoner/accounts/views/contacts.py	`99.00% <96.87%> (-1.00%)`	⬇️
src/open_inwoner/accounts/forms.py	`98.89% <100.00%> (+0.01%)`	⬆️
src/open_inwoner/accounts/managers.py	`89.36% <100.00%> (+0.47%)`	⬆️
src/open_inwoner/accounts/models.py	`97.77% <100.00%> (-0.87%)`	⬇️
.../open_inwoner/accounts/tests/test_contact_views.py	`100.00% <100.00%> (ø)`
src/open_inwoner/accounts/views/__init__.py	`100.00% <100.00%> (ø)`
src/open_inwoner/components/utils.py	`96.34% <0.00%> (-3.66%)`	⬇️
src/open_inwoner/accounts/choices.py	`97.05% <0.00%> (-2.95%)`	⬇️
... and 24 more

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

Bartvaderkin

I put my notes in the code.

I also noticed this that might existed before: maybe these buttons could be closer? The Accept button is really far from the Reject, and the Reject button label says Terug (Back)

src/open_inwoner/templates/export/include/contacts.html

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

src/open_inwoner/scss/components/Contacts/Contacts.scss

src/open_inwoner/accounts/forms.py

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

Bartvaderkin · 2022-12-07T13:56:44Z

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

+                    <li class="approval__list-item">
+                        <p>{{approval.get_full_name}}</p>
+                        {% url 'accounts:contact_approval' uuid=approval.uuid as approval_url %}
+                        {% render_form form=None method="POST" form_action=approval_url id="approval_form" spaceless=True show_notifications=True %}


ID-attribute must be unique so can't in a loop.

Bartvaderkin · 2022-12-07T14:00:41Z

src/open_inwoner/accounts/views/contacts.py

+        approved = request.POST.get("contact_approve")
+        rejected = request.POST.get("contact_reject")
+        if approved or rejected:
+            self.update_contact(sender, receiver, (approved or rejected))
+            return HttpResponseRedirect(self.success_url)
+
+    def update_contact(self, sender, receiver, type_of_approval):
+        if type_of_approval == "approve":
+            sender.contacts_for_approval.remove(receiver)
+            sender.user_contacts.add(receiver)
+            self.log_change(sender, _("contact was approved"))
+
+        elif type_of_approval == "reject":


I think it is convention to not use raw POST but always try to go for a Form, and use Choices instead of magic strings so validation is a bit harder and more regular.

The two buttons can have the same name in the HTML but different values and then the Form is just a CharField with the choices for the incoming data.

Also, looking at the post method: what happens if there is no approved or rejected? It could raise BadRequest since there is no HTML for this view.

Discussed this with Anna as well and I decided to go for the same approach as the one in deleting a contact. That's why you saw the ids in the for loop.

Ok, if it's same pattern as the other and Anna is ok with it then it's fine.

Let's wait for @annashamray 's approval as well because she hasn't seen this implemented.

I think there is a little misunderstanding.
I said that there is no need in adding additional forms into ContactListView context - we can just generate forms in the template since it's only csrf token, two buttons and 1 action. It's how the delete buttons for contacts are done.
Security-wise it should be ok, since we send csrf token.

But it's a different story with the view which process POST request (ContactApprovalView).
Here we can use forms to escape and validate user input, especially if we use values in buttons. We can use only button names like django admin does, and they do it without extra validation. I don't know is it a best practice or not.

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

Bartvaderkin · 2022-12-07T14:42:19Z

I forgot to mention this: there is a small CSS bug in the first header of the contacts table (probably from earlier):

vaszig · 2022-12-07T15:12:20Z

I put my notes in the code.

I also noticed this that might existed before: maybe these buttons could be closer? The Accept button is really far from the Reject, and the Reject button label says Terug (Back)

Yes, this was there before I touch the frontend!It seems weird but if @alextreme wants I can create a task for it.

vaszig · 2022-12-07T15:13:38Z

I forgot to mention this: there is a small CSS bug in the first header of the contacts table (probably from earlier):

Yes, there is a task in taiga about this. A change in the cases tables has broken all tables.

src/open_inwoner/accounts/models.py

alextreme · 2022-12-07T18:53:53Z

I put my notes in the code.
I also noticed this that might existed before: maybe these buttons could be closer? The Accept button is really far from the Reject, and the Reject button label says Terug (Back)

Yes, this was there before I touch the frontend!It seems weird but if @alextreme wants I can create a task for it.

Task #960 created, noticed it before so lets make this prettier.

Bartvaderkin

Just a few small notes.

src/open_inwoner/scss/components/Contacts/Contacts.scss

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

src/open_inwoner/accounts/views/contacts.py

annashamray

Nice! I'll approve after CI is happy

src/open_inwoner/accounts/views/contacts.py

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

src/open_inwoner/scss/components/Contacts/Contacts.scss

annashamray · 2022-12-09T18:38:08Z

src/open_inwoner/accounts/views/contacts.py

+        approved = request.POST.get("contact_approve")
+        rejected = request.POST.get("contact_reject")
+        if approved or rejected:
+            self.update_contact(sender, receiver, (approved or rejected))
+            return HttpResponseRedirect(self.success_url)
+
+    def update_contact(self, sender, receiver, type_of_approval):
+        if type_of_approval == "approve":
+            sender.contacts_for_approval.remove(receiver)
+            sender.user_contacts.add(receiver)
+            self.log_change(sender, _("contact was approved"))
+
+        elif type_of_approval == "reject":


I think there is a little misunderstanding.
I said that there is no need in adding additional forms into ContactListView context - we can just generate forms in the template since it's only csrf token, two buttons and 1 action. It's how the delete buttons for contacts are done.
Security-wise it should be ok, since we send csrf token.

But it's a different story with the view which process POST request (ContactApprovalView).
Here we can use forms to escape and validate user input, especially if we use values in buttons. We can use only button names like django admin does, and they do it without extra validation. I don't know is it a best practice or not.

alextreme · 2022-12-12T09:43:08Z

Removing @annashamray , for @Bartvaderkin to double-check after Vasileios's changes

Bartvaderkin · 2022-12-12T10:37:06Z

✔️ 👍

vaszig marked this pull request as draft December 6, 2022 14:58

vaszig force-pushed the task-897-add-contacts-approval branch from 0455693 to 9e3881e Compare December 7, 2022 07:43

vaszig marked this pull request as ready for review December 7, 2022 08:28

vaszig requested review from annashamray, svenvandescheur and Bartvaderkin December 7, 2022 08:29

Bartvaderkin suggested changes Dec 7, 2022

View reviewed changes

Bartvaderkin reviewed Dec 7, 2022

View reviewed changes

src/open_inwoner/templates/pages/profile/contacts/list.html Show resolved Hide resolved

vaszig commented Dec 7, 2022

View reviewed changes

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

vaszig requested a review from Bartvaderkin December 7, 2022 15:49

Bartvaderkin suggested changes Dec 8, 2022

View reviewed changes

vaszig force-pushed the task-897-add-contacts-approval branch from 4620695 to 9215625 Compare December 9, 2022 08:44

annashamray reviewed Dec 9, 2022

View reviewed changes

vaszig requested a review from annashamray December 12, 2022 08:35

alextreme removed the request for review from annashamray December 12, 2022 09:42

vaszig added 6 commits December 12, 2022 10:56

[897] Added contacts approval flow

4743bed

[#897] Added extra contact validation, fixed contacts export

2594f77

[#897] PR review fixes

3290d96

[#897] PR review fixes

bb58239

[#897] Added user custom queryset

7b45eb2

[#897] Improved template and fixing tests

9bbc49c

vaszig force-pushed the task-897-add-contacts-approval branch from 0f499d1 to 9bbc49c Compare December 12, 2022 10:00

Bartvaderkin approved these changes Dec 12, 2022

View reviewed changes

alextreme merged commit 8e7c465 into develop Dec 12, 2022

alextreme deleted the task-897-add-contacts-approval branch December 12, 2022 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#897] Feature/contacts approval flow #363

[#897] Feature/contacts approval flow #363

vaszig commented Dec 6, 2022 •

edited

Loading

codecov-commenter commented Dec 7, 2022 •

edited

Loading

Bartvaderkin left a comment

Bartvaderkin Dec 7, 2022

Bartvaderkin Dec 7, 2022

vaszig Dec 7, 2022

Bartvaderkin Dec 7, 2022

vaszig Dec 7, 2022

annashamray Dec 9, 2022

Bartvaderkin commented Dec 7, 2022

vaszig commented Dec 7, 2022

vaszig commented Dec 7, 2022

alextreme commented Dec 7, 2022

Bartvaderkin left a comment

annashamray left a comment

annashamray Dec 9, 2022

alextreme commented Dec 12, 2022

Bartvaderkin commented Dec 12, 2022

[#897] Feature/contacts approval flow #363

[#897] Feature/contacts approval flow #363

Conversation

vaszig commented Dec 6, 2022 • edited Loading

codecov-commenter commented Dec 7, 2022 • edited Loading

Codecov Report

Bartvaderkin left a comment

Choose a reason for hiding this comment

Bartvaderkin Dec 7, 2022

Choose a reason for hiding this comment

Bartvaderkin Dec 7, 2022

Choose a reason for hiding this comment

vaszig Dec 7, 2022

Choose a reason for hiding this comment

Bartvaderkin Dec 7, 2022

Choose a reason for hiding this comment

vaszig Dec 7, 2022

Choose a reason for hiding this comment

annashamray Dec 9, 2022

Choose a reason for hiding this comment

Bartvaderkin commented Dec 7, 2022

vaszig commented Dec 7, 2022

vaszig commented Dec 7, 2022

alextreme commented Dec 7, 2022

Bartvaderkin left a comment

Choose a reason for hiding this comment

annashamray left a comment

Choose a reason for hiding this comment

annashamray Dec 9, 2022

Choose a reason for hiding this comment

alextreme commented Dec 12, 2022

Bartvaderkin commented Dec 12, 2022

vaszig commented Dec 6, 2022 •

edited

Loading

codecov-commenter commented Dec 7, 2022 •

edited

Loading