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.

Sign up for GitHub

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

Jump to bottom

Add function to authorize user to register patient #9000

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Add function to authorize user to register patient #9000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
80a8168
Add function to authorize user to register patient
JavidSumra Nov 5, 2024
314b50c
Merge branch 'develop' into issues/8988/fix-infinite-rendering-non-ho…
JavidSumra Nov 6, 2024
fdb7940
Merge branch 'develop' into issues/8988/fix-infinite-rendering-non-ho…
JavidSumra Nov 6, 2024
bd34bcb
Add Requested Changes
JavidSumra Nov 6, 2024
7a399b6
fix merger conflict
JavidSumra Nov 6, 2024
088f675
Merge branch 'issues/8988/fix-infinite-rendering-non-home-facility' o…
JavidSumra Nov 6, 2024
6c32072
Merge branch 'ohcnetwork:develop' into issues/8988/fix-infinite-rende…
JavidSumra Nov 7, 2024
7976cea
Reuse Existing Function
JavidSumra Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions src/components/Facility/FacilityHome.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import uploadFile from "@/Utils/request/uploadFile";
import useQuery from "@/Utils/request/useQuery";
import { sleep } from "@/Utils/utils";

import { PatientRegisterAuth } from "../Patient/PatientRegister";

type Props = {
facilityId: string;
};
Expand Down Expand Up @@ -460,17 +462,19 @@ export const FacilityHome = ({ facilityId }: Props) => {
{CameraFeedPermittedUserTypes.includes(authUser.user_type) && (
<LiveMonitoringButton />
)}
<ButtonV2
variant="primary"
ghost
border
className="mt-2 flex w-full flex-row justify-center md:w-auto"
onClick={() => navigate(`/facility/${facilityId}/patient`)}
authorizeFor={NonReadOnlyUsers}
>
<CareIcon icon="l-plus" className="text-lg" />
<span className="text-sm">{t("add_details_of_patient")}</span>
</ButtonV2>
{PatientRegisterAuth(authUser, facilityData, facilityId) && (
<ButtonV2
variant="primary"
ghost
border
className="mt-2 flex w-full flex-row justify-center md:w-auto"
onClick={() => navigate(`/facility/${facilityId}/patient`)}
authorizeFor={NonReadOnlyUsers}
>
<CareIcon icon="l-plus" className="text-lg" />
<span className="text-sm">{t("add_details_of_patient")}</span>
</ButtonV2>
)}
<ButtonV2
id="view-patient-facility-list"
variant="primary"
Expand Down
62 changes: 37 additions & 25 deletions src/components/Patient/PatientRegister.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import TransferPatientDialog from "@/components/Facility/TransferPatientDialog";
import {
DistrictModel,
DupPatientModel,
FacilityModel,
WardModel,
} from "@/components/Facility/models";
import {
Expand Down Expand Up @@ -94,6 +95,8 @@ import {
scrollTo,
} from "@/Utils/utils";

import { UserModel } from "../Users/models";

type PatientForm = PatientModel &
PatientMeta & { age?: number; is_postpartum?: boolean };

Expand Down Expand Up @@ -919,31 +922,12 @@ export const PatientRegister = (props: PatientRegisterProps) => {
return <Loading />;
}

const PatientRegisterAuth = () => {
const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
if (
!showAllFacilityUsers.includes(authUser.user_type) &&
authUser.home_facility_object?.id === facilityId
) {
return true;
}
if (
authUser.user_type === "DistrictAdmin" &&
authUser.district === facilityObject?.district
) {
return true;
}
if (
authUser.user_type === "StateAdmin" &&
authUser.state === facilityObject?.state
) {
return true;
}

return false;
};

if (!isLoading && facilityId && facilityObject && !PatientRegisterAuth()) {
if (
!isLoading &&
facilityId &&
facilityObject &&
!PatientRegisterAuth(authUser, facilityObject, facilityId)
) {
Comment on lines +925 to +930
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use a more appropriate error component for unauthorized access.

Instead of using Error404 which indicates a missing resource, consider using a dedicated unauthorized access component or error message that clearly communicates the authorization issue to the user.

Example implementation:

-  if (
-    !isLoading &&
-    facilityId &&
-    facilityObject &&
-    !PatientRegisterAuth(authUser, facilityObject, facilityId)
-  ) {
-    return <Error404 />;
-  }
+  if (
+    !isLoading &&
+    facilityId &&
+    facilityObject &&
+    !PatientRegisterAuth(authUser, facilityObject, facilityId)
+  ) {
+    return (
+      <div className="text-center">
+        <h1 className="text-2xl font-bold text-red-600">Unauthorized Access</h1>
+        <p>You are not authorized to register patients in this facility.</p>
+      </div>
+    );
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (
!isLoading &&
facilityId &&
facilityObject &&
!PatientRegisterAuth(authUser, facilityObject, facilityId)
) {
if (
!isLoading &&
facilityId &&
facilityObject &&
!PatientRegisterAuth(authUser, facilityObject, facilityId)
) {
return (
<div className="text-center">
<h1 className="text-2xl font-bold text-red-600">Unauthorized Access</h1>
<p>You are not authorized to register patients in this facility.</p>
</div>
);
}

return <Error404 />;
}

Expand Down Expand Up @@ -1889,3 +1873,31 @@ export const PatientRegister = (props: PatientRegisterProps) => {
</Form>
);
};

export function PatientRegisterAuth(
authUser: UserModel,
facilityObject: FacilityModel | undefined,
facilityId: string,
) {
const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
if (
!showAllFacilityUsers.includes(authUser.user_type) &&
authUser.home_facility_object?.id === facilityId
) {
return true;
}
if (
authUser.user_type === "DistrictAdmin" &&
authUser.district === facilityObject?.district
) {
return true;
}
if (
authUser.user_type === "StateAdmin" &&
authUser.state === facilityObject?.state
) {
return true;
}

return false;
}
Comment on lines +1877 to +1903
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve the PatientRegisterAuth function implementation.

The function needs several improvements for better maintainability and type safety:

  1. Add return type annotation and JSDoc documentation
  2. Move constants outside function
  3. Simplify logic with early returns
+const FACILITY_ADMIN_TYPES = ["DistrictAdmin", "StateAdmin"] as const;
+
+/**
+ * Checks if a user is authorized to register patients in a specific facility
+ * @param authUser - The authenticated user
+ * @param facilityObject - The facility to check authorization for
+ * @param facilityId - The ID of the facility
+ * @returns boolean indicating if the user is authorized
+ */
-export function PatientRegisterAuth(
+export function PatientRegisterAuth(
   authUser: UserModel,
   facilityObject: FacilityModel | undefined,
   facilityId: string,
-) {
-  const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
-  if (
-    !showAllFacilityUsers.includes(authUser.user_type) &&
-    authUser.home_facility_object?.id === facilityId
-  ) {
-    return true;
-  }
-  if (
-    authUser.user_type === "DistrictAdmin" &&
-    authUser.district === facilityObject?.district
-  ) {
-    return true;
-  }
-  if (
-    authUser.user_type === "StateAdmin" &&
-    authUser.state === facilityObject?.state
-  ) {
-    return true;
-  }
-
-  return false;
+): boolean {
+  // Regular users can only access their home facility
+  if (!FACILITY_ADMIN_TYPES.includes(authUser.user_type)) {
+    return authUser.home_facility_object?.id === facilityId;
+  }
+
+  // District admins can access facilities in their district
+  if (authUser.user_type === "DistrictAdmin") {
+    return authUser.district === facilityObject?.district;
+  }
+
+  // State admins can access facilities in their state
+  if (authUser.user_type === "StateAdmin") {
+    return authUser.state === facilityObject?.state;
+  }
+
+  return false;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function PatientRegisterAuth(
authUser: UserModel,
facilityObject: FacilityModel | undefined,
facilityId: string,
) {
const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
if (
!showAllFacilityUsers.includes(authUser.user_type) &&
authUser.home_facility_object?.id === facilityId
) {
return true;
}
if (
authUser.user_type === "DistrictAdmin" &&
authUser.district === facilityObject?.district
) {
return true;
}
if (
authUser.user_type === "StateAdmin" &&
authUser.state === facilityObject?.state
) {
return true;
}
return false;
}
const FACILITY_ADMIN_TYPES = ["DistrictAdmin", "StateAdmin"] as const;
/**
* Checks if a user is authorized to register patients in a specific facility
* @param authUser - The authenticated user
* @param facilityObject - The facility to check authorization for
* @param facilityId - The ID of the facility
* @returns boolean indicating if the user is authorized
*/
export function PatientRegisterAuth(
authUser: UserModel,
facilityObject: FacilityModel | undefined,
facilityId: string,
): boolean {
// Regular users can only access their home facility
if (!FACILITY_ADMIN_TYPES.includes(authUser.user_type)) {
return authUser.home_facility_object?.id === facilityId;
}
// District admins can access facilities in their district
if (authUser.user_type === "DistrictAdmin") {
return authUser.district === facilityObject?.district;
}
// State admins can access facilities in their state
if (authUser.user_type === "StateAdmin") {
return authUser.state === facilityObject?.state;
}
return false;
}

Loading