Fixes:Gives access of consultation details to users for only allowed facilities. #8727
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
Apologies for the incorrect changes. |
step 1: Admit Patient A to Facility A, then discharge them from Facility A. Repeat this process for Facility B and Facility C. |
Thankyou very much for the much needed explaination. I have made some changes, kindly review them. And let me know in case of modifications. |
|
The logic seems pretty convincing that the consultation facilities are checked whether it matches with the allowed facilities of the user. 
However, the problem you stated occurs when the facility is no more linked with the district admin and i think that is correct since it may happen that some facilities get linked to different district admins and therefore, the previous admin should not have the access to it. Right?! Also "View / Upload Consultation Files" should also have same added condition right? That if the user is not allowed a facility, it should not be able to interact with the consultation records. Right?! @nihal467 |
|
@Sulochan-khadka The district admins and state admins currently have a super power that they can view everything that they can see. We will eventually be taking it down. In this specific case lets keep it blocked, if someone comes up with an issue lets understand what they want. From a privacy perspective its always safe to limit maximum access. @nihal467 do mark someone to address these questions when you see it. |
|
also @Sulochan-khadka add a proper title for the PR |
Sulochan-khadka
changed the title
|
@bodhish so my logic is fine right? Also if the user is not allowed a facility, it should not be able to interact with the consultation records. Right?! Also i have changed the PR title. I hope this solves the objectives |
Sulochan-khadka
changed the title
|
👋 Hi, @Sulochan-khadka, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@nihal467 @bodhish @rithviknishad is the proposed solution appropriate? Let me know, so that i can solve the merge conflicts. |
WalkthroughThe changes in the Changes
Assessment against linked issues
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (3)
src/Components/Facility/ConsultationCard.tsx (3)
179-181: Enhance permission checks and improve user feedback.The facility permission checks are consistent but could be improved for better maintainability and user experience.
- Extract the permission check to a reusable function:
+const hasPermissionForFacility = (facilities: any[], facilityName: string) => { + return facilities?.some((item) => item.name === facilityName); +}; // Then use it in the buttons: disabled={!hasPermissionForFacility(facilities, itemData.facility_name)}
- Add tooltips to explain why buttons are disabled:
<ButtonV2 disabled={!hasPermissionForFacility(facilities, itemData.facility_name)} + title={!hasPermissionForFacility(facilities, itemData.facility_name) + ? "You don't have permission to access this facility's consultations" + : undefined} >Also applies to: 192-194, 213-218
Line range hint
1-228: Consider caching and memoizing facility permissions.The current implementation fetches facility permissions for each consultation card, which could impact performance when displaying multiple consultations.
Consider these architectural improvements:
- Move facility permissions to a higher-level context or state management
- Implement caching for facility permissions
- Memoize the permission check function to prevent unnecessary re-renders
This would improve performance while maintaining the security requirements for facility-based access control.
Based on the shell script results, I can now provide a final response:
Consider using facility IDs for permission checks instead of names
The codebase shows that facility IDs are consistently used throughout the application for most operations (routing, API calls, etc.). However, the permission check in ConsultationCard.tsx uses facility names for comparison:
!facilities?.some((item) => item.name === itemData.facility_name)Key findings that support using IDs instead of names:
- Facility IDs are used as stable identifiers in API routes (
/api/v1/facility/{facility_id}/)- The facility model contains both
idandfacility_namefields- Other components like FacilityCard use IDs for routing (
/facility/${facility.id})Consider refactoring the permission check to use IDs:
!facilities?.some((item) => item.id === itemData.facility)🔗 Analysis chain
Line range hint
179-218: Verify facility name comparison logic.The current implementation uses facility names for permission checks. This might be fragile if facility names can change or contain special characters.
Let's verify the facility data structure and potential edge cases:
Consider using facility IDs instead of names for more reliable permission checks if the data structure supports it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check facility name uniqueness and format # Expected: Facility names should be unique and follow a consistent format # Check facility name patterns in the codebase echo "Checking facility name patterns..." rg -g '!*.test.*' -g '!*.spec.*' 'facility_name' -B 2 -A 2 # Check for facility ID usage echo "Checking facility ID usage..." rg -g '!*.test.*' -g '!*.spec.*' 'facility.id|facility_id' -B 2 -A 2Length of output: 26919
| import useQuery from "../../Utils/request/useQuery"; | ||
| import routes from "../../Redux/api"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading and error handling for facility data fetching.
The facility data fetching implementation could be enhanced to handle edge cases and improve user experience.
Consider these improvements:
import useQuery from "../../Utils/request/useQuery";
import routes from "../../Redux/api";
+import { LoadingIndicator } from "../Common/LoadingIndicator";
export const ConsultationCard = (props: ConsultationProps) => {
- const facilities = useQuery(routes.getPermittedFacilities).data?.results;
+ const { data: facilitiesData, error: facilitiesError, loading } = useQuery(routes.getPermittedFacilities);
+ const facilities = facilitiesData?.results;
+
+ if (loading) {
+ return <LoadingIndicator />;
+ }
+
+ if (facilitiesError) {
+ Notification.Error({
+ msg: "Failed to fetch facility permissions",
+ });
+ }Also applies to: 30-30
|
do i need to add these implementations recommended by this bot , or is it optional? @nihal467 |
|
@bodhish Currently, admins can access all routes of the disabled button in the patient details page from the patient consultation page, but this approach isn’t ideal for effectively restricting access. Many users with district admin roles rely on this access to manage certain data. The main issue we’re aiming to resolve is to ensure that non-admin users cannot access consultation files of a facility that isn’t linked to their user ID. If a non-admin user who is not linked to a facility tries to access facility data, either by clicking on a patient card to view consultations from the patient list page or by using the "View Consultation" button on the patient details page, an error message—such as "You don't have access to this file"—should be displayed instead of redirecting them to a 404 page. Could you review his deploy preview along with my comment, and then provide a response? |
|
Does our backend send data for these requests / pages? |
backend will not send the data if the user doesn't have access to the consultation record |
|
In that case should we over engineer? The page would render the empty message right 🤔 |
Yes, rather than disabling the button, we can simply throw an error on the frontend stating, "You don't have access to this file," replacing the current behavior of redirecting restricted users to a 404 page. CC: @Sulochan-khadka |
@bodhish is this approach ok ? |
nihal467
removed
needs testing
needs review
merge conflict
|
👋 Hi, @Sulochan-khadka, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
| @@ -173,6 +176,9 @@ export const ConsultationCard = (props: ConsultationProps) => { | |||
| `/facility/${itemData.facility}/patient/${itemData.patient}/consultation/${itemData.id}`, | |||
| ) | |||
| } | |||
| disabled={ | |||
| !facilities?.some((item) => item.name === itemData.facility_name) | |||
There was a problem hiding this comment.
don't we have facility id in the api ? 🤔
|
Closing the issue as don't have any progress. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes