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

[#2931] Update company data on login #1524

Closed
wants to merge 2 commits into from

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Dec 9, 2024

@pi-sigma pi-sigma force-pushed the task/2931-update-company-data-login branch from 5b7cd3a to c38c7b2 Compare December 9, 2024 14:36
@pi-sigma pi-sigma changed the title Update company data on login [#2931] Update company data on login Dec 9, 2024
@pi-sigma pi-sigma force-pushed the task/2931-update-company-data-login branch from c38c7b2 to 2a5012b Compare December 11, 2024 14:24
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.

Discussed off-line, we'll do this PR first before the selection screen skipper.

src/open_inwoner/kvk/views.py Outdated Show resolved Hide resolved
src/open_inwoner/kvk/views.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/signals.py Show resolved Hide resolved
src/open_inwoner/kvk/views.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/signals.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2931-update-company-data-login branch 2 times, most recently from 39d3cc6 to 557a8f2 Compare December 13, 2024 15:30
@pi-sigma pi-sigma marked this pull request as ready for review December 13, 2024 15:51
@pi-sigma pi-sigma requested a review from swrichards December 13, 2024 15:51
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.

A few issues to do with vestiging/kvk pairs, but maybe we can tackle that later, let's discuss on monday.

Comment on lines +43 to +55
fields = {
"company_name": vestiging.get("naam"),
"city": binnenlandsadres.get("plaats"),
"postcode": binnenlandsadres.get("postcode"),
"street": binnenlandsadres.get("straatnaam"),
"housenumber": binnenlandsadres.get("huisnummer"),
}

for field, new_value in fields.items():
current_value = getattr(user, field, None)
if new_value and new_value != current_value:
setattr(user, field, new_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure these checks have added value: the kvk is the source of truth here I think, we can just overwrite all the fields no (assuming there's a binnenlandsAdres)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't be enough to check for the presence of binnenlandAdres, because each field in binnenlandAdres is potentially null and would have to be checked as well. Some of the fields may actually be required (an address without at least city name would be strange), but I don't know which ones (the spec doesn't say). I find it simpler therefore to get the values from the dict and check if they are different from what's stored on the user.

@@ -122,6 +125,24 @@ def get_all_company_branches(self, kvk: str, **kwargs) -> list[dict | None]:

return branches

@cache("vestigingsnummer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@cache("vestigingsnummer")
@cache("vestiging:{vestigingsnummer}")

I am a bit concerned by the **kwargs though, I don't think you can currently add that as a parameter in the cache key, but that means you'll always get a cached result even if kwargs changes. Is there any way to actually name the kwargs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a use case because the only kwarg passed on is the vestigingsnummer. The code is misleading though; kwargs.update suggests that there is other stuff in there. I changed the function signature + body to make clear that we're only passing on the vestigingsnummer.

src/open_inwoner/kvk/client.py Outdated Show resolved Hide resolved
src/open_inwoner/kvk/views.py Show resolved Hide resolved
src/open_inwoner/kvk/tests/test_views.py Show resolved Hide resolved
src/open_inwoner/kvk/client.py Outdated Show resolved Hide resolved
src/open_inwoner/kvk/constants.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.27%. Comparing base (235ec5a) to head (c083269).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1524      +/-   ##
===========================================
+ Coverage    94.25%   94.27%   +0.01%     
===========================================
  Files         1068     1071       +3     
  Lines        40466    40597     +131     
===========================================
+ Hits         38141    38271     +130     
- Misses        2325     2326       +1     

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

@pi-sigma pi-sigma force-pushed the task/2931-update-company-data-login branch from 557a8f2 to c083269 Compare December 16, 2024 11:33
@pi-sigma pi-sigma requested a review from swrichards December 16, 2024 12:00
@pi-sigma pi-sigma marked this pull request as draft December 19, 2024 09:16
@pi-sigma
Copy link
Contributor Author

At the time of opening this PR we are not able to support separate updates for users with the same kvkNummer but different vesttigingsnummer because our user model treats users with the same kvk as identical. For now, we will only update the company name, treating the hoofdvestiging as the source of truth. Part of the code can be re-used once we decide how to deal with the kvk/vestigingsnummer situation and (potentially) refactor our user model to deal with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants