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

[#2274] Pass data in Haalcenntraal POST as JSON #1145

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Apr 8, 2024

Taiga: #2274

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.07%. Comparing base (f36f589) to head (1bbba84).
Report is 676 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1145   +/-   ##
========================================
  Coverage    95.07%   95.07%           
========================================
  Files          948      949    +1     
  Lines        33556    33574   +18     
========================================
+ Hits         31903    31921   +18     
  Misses        1653     1653           

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

@pi-sigma pi-sigma force-pushed the fix/2274-haalcentraal-api branch from 1562055 to dc4d178 Compare April 9, 2024 08:18
@pi-sigma pi-sigma marked this pull request as ready for review April 9, 2024 09:47
@pi-sigma pi-sigma requested a review from stevenbal April 9, 2024 09:47
src/open_inwoner/haalcentraal/api.py Outdated Show resolved Hide resolved
src/open_inwoner/haalcentraal/api.py Outdated Show resolved Hide resolved
src/open_inwoner/haalcentraal/tests/test_client.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the fix/2274-haalcentraal-api branch from dc4d178 to 4c25c7d Compare April 11, 2024 10:42
"burgerservicenummer": ["999993847"],
}

self.assertEqual(response.request.body, json.dumps(data).encode("utf-8"))
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works for now I'd like to note I would've gone the other way: JSON parse the request body and compare it to the expected dict.

JSON serialization is technically not 100% stable (there are multiple valid JSON strings possible for the same data, because of dict ordering and pretty pretting), and in the assertion you'd get a structural diff instead of a text diff.

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 point!

@pi-sigma pi-sigma force-pushed the fix/2274-haalcentraal-api branch from 4c25c7d to 1bbba84 Compare April 11, 2024 14:53
"burgerservicenummer": ["999993847"],
}

self.assertEqual(json.loads(response.request.body), data)
Copy link
Contributor

Choose a reason for hiding this comment

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

this could have used response.json, but I'll approve to get this merged for the release

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, feels unnecessary

@alextreme alextreme merged commit 81b9ab7 into develop Apr 19, 2024
15 checks passed
@alextreme alextreme deleted the fix/2274-haalcentraal-api branch April 19, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants