feat: added last_updated_at in players search results and player summary #184
Conversation
Reviewer's Guide by SourceryThis pull request adds a new field 'last_updated_at' to the player search results and player summary. The implementation includes updating models, parsers, handlers, and tests to accommodate this new field. The changes also involve modifying the search functionality to allow ordering by the new 'last_updated_at' field. File-Level Changes
Tips
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @TeKrop - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a utility function or option to convert the Unix timestamp in
last_updated_atto a more human-readable format for improved usability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
| data_type = SearchDataType.LAST_UPDATED_AT | ||
|
|
||
| def retrieve_data_value(self, player_data: dict) -> int: |
There was a problem hiding this comment.
suggestion: Add error handling for missing 'lastUpdated' key
Consider adding error handling in case the 'lastUpdated' key is missing from the player_data dictionary. You could use a try-except block or the dict.get() method with a default value to make this more robust and prevent potential KeyError exceptions.
| def retrieve_data_value(self, player_data: dict) -> int: | |
| def retrieve_data_value(self, player_data: dict) -> int: | |
| return player_data.get("lastUpdated", 0) |
| @@ -35,7 +35,13 @@ def test_get_player_career( | |||
| side_effect=[ | |||
| # Player HTML page | |||
| Mock(status_code=status.HTTP_200_OK, text=player_html_data), | |||
There was a problem hiding this comment.
suggestion (testing): Update test to include last_updated_at parser
The test has been updated to include an additional mock for the last_updated_at parser. However, it would be beneficial to add assertions to verify that the last_updated_at field is correctly included in the response.
side_effect=[
Mock(status_code=status.HTTP_200_OK, text=player_html_data),
Mock(return_value=datetime(2023, 1, 1, tzinfo=timezone.utc)),
| @@ -57,7 +57,15 @@ def test_get_player_stats( | |||
| overfast_client, | |||
| "get", | |||
| side_effect=[ | |||
| # Player HTML page | |||
| Mock(status_code=status.HTTP_200_OK, text=player_html_data), | |||
There was a problem hiding this comment.
suggestion (testing): Add assertions for last_updated_at in player stats
The test has been updated to include mocks for the last_updated_at parser, but it's important to add assertions to verify that the last_updated_at field is correctly included in the player stats response.
overfast_client,
"get",
side_effect=[
# Player HTML page
Mock(status_code=status.HTTP_200_OK, text=player_html_data),
# Last updated timestamp
Mock(status_code=status.HTTP_200_OK, text="2023-05-01 12:00:00"),
],
)
response = client.get(f"/players/{player_id}/stats")
assert response.status_code == status.HTTP_200_OK
assert "last_updated_at" in response.json()
assert response.json()["last_updated_at"] == "2023-05-01 12:00:00"
| player_data = player_json_data.copy() | ||
| del player_data["summary"]["namecard"] | ||
| del player_data["summary"]["last_updated_at"] |
There was a problem hiding this comment.
suggestion (testing): Add test for LastUpdatedAtParser
While the test has been updated to remove the last_updated_at field from the player data, it would be beneficial to add a separate test case for the LastUpdatedAtParser to ensure it correctly parses and returns the last_updated_at value.
@pytest.mark.asyncio
async def test_last_updated_at_parser():
mock_data = {"summary": {"last_updated_at": "2023-05-01T12:00:00Z"}}
parser = LastUpdatedAtParser()
result = await parser.parse(mock_data)
assert isinstance(result, datetime)
assert result.isoformat() == "2023-05-01T12:00:00+00:00"
Summary by Sourcery
Introduce a new 'last_updated_at' field in player search results and player summary to track the last update time of player profiles. Enhance the player search API to support ordering by this new field and update relevant test cases to ensure proper functionality.
New Features:
Enhancements:
Tests: