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

[#1536] Refactored Haalcentraal to use a dataclass and update to 2.0-final #727

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

Bartvaderkin
Copy link
Contributor

The refactoring and update commits are split for convenience.

@alextreme
Copy link
Member

Ziet er in principe netjes uit. Je houdt volgens mij hiermee nog wel rekening ermee dat Groningen nog HC BRP v1.3 hanteert, toch?

@Bartvaderkin
Copy link
Contributor Author

@alextreme Ja die switch op settings.BRP_VERSION zit er nog in.

@Bartvaderkin Bartvaderkin marked this pull request as ready for review August 21, 2023 09:04
@override_settings(
LOG_OUTGOING_REQUESTS_ENABLED=True,
LOG_OUTGOING_REQUESTS_DB_SAVE=True,
)
class TestPreSaveSignal(ClearCachesMixin, HaalCentraalMixin, TestCase):
Copy link
Contributor

@pi-sigma pi-sigma Aug 21, 2023

Choose a reason for hiding this comment

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

  • I don't think there is a setting LOG_OUTGOING_REQUESTS_ENABLED
  • LOG_OUTGOING_REQUESTS_DB_SAVE=True is the default, so override shouldn't be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I see this got updated since I last had overrides in my local.py

I'm keeping the @override_settings(LOG_OUTGOING_REQUESTS_DB_SAVE=True) here because settings like these are often modified in local.py (as I do).

@@ -83,24 +91,49 @@ def update_brp_data_in_db(user, brp_version, initial=True):

Copy link
Contributor

Choose a reason for hiding this comment

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

Oddly the line I want to comment doesn't show up in this view (only if you follow "show file").

Instead of

 if data.get("personen"):
     data = data.get("personen", [])[0]
 else:
     data = []

perhaps the following:

if data := data.get("personen", []):
    data = data[0]

then you can ditch the else clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, some old code I should have addressed.

I updated the code, with a small change of not using the default value for .get(), because we want the None value for the walrus operator.

if data := data.get("personen"):
    data = data[0]

Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

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

Couple of small remarks/suggestions.

@Bartvaderkin
Copy link
Contributor Author

Thanks for the feedback, I updated the PR with new commits.

@alextreme alextreme merged commit d1016dc into develop Aug 22, 2023
9 of 10 checks passed
@alextreme alextreme deleted the feature/refactor-haalcentraal branch August 22, 2023 17:53
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