[POLIO-2027] Add Performance Dashboard Backend API #2557
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
quang-le
left a comment
quang-le
left a comment
There was a problem hiding this comment.
Looks good overall.
Please cleanup commented code/prints
| created_at = TimestampField(read_only=True) | ||
| updated_at = TimestampField(read_only=True) |
There was a problem hiding this comment.
Please use DateTimeField instead
| # def create(self, request, *args, **kwargs): | ||
| # print("--- Inside ViewSet create method ---") # DEBUG | ||
| # print(f"ViewSet's self.request: {self.request}") # DEBUG | ||
| # print(f"Request from args: {request}") # DEBUG | ||
| # print(f"Is user authenticated? {request.user.is_authenticated}") # DEBUG | ||
| # if hasattr(request.user, 'iaso_profile'): | ||
| # print(f"User's account from view: {request.user.iaso_profile.account}") # DEBUG | ||
| # else: | ||
| # print("User has no iaso_profile") # DEBUG | ||
| # | ||
| # # Call the parent create method which handles serializer instantiation | ||
| # # and should pass the context (including request) automatically, | ||
| # # or now explicitly via get_serializer_context | ||
| # return super().create(request, *args, **kwargs) |
There was a problem hiding this comment.
I forgot to delete it. I used to api guidelines to create the API but Thibaullt later told me that the API would not available on the mobile side but I forgot to delete it. This will be fixed. Same as the common.py file
| # def __init__(self, *args, **kwargs): | ||
| # # Call the parent constructor first | ||
| # super().__init__(*args, **kwargs) | ||
| # # Print the raw data received by the serializer instance | ||
| # print(f"--- Serializer __init__ ---") | ||
| # print(f"Raw data passed to serializer: {self.initial_data}") | ||
| # print(f"Context passed to serializer: {self.context}") | ||
| # print(f"Request in context? {'request' in self.context}") | ||
| # if 'request' in self.context: | ||
| # req = self.context['request'] | ||
| # print(f" - Request object: {req}") | ||
| # print(f" - User in request: {getattr(req, 'user', 'No user found')}") | ||
| # print(f" - Is user authenticated? {getattr(req.user, 'is_authenticated', False)}") | ||
| # print(f"--- End Serializer __init__ ---") | ||
| # | ||
| # def is_valid(self, raise_exception=False): | ||
| # print(f"--- Serializer is_valid called ---") | ||
| # print(f"Data before validation: {self.initial_data}") | ||
| # print(f"Context in is_valid: {self.context}") | ||
| # # Call the parent is_valid | ||
| # valid = super().is_valid(raise_exception=raise_exception) | ||
| # print(f"Validation result: {valid}") | ||
| # if not valid: | ||
| # print(f"Validation errors: {self.errors}") | ||
| # print(f"--- End Serializer is_valid ---") | ||
| # return valid |
| # def test_filter_by_antigen(self): | ||
| # """ | ||
| # Test that we can filter dashboards by antigen. | ||
| # """ | ||
| # # There are 2 dashboards with 'nOPV2' antigen | ||
| # params = {"antigen": "nOPV2"} | ||
| # response = self.client.get(self.PERFORMANCE_DASHBOARD_API_URL, data=params) | ||
| # self.assertJSONResponse(response, status.HTTP_200_OK) | ||
| # response_data = response.json() | ||
| # self.assertEqual(response_data["count"], 5) | ||
| # | ||
| # result_ids = {item["id"] for item in response_data["results"]} | ||
| # self.assertIn(self.dashboard_4.id, result_ids) | ||
| # self.assertIn(self.dashboard_8.id, result_ids) | ||
| # | ||
| # def test_filter_by_date_range(self): | ||
| # """ | ||
| # Test that we can filter dashboards by a date range. | ||
| # """ | ||
| # # There are 2 dashboards between Feb 1 and Mar 31, 2023 | ||
| # params = {"date_from": "2023-02-01", "date_to": "2023-03-31"} | ||
| # response = self.client.get(self.PERFORMANCE_DASHBOARD_API_URL, data=params) | ||
| # self.assertJSONResponse(response, status.HTTP_200_OK) | ||
| # response_data = response.json() | ||
| # self.assertEqual(response_data["count"], 2) | ||
| # | ||
| # result_ids = {item["id"] for item in response_data["results"]} | ||
| # self.assertIn(self.dashboard_konoha_final_nopv2.id, result_ids) | ||
| # self.assertIn(self.dashboard_suna_commented_bopv.id, result_ids) |
There was a problem hiding this comment.
The jira ticket specified that the API will only filter by country and country block
| updated_at = TimestampField(read_only=True) | ||
|
|
||
| # For read operations, we want to display the country's name | ||
| country = OrgUnitNestedSerializer(read_only=True) |
There was a problem hiding this comment.
| country = OrgUnitNestedSerializer(read_only=True) | |
| country_name = CharField(source="country.name", read_only=True) |
Since we already have a field for country_id it may be easier to skip the serializer
| class UserNestedSerializer(serializers.ModelSerializer): | ||
| class Meta: | ||
| model = User | ||
| fields = ["id", "username", "first_name", "last_name"] | ||
| ref_name = "UserNestedSerializerForNationalLogisticsPlan" | ||
|
|
There was a problem hiding this comment.
We do have a lot of these, you might be able to re-use one
|
|
||
| def create(self, validated_data): | ||
| # Set created_by and account automatically from the request user | ||
| # print("--- Inside create method ---") # DEBUG |
| # Set created_by and account automatically from the request user | ||
| # print("--- Inside create method ---") # DEBUG | ||
| request = self.context.get("request") | ||
| # print(f"Request object: {request}") # DEBUG |
There was a problem hiding this comment.
Please remove all these commented prints
tdethier
changed the title
tdethier
left a comment
tdethier
left a comment
There was a problem hiding this comment.
Overall, it's not bad 👍
We can simplify some parts and make everything cleaner. I would add some more tests as well.
The only thing that's really missing is the time window of the non admin perm.
I suggest we take some time together to go through all comments and make the changes
| # For read operations, we want to display the country's name | ||
| country_name = serializers.CharField(source="country.name", read_only=True) | ||
| # For write operations (create/update), we expect the country ID | ||
| country_id = serializers.PrimaryKeyRelatedField(source="country", queryset=OrgUnit.objects.all(), write_only=True) |
There was a problem hiding this comment.
Since you have another serializer for write operations, you don't need to split this. You should probably use a nested serializer and have a country attribute
| "country_name", # For read operations (displaying nested country object) | ||
| "country_id", # For write operations (accepting country ID) |
| "country_id", # Only country_id is needed for input | ||
| "antigen", | ||
| ] | ||
| # read_only_fields = ["account"] # No longer needed here |
There was a problem hiding this comment.
| # read_only_fields = ["account"] # No longer needed here |
| request = self.context.get("request") | ||
|
|
||
| if request and hasattr(request, "user") and request.user.is_authenticated: | ||
| try: | ||
| profile = request.user.iaso_profile | ||
| validated_data["created_by"] = request.user | ||
| validated_data["account"] = profile.account # Ensure account is set here | ||
| except AttributeError as e: | ||
| logger.error(f"User {request.user} does not have an iaso_profile or account: {e}") | ||
| raise serializers.ValidationError("User profile or account not found.") | ||
| except Exception as e: | ||
| logger.error(f"Unexpected error getting profile/account for {request.user}: {e}") | ||
| raise serializers.ValidationError("Unexpected error encountered while fetching profile/account.") | ||
| else: | ||
| # This should ideally not happen if permissions are checked correctly before the serializer | ||
| logger.error("Request or authenticated user not available in context during creation.") | ||
| raise serializers.ValidationError("Request context or authenticated user missing.") | ||
|
|
||
| # Call the parent create method with the updated validated_data | ||
| return super().create(validated_data) |
There was a problem hiding this comment.
If you define how permissions are handled in your viewset, you no longer need to worry about that in your serializer. In that case, you can consider that:
- you do have a user
- they have authenticated
- they have the required permissions
This means that you could almost drop this create() method and let DRF do its thing because you don't have any specific business logic to add here (except for the created_by field)
| def update(self, instance, validated_data): | ||
| # Set updated_by automatically from the request user | ||
| request = self.context.get("request") | ||
| if request and hasattr(request, "user") and request.user.is_authenticated: |
There was a problem hiding this comment.
Here as well, if this block is reached, we know for sure that we do have a user
|
|
||
| # Check that nested objects are serialized correctly | ||
|
|
||
| self.assertEqual(data["country_name"], dashboard.country.name) |
There was a problem hiding this comment.
It's not nested now, but your AI is right, it should be 😁
| if not hasattr(self.user_Hashirama, "iaso_profile"): | ||
| self.user_Hashirama.iaso_profile = m.IasoProfile.objects.create( | ||
| user=self.user_Hashirama, account=self.account_hokage | ||
| ) | ||
| self.user_Hashirama.save() |
There was a problem hiding this comment.
You don't need to check that because you define the user however you want in your setup, so you are always sure that they have a profile, especially if you have used the test helper to create the user
| is_valid = serializer.is_valid() | ||
| if not is_valid: | ||
| print("Serializer validation errors:", serializer.errors) |
There was a problem hiding this comment.
You don't want to print if it's not valid, because it should be
You need to self.assertTrue(serializer.is_valid())
| from .common_test_data import PerformanceDashboardAPIBase | ||
|
|
||
|
|
||
| class PerformanceDashboardPermissionsAPITestCase(PerformanceDashboardAPIBase): |
There was a problem hiding this comment.
You don't need this class, you have already tested permissions in test_views
|
|
||
| class Meta: | ||
| model = PerformanceDashboard | ||
| fields = ["status", "antigen"] |
There was a problem hiding this comment.
Did you forget these 2 fields or did you forget to add frontend components to add them?
What problem is this PR solving? Explain here in one sentence.
Related JIRA tickets : POLIO-2027
Self proofreading checklist
Doc
Tell us where the doc can be found (docs folder, wiki, in the code...).
Changes
Added a performance dashboard model and its API that shows these features: Status, Date, Country and Antigen. The API also filters by country and country block
How to test
In the terminal, start the IASO app in docker, and type this command:
sudo docker compose run iaso manage test plugins.polio.tests.api.performance_dashboardPrint screen / video
Notes
Things that the reviewers should know:
Follow the Conventional Commits specification
The merge message of a pull request must follow the Conventional Commits specification.
This convention helps to automatically generate release notes.
Use lowercase for consistency.
Example:
Note that the Jira reference is preceded by a line break.
Both the line break and the Jira reference are entered in the Add an optional extended description… field.
Doc
Tell us where the doc can be found (docs folder, wiki, in the code...).
Changes
Added a performance dashboard that shows these features: Status, Date, Country and Antigen. The dashboard also filters by country and country block
How to test
In the terminal, start the IASO app in docker, and type this command:
sudo docker compose run iaso manage test plugins.polio.tests.api.performance_dashboardPrint screen / video
Notes
Things that the reviewers should know:
Follow the Conventional Commits specification
The merge message of a pull request must follow the Conventional Commits specification.
This convention helps to automatically generate release notes.
Use lowercase for consistency.
Example:
Note that the Jira reference is preceded by a line break.
Both the line break and the Jira reference are entered in the Add an optional extended description… field.