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

[BUG] fix Location/ Bed Management issue ( District Lab Admin Account) #1959

Merged

Conversation

AnkurPrabhu
Copy link
Contributor

@AnkurPrabhu AnkurPrabhu commented Mar 11, 2024

Proposed Changes

  • Brief of changes made.
    the issue here was mainly permission related
    District Lab Admin had read access for facility but not object read access (refer FacilityPermissionMixin)
    same for asset_location while actually managing beds it uses (refer FacilityRelatedPermissionMixin)

Associated Issue

  • Link to issue here : issue

Merge Checklist

  • Tests added/fixed
  • Update docs in /docs
  • Linting Complete
  • Any other necessary step

Only PR's with test cases included and passing lint and test pipelines will be reviewed

@coronasafe/care-backend-maintainers @coronasafe/care-backend-admins

@AnkurPrabhu
Copy link
Contributor Author

@rithviknishad @sainak pls take a look works fine for me

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.

A state lab admin and greater user would have access to facilities belonging to the user's state and not user's district

@rithviknishad
Copy link
Member

Also, need tests

@AnkurPrabhu
Copy link
Contributor Author

A state lab admin and greater user would have access to facilities belonging to the user's state and not user's district

@rithviknishad i did not understand this comment what change is needed we dont allow object read for district lab admin ?

i added it to district lab admin as in FacilityQSPermissions we allow DistrictLabAdmin to view it

@rithviknishad
Copy link
Member

FacilityQSPermissions also has a case for state lab admin and greater user:

image

@AnkurPrabhu
Copy link
Contributor Author

AnkurPrabhu commented Mar 11, 2024

okay is this what ur saying
i have to add
or (
hasattr(self, "district")
and request.user.user_type >= User.TYPE_VALUE_MAP["DistrictLabAdmin"]
and request.user.district == self.district
)

instead of simply -> adding request.user.user_type >= User.TYPE_VALUE_MAP["DistrictLabAdmin"] ?

also same changes for statelabadmin as well

@AnkurPrabhu
Copy link
Contributor Author

hey i just noticed that for model AssetLocation we dont have a field named state thats why its failing for assetlocation where as for facility we have state so this fixed the facility permission issue part but shows the error while managing beds
should i add the state field ?
@rithviknishad

@rithviknishad
Copy link
Member

We don't need to add a state field as it'd no longer be having a single source of truth.

A facility belongs to a state and Asset Location belongs to a facility. Bed belongs to an Asset Location.

@AnkurPrabhu
Copy link
Contributor Author

@rithviknishad i have added tests and also did the same for state lab admin looks good to me let me know if any changes are required

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.35%. Comparing base (90565c2) to head (d23bf78).
Report is 21 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1959      +/-   ##
===========================================
+ Coverage    62.19%   62.35%   +0.15%     
===========================================
  Files          221      221              
  Lines        12203    12219      +16     
  Branches      1742     1744       +2     
===========================================
+ Hits          7590     7619      +29     
+ Misses        4305     4289      -16     
- Partials       308      311       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnkurPrabhu
Copy link
Contributor Author

@khavinshankar pls take a look

@vigneshhari vigneshhari merged commit 535f6a5 into coronasafe:develop Mar 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants