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

[#1052-4]Feature/ haalcentraal contact data #430

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Jan 17, 2023

In this PR I added an extra check (is_prepopulated=True, which is True when we use BRP in combination with DigiD) in order to disable the fields and show Mijn gegevens to the user.
If we want the form fields disabled generally with DigiD I will have to update this accordingly.

@vaszig vaszig marked this pull request as draft January 17, 2023 16:05
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #430 (7376162) into develop (6993c5e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #430      +/-   ##
===========================================
+ Coverage    96.28%   96.29%   +0.01%     
===========================================
  Files          469      470       +1     
  Lines        15596    15640      +44     
===========================================
+ Hits         15017    15061      +44     
  Misses         579      579              
Impacted Files Coverage Δ
src/open_inwoner/accounts/forms.py 97.96% <100.00%> (+0.06%) ⬆️
...oner/accounts/migrations/0053_user_display_name.py 100.00% <100.00%> (ø)
src/open_inwoner/accounts/models.py 97.78% <100.00%> (+<0.01%) ⬆️
.../open_inwoner/accounts/tests/test_profile_views.py 100.00% <100.00%> (ø)
src/open_inwoner/accounts/views/profile.py 99.13% <100.00%> (+0.03%) ⬆️

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

@vaszig vaszig force-pushed the feature/1052-haalcentraal-contact-data branch from 36a8bb1 to 88172a4 Compare January 18, 2023 10:54
@vaszig vaszig force-pushed the feature/1052-haalcentraal-contact-data branch from 88172a4 to ecce86d Compare January 18, 2023 14:50
@vaszig vaszig marked this pull request as ready for review January 18, 2023 16:07
@alextreme
Copy link
Member

General idea is good, however thinking about the edit-profile view there isn't much point in showing all the disabled fields if the User is prepopulated, and the opt-out structure feels a bit brittle (easy to add new fields to the form that might be made editable by accident).

How about we go for 2 forms: one where the User is prepopulated and one where this isn't the case. The former would only contain the fields that are actually editable.

@vaszig
Copy link
Contributor Author

vaszig commented Jan 19, 2023

@alextreme I agree, the only reason I did this in one form was because the task is about making specific fields readonly. Now that we are going for two separate forms this does not seem to be needed.

@vaszig vaszig requested review from Bartvaderkin, annashamray and alextreme and removed request for annashamray January 19, 2023 08:58
Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

This is fine. Was there a separate task for updating the NecessaryUserForm?

@vaszig
Copy link
Contributor Author

vaszig commented Jan 19, 2023

This is fine. Was there a separate task for updating the NecessaryUserForm?

Yes, this is the task (#1055) that I am working now.

@alextreme alextreme merged commit 41062e3 into develop Jan 19, 2023
@alextreme alextreme deleted the feature/1052-haalcentraal-contact-data branch January 19, 2023 18:41
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.

3 participants