Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(asset_location): added duty_staff endpoint #1689

Closed
wants to merge 5 commits into from

Conversation

aeswibon
Copy link
Member

@aeswibon aeswibon commented Oct 27, 2023

Feature Request

Proposed Changes

  • Adds duty_staff field to asset location model and serializer

Associated PR

Merge Checklist

  • Tests added/fixed
  • Update docs in /docs
  • Linting Complete

@coronasafe/code-reviewers

@aeswibon aeswibon requested a review from sainak October 27, 2023 13:16
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: Patch coverage is 70.21277% with 14 lines in your changes missing coverage. Please review.

Project coverage is 61.43%. Comparing base (5177552) to head (7e690cf).
Report is 364 commits behind head on develop.

Files with missing lines Patch % Lines
care/facility/api/viewsets/asset.py 60.60% 8 Missing and 5 partials ⚠️
care/facility/models/asset.py 87.50% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@nihal467
Copy link
Member

@cp-coder remove the maximum limit of 3 users

@aeswibon
Copy link
Member Author

Okay, I will remove the validation

@nihal467
Copy link
Member

LGTM

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

care/facility/api/serializers/asset.py Outdated Show resolved Hide resolved
care/facility/api/serializers/asset.py Outdated Show resolved Hide resolved
@sainak
Copy link
Member

sainak commented Nov 3, 2023

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

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cp-coder I don't see a custom through model that inherits BaseModel

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removing a duty staff user from asset location, those records are not set to deleted=True instead it's permanently deleted right?

There still seems to be no API endpoint/way to unlink a staff from a location.

@aeswibon
Copy link
Member Author

When removing a duty staff user from asset location, those records are not set to deleted=True instead it's permanently deleted right?

There still seems to be no API endpoint/way to unlink a staff from a location.

We can just remove the staff from the doctor/staff list in the location form and it will automatically unlink the staff.

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{"duty_staff": "Staff already assigned to the location"}
)

user = User.objects.filter(id=duty_staff, home_facility=asset.facility)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use first() here itself. no need for 2 queries.

@rithviknishad
Copy link
Member

rithviknishad commented Jan 8, 2024

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 sainak added the to-be-closed PRs with no updates in the last 3 weeks will be closed label May 12, 2024
@nihal467 nihal467 reopened this Aug 13, 2024
@nihal467 nihal467 requested a review from a team as a code owner August 13, 2024 08:13
@nihal467 nihal467 added changes required and removed do-not-merge waiting-for-review Hold to-be-closed PRs with no updates in the last 3 weeks will be closed labels Aug 13, 2024
@nihal467
Copy link
Member

nihal467 commented Aug 13, 2024

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

@vigneshhari
Copy link
Member

Closing because of inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants