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:)
|
|
||
| @policy_bp.route("/<policy_id>", methods=["GET"]) | ||
| @validate_country | ||
| def get_policy(country_id: str, policy_id: int | str) -> Response: |
There was a problem hiding this comment.
comment non-blocking - for someone reading this who has not already seen it, reference to discussion on #2038 and where to define the "country" part of the path.
| policy data in JSON format | ||
| """ | ||
|
|
||
| if policy_id is None: |
There was a problem hiding this comment.
question Is this even possible? I would expect the path to just not match.
If possible, love to learn how. If not possible, suggestion remove the unexecuted code.
| # Specifically cast policy_id to an integer | ||
| policy_id = int(policy_id) | ||
|
|
||
| policy: dict | None = policy_service.get_policy(country_id, policy_id) |
There was a problem hiding this comment.
accolade I like explicit typing where you can't tell what the result is without an IDE or reading code in another file. I don't know how much the rest of the industry agrees with me though.
| 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.
| print(result) | ||
| print(result["policy_json"]) | ||
| print(type(result["policy_json"])) |
There was a problem hiding this comment.
nonblocking, comment did you mean to leave these in? If not remove.
|
|
||
| class TestPolicyService: | ||
|
|
||
| test_policy_id = 8 # Arbitrary value higher than 5 |
There was a problem hiding this comment.
nitt, suggestion
- there are conventions we can adopt to say "arbitrary valid value" so for instance a_test_policy_id invalid_test_policy_id, etc. suggestion a_test_policy_id <-- implies a valid test policy ID where the actual value isn't important.
- This kind of raises more questions than it answers for me. suggestion - clarify what's important about being higher than 5.
| 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.)
| test_policy = {} | ||
| print(test_policy) | ||
| print(type(test_policy)) |
There was a problem hiding this comment.
nitt, non-blocking Did you mean to leave these in? If not remove.
Fixes #1987.
This migrates the
policyendpoints to the new API structure. It also adds a series of unit tests to the new service.