fix(asset_location): added duty_staff endpoint #1689
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1689 +/- ##
===========================================
+ Coverage 61.39% 61.43% +0.03%
===========================================
Files 211 211
Lines 11593 11639 +46
Branches 1660 1674 +14
===========================================
+ Hits 7118 7150 +32
- Misses 4203 4212 +9
- Partials 272 277 +5 ☔ View full report in Codecov by Sentry. |
|
@cp-Coder remove the maximum limit of 3 users |
|
Okay, I will remove the validation |
|
LGTM |
There was a problem hiding this comment.
When removing a duty staff user from asset location, those records are not set to deleted=True instead it's permanently deleted right?
cc: @sainak
yeah we can use a custom through model cc: @vigneshhari |
We can just remove the staff from the doctor/staff list in the location form and it will automatically unlink the staff. |
There was a problem hiding this comment.
But this just deletes and recreates the same objects everytime a new user is assigned to a location. Say there are three users, A, B, C. And you want to remove user C and add user D.
In the current approach, you are removing all existing ones for a particular location (A, B, C) and you create new M2M records with (A, B, D).
This performs unnecessary soft deletes and create operations for user A and B. And if we inspect the created_date of those records, it'd end up being the new date, although A and B user were linked before this datetime.
Imaging you have 10 users linked to a location. You'd end up recreating 10 records? Why?
This seems unnecessary.
What's required is, two API endpoints:
POST /location/{location__external_id}/duty_staff which will link a user to a location.
DELETE /location{id}/duty_staff/{external_id} which will unlink a user from a location (soft-delete).
This would avoid unnecessary soft deletes and creates too, and the created_date of the m2m record also would be correct.
aeswibon
force-pushed
the
issue#5364
branch
2 times, most recently
from
80f2dfb
to
27361cf
Compare
| {"duty_staff": "Staff already assigned to the location"} | ||
| ) | ||
|
|
||
| user = User.objects.filter(id=duty_staff, home_facility=asset.facility) |
There was a problem hiding this comment.
This needs to be performed first, we need to check if a use exists before doing anything else, we also need to ensure that the id is an integer or perform some validation on it.
| {"duty_staff": "Staff already assigned to the location"} | ||
| ) | ||
|
|
||
| user = User.objects.filter(id=duty_staff, home_facility=asset.facility) |
There was a problem hiding this comment.
use first() here itself. no need for 2 queries.
|
Marking as hold as the requirements have changed and a more refined feature "User Availability" will be built that adds more depth and it would replace the "Duty Staff for Locations" feature. Issue: ohcnetwork/care_fe#6968 |
sainak
added
the
to-be-closed
nihal467
added
changes required
and removed
do-not-merge
waiting-for-review
Hold
to-be-closed
|
reopening the PR, we can fix and merge it for now, as the larger PR of generic scheduling is under the planning phase @rithviknishad can you clear conflict and make it ready for testing |
Feature Request
Proposed Changes
duty_stafffield toasset locationmodel and serializerAssociated PR
Merge Checklist
/docs@coronasafe/code-reviewers