Migrate policy endpoints to new API structure
#2025
Conversation
anth-volk
force-pushed
the
feat/1987-migrate-policy
branch
from
1619ca7 to
43d4863
Compare
| @@ -12,7 +12,9 @@ | |||
|
|
|||
| @validate_country | |||
| @economy_bp.route("/<policy_id>/over/<baseline_policy_id>", methods=["GET"]) | |||
| def get_economic_impact(country_id, policy_id, baseline_policy_id): | |||
| def get_economic_impact( | |||
| country_id: str, policy_id: str | int, baseline_policy_id: str | int | |||
There was a problem hiding this comment.
question, maybe blocking - I don't think, as currently written, this type is correct. I believe it's always str unless you use the <int:whatever> syntax
Assuming I'm correct, the suggestion is to either change the path or change the type to str. If not, excited to learn:)
There was a problem hiding this comment.
This entire route/service combo was the first one I did, prior to many of your incredibly helpful suggestions & improvements. Let me more broadly bring this up to snuff as part of this PR.
| status=404, | ||
| ) | ||
| else: | ||
| return Response( |
There was a problem hiding this comment.
comment, non-blocking - again just referencing #2038 on
- Consistency of return values structure and
- globally handling exception/response mapping instead of writing one in every handler.
| assert result is not None | ||
| assert isinstance(result["policy_json"], dict) | ||
| assert result["policy_json"]["param"] == "value" |
There was a problem hiding this comment.
suggestion, non-blocking I think you can do all this in one go with assertpy's assert_that? (not something I've used a ton). see dict-comparison
| assert message == "Policy created" | ||
| assert exists is False | ||
| assert ( | ||
| mock_database.query.call_count == 3 |
There was a problem hiding this comment.
suggestion, non-blocking I would personally just check the calls directly instead of count. (i.e. verify it made each query against the expected string). It doesn't involve much more work and it can catch issues (I.e. if I inject the wrong value into the query, etc.)
anth-volk
force-pushed
the
feat/1987-migrate-policy
branch
from
5a9669b to
979037a
Compare
Fixes #1987.
This migrates the
policyendpoints to the new API structure. It also adds a series of unit tests to the new service.