Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2638] Implement choice of zaken notification channel #1336

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Aug 7, 2024

  • User can choose the channel for case notifications (post + digital or digital only) when registering
  • User can edit the channel for case notifications (post + digital or digital only) in their profile
  • Staff users can enable/disable the choice of notification channel via the admin

The default is post + digital.

As discussed, this is an MVP; styling of the radio boxes needs touch up to conform with design.

Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2638

@pi-sigma pi-sigma changed the title [#2638] zaken notifications choice [#2638] Implement choice of zaken notification channel Aug 7, 2024
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch from 9664b3d to 3f7ef37 Compare August 7, 2024 09:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 98.38710% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.12%. Comparing base (8f22d47) to head (204dbdb).

Files Patch % Lines
src/open_inwoner/accounts/views/mixins.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop    #1336    +/-   ##
=========================================
  Coverage    95.11%   95.12%            
=========================================
  Files          995      998     +3     
  Lines        36472    36572   +100     
=========================================
+ Hits         34692    34790    +98     
- Misses        1780     1782     +2     

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

@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch 8 times, most recently from 0c97f04 to 4526a58 Compare August 8, 2024 14:00
@pi-sigma
Copy link
Contributor Author

pi-sigma commented Aug 8, 2024

@swrichards

  • I'm updating the klanten api by overwriting the get method of the CustomDigiDAssertionConsumerServiceView. Not entirely happy with this, as I had to copy over the code from the parent class and add the relevant function. I would have preferred to do this closer to where the user is created, but this is done by the DigiDBackend ouf our digid-eherkenning library. Suggestions welcome.
  • I'm using the same method update_klant_api in different forms. Could be extracted since the code is the same, but I wanted to get some feedback first. Also, I wasn't sure what the best place would be to put this.
  • The styling of the radio forms does not conform to design yet (it's missing the icons, and the radio buttons are aligned vertically instead of horizontally). Not sure if it's worth creating a separate component for this use case, or if updating our existing radio component is the way to go. Could also use input from @jiromaykin on this.

@pi-sigma pi-sigma marked this pull request as ready for review August 8, 2024 14:10
@pi-sigma pi-sigma requested a review from swrichards August 8, 2024 14:10
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch from 2822684 to 74911be Compare August 9, 2024 12:57
@swrichards
Copy link
Contributor

  • The styling of the radio forms does not conform to design yet (it's missing the icons, and the radio buttons are aligned vertically instead of horizontally). Not sure if it's worth creating a separate component for this use case, or if updating our existing radio component is the way to go. Could also use input from @jiromaykin on this.

Replied to the other points in separate threads, but for posterity: we agreed that this point would be handled separately in a future story.

src/open_inwoner/accounts/views/auth.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/components/templatetags/form_tags.py Outdated Show resolved Hide resolved
src/open_inwoner/openklant/api_models.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch 2 times, most recently from 80700f6 to c528f2f Compare August 13, 2024 08:39
@pi-sigma pi-sigma requested a review from swrichards August 13, 2024 09:09
src/open_inwoner/accounts/views/signals.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/mixins.py Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/registration.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/registration.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch 2 times, most recently from d2278ca to 1e2141d Compare August 13, 2024 09:53
src/open_inwoner/accounts/views/registration.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/profile.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/views/signals.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch from 1e2141d to 1992d6a Compare August 13, 2024 12:13
@pi-sigma pi-sigma requested a review from swrichards August 13, 2024 13:34
Copy link
Contributor

@swrichards swrichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one more suggestion for the signal function, but it's not a blocker: if you don't get around to it, happy to merge as-is.

src/open_inwoner/accounts/views/signals.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch from 1992d6a to d4da772 Compare August 13, 2024 14:23
@swrichards swrichards self-requested a review August 13, 2024 15:43
@swrichards swrichards requested a review from alextreme August 13, 2024 15:43
@alextreme
Copy link
Member

This is fine for municipalities that make use of the eSuite Klanten API, nice work on that.

However, MG (see comment from Marieke in the design) doesn't use the eSuite Klanten API and also doesn't have a process interally for not sending updates via snailmail.

This case needs to be taken into account, otherwise citizens will check 'digitaal only' but still receive their updates via post.

@pi-sigma pi-sigma marked this pull request as draft August 14, 2024 10:39
@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch 2 times, most recently from b451091 to 3a591fb Compare August 14, 2024 14:28
@alextreme alextreme requested a review from swrichards August 14, 2024 15:08
@pi-sigma pi-sigma marked this pull request as ready for review August 15, 2024 06:51
@alextreme
Copy link
Member

@swrichards see if you can re-review this PR, it's set to approved however new changes were necessary as discussed

@pi-sigma pi-sigma force-pushed the task/2638-zaken-notifications-choice branch from 3a591fb to 204dbdb Compare August 15, 2024 10:53
@pi-sigma pi-sigma requested a review from swrichards August 15, 2024 11:39
@alextreme alextreme merged commit b08b824 into develop Aug 15, 2024
18 checks passed
@alextreme alextreme deleted the task/2638-zaken-notifications-choice branch August 15, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants