Deleted facilityuser viewset #13502
Deleted facilityuser viewset #13502
Conversation
github-actions
bot
added
DEV: backend
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces a new Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
kolibri/core/auth/api.py (2)
529-538: Simplify control flow by removing unnecessary else clause.The
elseclause after thereturnstatement is unnecessary and can be simplified.def destroy(self, request, *args, **kwargs): if kwargs.get("pk"): # Single object deletion user = self.get_object() user.delete() return Response(status=status.HTTP_204_NO_CONTENT) - else: - # Bulk deletion - return self.bulk_destroy(request, *args, **kwargs) + # Bulk deletion + return self.bulk_destroy(request, *args, **kwargs)
545-553: Simplify dictionary key check.Use direct membership testing instead of calling
.keys()for better performance and readability.def allow_bulk_restore(self): """ Hook to ensure that the bulk restore should be allowed. By default this checks that the restore is only applied to filtered querysets. """ return any( - key in self.filterset_fields for key in self.request.query_params.keys() + key in self.filterset_fields for key in self.request.query_params )kolibri/core/auth/test/test_api.py (1)
1119-1142: Consider using actual soft delete mechanism instead of direct database updates.While the ordering logic is correct, directly updating
date_deletedvia database queries may not accurately reflect real-world usage. Consider using the actual soft delete endpoint to create the test data for more authentic testing.- for idx, user in enumerate(users): - models.FacilityUser.objects.filter(id=user.id).update( - date_deleted=timezone.now() - timedelta(days=idx + 1) - ) + # Soft delete users at different times to create natural ordering + for idx, user in enumerate(users): + with patch('django.utils.timezone.now', return_value=timezone.now() - timedelta(days=idx + 1)): + self.client.delete( + reverse("kolibri:core:facilityuser-detail", kwargs={"pk": user.pk}) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kolibri/core/auth/api.py(3 hunks)kolibri/core/auth/api_urls.py(2 hunks)kolibri/core/auth/test/test_api.py(1 hunks)kolibri/plugins/facility/assets/src/views/UserPage/index.vue(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
kolibri/core/auth/api.py
552-552: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 Pylint (3.3.7)
kolibri/core/auth/api.py
[refactor] 520-520: Too many ancestors (9/7)
(R0901)
[refactor] 531-538: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Python unit tests for currently supported Python versions (3.12)
- GitHub Check: Python unit tests on Windows Server (3.8)
- GitHub Check: Python unit tests for currently supported Python versions (3.11)
- GitHub Check: Python unit tests for currently supported Python versions (3.13)
- GitHub Check: Python postgres unit tests
- GitHub Check: Python unit tests on Mac OS (3.10)
- GitHub Check: Python unit tests for currently supported Python versions (3.10)
- GitHub Check: Python unit tests for currently supported Python versions (3.9)
- GitHub Check: Python unit tests for EOL Python versions (3.7)
- GitHub Check: Python unit tests for EOL Python versions (3.6)
- GitHub Check: Python unit tests for EOL Python versions (3.8)
- GitHub Check: All file linting
- GitHub Check: Build WHL file / Build WHL
🔇 Additional comments (11)
kolibri/plugins/facility/assets/src/views/UserPage/index.vue (1)
234-234: LGTM! Frontend-backend field alignment fix.This change correctly aligns the column identifier with the backend field name, ensuring consistent sorting and filtering behavior for the user identifier column.
kolibri/core/auth/api_urls.py (2)
5-5: LGTM! Proper import for new viewset.
41-43: LGTM! Standard router registration.The routing configuration follows established patterns and correctly exposes the new DeletedFacilityUserViewSet endpoints.
kolibri/core/auth/api.py (5)
460-461: LGTM! Backend support for id_number sorting and filtering.Adding
"id_number"to bothvaluesandordering_fieldsenables proper sorting and filtering functionality for the user identifier field, addressing the 500 error mentioned in the PR objectives.
508-510: LGTM! Improved error handling in consolidate method.Using
x.get(ordering_param, "")with a default value prevents KeyError exceptions when an invalid sorting field is passed, which addresses the 500 error prevention mentioned in the PR objectives.
520-527: Well-designed inheritance approach for code reuse.Inheriting from
FacilityUserViewSeteffectively reuses existing functionality while appropriately extending the values and ordering fields withdate_deletedfor soft-deleted user management.
540-543: LGTM! Appropriate hard deletion for soft-deleted users.The bulk destroy implementation correctly performs hard deletion (actual
.delete()) instead of soft deletion, which is the expected behavior for managing soft-deleted users. The permission check prevents users from deleting themselves.
555-571: LGTM! Well-implemented restore functionality.The restore action properly:
- Validates that bulk restore is only allowed on filtered querysets
- Checks for existence of users to restore
- Updates
date_deletedtoNoneto restore users- Returns appropriate HTTP status codes
The implementation follows REST conventions and includes proper error handling.
kolibri/core/auth/test/test_api.py (3)
1094-1117: Excellent test coverage for soft-deleted user retrieval.This test properly validates the core functionality of the new
DeletedFacilityUserViewSetby creating users, soft-deleting them, and verifying they can be retrieved with properdate_deletedvalues.
1178-1195: Well-structured test for successful bulk hard delete.This test properly validates the complete hard delete workflow: soft delete followed by hard delete, with correct assertions verifying the users are permanently removed.
1216-1226: Good error case validation for restore functionality.This test properly validates that attempting to restore non-soft-deleted users returns an appropriate 404 error, ensuring the API behaves correctly in edge cases.
AlexVelezLl
force-pushed
the
deleted-facilityuser-viewset
branch
2 times, most recently
from
d6142a9 to
e3b46d4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Build Artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
kolibri/core/auth/api.py (3)
520-527: Consider the inheritance depth complexity.The static analysis tool flagged "Too many ancestors (9/7)" for this class. While inheriting from
FacilityUserViewSetprovides good code reuse for filtering, sorting, and pagination, the deep inheritance hierarchy adds complexity.The inheritance approach is reasonable for code reuse, but monitor if this becomes difficult to maintain. Consider composition patterns if the inheritance chain grows further.
544-560: Improve error message for bulk restore validation.The
allow_bulk_restorevalidation correctly prevents accidental bulk restoration of all deleted users by requiring filters. However, returning a 400 status with no error message could confuse API consumers.if not self.allow_bulk_restore(): return Response( + {"detail": "Bulk restore requires query filters to prevent accidental restoration of all deleted users."}, status=status.HTTP_400_BAD_REQUEST, )
562-567: Consider returning restoration count for better UX.The restore action currently returns 204 No Content. Consider returning information about how many users were restored to provide better feedback to API consumers.
users.update(date_deleted=None) + restored_count = users.count() - return Response(status=status.HTTP_204_NO_CONTENT) + return Response({"restored_count": restored_count}, status=status.HTTP_200_OK)kolibri/core/auth/test/test_api.py (1)
13-13: Consider using unittest.mock for consistency.For modern Python versions, consider importing from
unittest.mockinstead of the standalonemockpackage:-from mock import patch +from unittest.mock import patch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kolibri/core/auth/api.py(3 hunks)kolibri/core/auth/test/test_api.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
kolibri/core/auth/api.py
[refactor] 520-520: Too many ancestors (9/7)
(R0901)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Python unit tests for currently supported Python versions (3.13)
- GitHub Check: Python unit tests for currently supported Python versions (3.12)
- GitHub Check: Python unit tests on Windows Server (3.8)
- GitHub Check: Python unit tests on Mac OS (3.10)
- GitHub Check: Python unit tests for currently supported Python versions (3.11)
- GitHub Check: Python unit tests for currently supported Python versions (3.9)
- GitHub Check: Python unit tests for currently supported Python versions (3.10)
- GitHub Check: Python unit tests for EOL Python versions (3.6)
- GitHub Check: Python unit tests for EOL Python versions (3.7)
- GitHub Check: Python unit tests for EOL Python versions (3.8)
- GitHub Check: Python postgres unit tests
- GitHub Check: Build WHL file / Build WHL
🔇 Additional comments (12)
kolibri/core/auth/api.py (4)
460-460: LGTM: Fixes ordering byid_numberfield.Adding
id_numbertoordering_fieldsresolves the 500 error when sorting users by this field, since it was already present in thevaluestuple but missing from ordering fields.
508-510: Good defensive programming to prevent KeyError.Using
.get(ordering_param, "")instead of direct dictionary access prevents 500 errors when invalid sorting fields are passed, which aligns with the PR objective.
529-542: Implementation correctly handles hard deletion with permission checks.The
destroymethod properly implements hard deletion (.delete()) instead of soft deletion for the deleted users viewset, andperform_bulk_destroyincludes the necessary permission check to prevent users from deleting themselves.
552-568: ```shell
#!/bin/bashDisplay FacilityUserViewSet including its permission_classes and filter_backends
rg -n "class FacilityUserViewSet" -A50 -B5 kolibri/core/auth/api.py
</details> <details> <summary>kolibri/core/auth/test/test_api.py (8)</summary> `1095-1118`: **Well-structured test for soft-deleted user retrieval.** This test comprehensively covers the soft-deleted user listing functionality with proper setup, execution, and verification steps. --- `1120-1150`: **Excellent use of mocking for time-based testing.** This test properly uses `mock.patch` to control timestamps and verify date-based ordering functionality. The combination of mocked deletion times and manual database updates ensures reliable test results. --- `1152-1169`: **Good security validation for bulk operations.** This test properly verifies that bulk hard delete operations require explicit ID filtering, preventing accidental mass deletions. --- `1171-1184`: **Correctly tests no-op behavior for hard delete.** This test appropriately verifies that attempting to hard delete non-soft-deleted users results in a successful response but no actual deletion, implementing graceful handling of edge cases. --- `1186-1203`: **Comprehensive test for complete hard deletion.** This test properly validates the full hard delete workflow, ensuring soft-deleted users are permanently removed from the database. --- `1205-1222`: **Consistent security validation for restore operations.** This test appropriately mirrors the security requirements for hard delete operations, ensuring restore operations also require explicit ID filtering to prevent unintended bulk actions. --- `1224-1234`: **Appropriate error handling for invalid restore.** This test correctly validates that attempting to restore non-soft-deleted users returns a 404 status, providing clear feedback for invalid operations. --- `1236-1256`: **Comprehensive restore functionality validation.** This test thoroughly validates the user restoration workflow, confirming both the removal from soft-deleted state and successful restoration to active status. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
To your question about the approach - I think that your restore() method could probably just be renamed to update() and the default DRF API for the PATCH request should work as expected.
For the question of inheriting from the FacilityUserViewset class - this results in there being some unused routes provided for the PUT and POST handlers - so for example, I ran this through the API Explorer:
curl -X 'POST' \
'http://localhost:8000/api/auth/deletedfacilityuser/' \
-H 'accept: application/json' \
-H 'Content-Type: application/json' \
-H 'X-CSRFToken: 0qHGvU9WO8WrJq077ROqHf8DEMDfZpiJeiBUnTjG0XHaqYxwl3M3CVID9gGjSa3o' \
-d '{
"username": "c",
"full_name": "b",
"password": "b",
"facility": "54949ffd24603a3c8b886df966a83b76",
"id_number": "b",
"gender": "MALE",
"birth_year": "1988",
"extra_demographics": {}
}'It returns a 404 BUT it also successfully creates the user (I suspect the 404 is related to what you ran into w/ the update() method override).
So we'll probably either want to build a new class that borrows some of the things used in FacilityUserViewSet -- or we could simply override those methods in this subclass to return 404 all the time. I'm sure there are other approaches available but these feel simplest -- keen to hear @rtibbles thoughts here as well.
kolibri/core/auth/api.py
Outdated
| # Bulk deletion | ||
| return self.bulk_destroy(request, *args, **kwargs) |
There was a problem hiding this comment.
Should this be in an else: block for only when there isn't a pk? See Richard's comment on similar code in @ozer550 's PR
There was a problem hiding this comment.
Oh good catch. The if above is early returning a response already, so this is similar to have an else block. I will add the else clause to be more explicit on the intention. However, I think we can never get to this point, since it seems that if the pk arg isnt provided, it automatically just runs the bulk_destroy method, but Im probably missing something. Just added here for consistency with the FacilityUserViewSet implementation.
There was a problem hiding this comment.
I wouldn't bother adding the else - early return pattern seems fine to me, and I have positively encouraged it elsewhere!
There was a problem hiding this comment.
A few questions, one for @nucleogenesis about what the ordering should be, one for @AlexVelezLl about what the intended restrictions actually are.
kolibri/core/auth/api.py
Outdated
| # Bulk deletion | ||
| return self.bulk_destroy(request, *args, **kwargs) |
There was a problem hiding this comment.
I wouldn't bother adding the else - early return pattern seems fine to me, and I have positively encouraged it elsewhere!
kolibri/core/auth/api.py
Outdated
| class DeletedFacilityUserViewSet(FacilityUserViewSet): | ||
| """Viewset for managing soft-deleted FacilityUsers.""" | ||
|
|
||
| order_by_field = "username" |
There was a problem hiding this comment.
I don't recall the details of the designs, @nucleogenesis are we ordering in the frontend by date deleted (most recent first) or by something else? Do we allow sorting by other fields? Would be good to have the order_by_field and ordering_fields set according to the needs of the frontend, but I don't see it detailed in the issue.
There was a problem hiding this comment.
Great catch Richard thank you for that - I failed to consider it when writing the issue.
The design for the trash page shows that we'll order them so that the longest-ago-deleted user is shown first (ie, the users in the most imminent danger of being permanently deleted). Although I can see a case for the inverse as well.
I think changing it to order by date_deleted in either direction will work for this PR then we'll be able to quickly make a change once it gets into user testing if QA et al think it should be the other direction by default.
There was a problem hiding this comment.
Thanks! Will change this to -date_deleted then 👐.
| + f"?member_of={self.facility.id}" | ||
| ) | ||
|
|
||
| self.assertEqual(response.status_code, 400) |
There was a problem hiding this comment.
This is only returning a 400 because the member_of filter would include the current super user, as such the naming of the test feels a little misleading. If the superuser doing the deletion was a user in a different facility to this one, then this would not return a 400, it would succeed.
Do we want a stronger guarantee than this (in which case we should update the implementation) or is this fine (in which case we should just update the test name)?
There was a problem hiding this comment.
Hi @rtibbles! The member_of filter here will not include the current superuser because the current superuser cannot be soft deleted, so it cant reach to this point. The implementation of the BulkDeleteMixin looks just for the filterset_fields attribute not for the fields described in the related Filterset class https://github.com/AlexVelezLl/kolibri/blob/e3b46d4b39e51d4d4feb76b1b5f2cc9625d93348/kolibri/core/mixins.py#L39 and since the only field we have in the filterset_fields is by_id https://github.com/AlexVelezLl/kolibri/blob/e3b46d4b39e51d4d4feb76b1b5f2cc9625d93348/kolibri/core/auth/api.py#L435, then the only required filter is by_ids any other filter will return a 400 error if by_ids is not present. I just double checked running the same tests but with a "user_type=learner" and the test is passing too.
There was a problem hiding this comment.
Ah - that was my mistake for letting that addition of filterset_fields through. That wasn't needed, and will break functionality.
There was a problem hiding this comment.
Can you revert that change - the FacilityUserViewset needs to be filterable by more than just by_ids.
There was a problem hiding this comment.
Oh, didnt know the intention was to be able to delete by other criteria other than by_ids 😅. Should we fix the BulkDeleteMixin then right? To look for other filters present in a related filterset class and not just the filterset_fields
AlexVelezLl
force-pushed
the
deleted-facilityuser-viewset
branch
3 times, most recently
from
f89090c to
dc68fa8
Compare
AlexVelezLl
force-pushed
the
deleted-facilityuser-viewset
branch
2 times, most recently
from
6fe7b41 to
f6394a2
Compare
AlexVelezLl
force-pushed
the
deleted-facilityuser-viewset
branch
from
f6394a2 to
bbce1da
Compare
|
@nucleogenesis @rtibbles Just a heads up that this is ready for a re-review |
kolibri/core/auth/api.py
Outdated
| Hook to ensure that the bulk restore should be allowed. | ||
| By default, these restrictions are the same as for bulk deletion, | ||
| """ | ||
| return self.allow_bulk_destroy(self.get_queryset()) |
There was a problem hiding this comment.
I think, maybe we don't need this method at all, see below.
kolibri/core/auth/api.py
Outdated
| """ | ||
| Restore soft-deleted FacilityUsers. | ||
| """ | ||
| if not self.allow_bulk_restore(): |
There was a problem hiding this comment.
If we modify allow_bulk_destroy as per my comments below, this can just become:
if not self.allow_bulk_destroy():
kolibri/core/auth/test/test_api.py
Outdated
| @@ -1153,6 +1344,7 @@ def _make_request(self, user=None, url=None, params=None): | |||
|
|
|||
| def test_user_list(self): | |||
| response = self._make_request(self.superuser) | |||
| self.maxDiff = None | |||
There was a problem hiding this comment.
Can remove this before we merge!
| """ | ||
| Hook to ensure that the bulk destroy should be allowed. | ||
| By default this checks that the destroy is only applied to | ||
| filtered querysets. | ||
| """ | ||
| filter_fields = set() |
There was a problem hiding this comment.
This looks like the right change to me - sufficiently general to catch the use cases we need, but not going overboard!
kolibri/core/mixins.py
Outdated
| @@ -28,22 +30,33 @@ class BulkDeleteMixin(object): | |||
|
|
|||
| # Taken from https://github.com/miki725/django-rest-framework-bulk | |||
|
|
|||
| def allow_bulk_destroy(self, qs, filtered): | |||
| def allow_bulk_destroy(self, qs): | |||
There was a problem hiding this comment.
Ah, so because we're not even looking at the filtered queryset this doesn't matter.
I wonder, could we just call qs = self.get_queryset() within this function and avoid having to pass it as an argument at all?
There was a problem hiding this comment.
Yes, that makes sense
|
Thanks @rtibbles! Merging :) |
Summary
Comments
FacilityUserViewSetto reuse all the current sorting, filtering, searching, pagination machinery without having to duplicate too much code. Is this is not a common pattern I can just duplicate the relevant parts of the code.date_deltedwith a patch request, and it correctly restored the user, but the api was returning an error 404 because it was trying to query the just updated user, and it wasnt finding it because after the update, this user wasnt belonging to the current queryset (soft_delted_objects) anymore. Gemini suggested that we could override theget_objectmethod, but it said that a cleaner option here was to implement a new action to better encapsulate the restore behavior. Im open to look for the best solution here.References
Closes #13445.
Reviewer guidance