From 25ea038b5f8d73f20abff3d65ef81ada5c706be0 Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Thu, 3 Oct 2024 15:04:52 +0200 Subject: [PATCH 1/7] Ensure consistent sorting for loans Updated the `sortByDueDate` function to handle cases where the loan does not have a due date and now using `dayjs` for date manipulation instead of the native Date object. Previously, the code used the current date as a fallback, causing inconsistent sorting results. The new logic assigns 'Infinity' to loans without a due date, ensuring they always appear at the bottom of the list. --- src/core/utils/useLoans.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/utils/useLoans.tsx b/src/core/utils/useLoans.tsx index f29eb0861e..7520e3f3bf 100644 --- a/src/core/utils/useLoans.tsx +++ b/src/core/utils/useLoans.tsx @@ -1,3 +1,4 @@ +import dayjs from "dayjs"; import { useGetLoansV2 } from "../fbs/fbs"; import { useGetV1UserLoans } from "../publizon/publizon"; import { daysBetweenTodayAndDate, materialIsOverdue } from "./helpers/general"; @@ -48,11 +49,11 @@ const getDueDatesLoan = (list: LoanType[]) => { const sortByDueDate = (list: LoanType[]) => { // Todo figure out what to do if loan does not have loan date // For now, its at the bottom of the list - return list.sort( - (a, b) => - new Date(a.dueDate || new Date()).getTime() - - new Date(b.dueDate || new Date()).getTime() - ); + return list.sort((a, b) => { + const dateA = a.dueDate ? dayjs(a.dueDate).valueOf() : Infinity; + const dateB = b.dueDate ? dayjs(b.dueDate).valueOf() : Infinity; + return dateA - dateB; + }); }; type Loans = { From a8a75cea97dc365dd6b3c00d4d42d6d340ca3dbf Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 7 Oct 2024 14:38:03 +0200 Subject: [PATCH 2/7] Remove the `sortByDueDate` responsibility from `useLoans` The `useLoans` hook should not handle the sorting of loans. - Moved `sortByDueDate` to a helper function to maintain separation of concerns. Used `lodash` to prevent modification of the original array. - Added `sortLoansByIsRenewableThenDueDate` to enable proper sorting of loans on the dashboard. **Note:** The `stackedMaterialsDueDatesFbs` function will be removed in the next commit. --- src/apps/loan-list/list/loan-list.tsx | 9 ++-- .../GroupModal/GroupModalLoansList.tsx | 11 +++-- src/core/utils/helpers/general.ts | 27 +++++++++++- src/core/utils/useLoans.tsx | 41 ++++++------------- 4 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/apps/loan-list/list/loan-list.tsx b/src/apps/loan-list/list/loan-list.tsx index 639faef2e1..1387055907 100644 --- a/src/apps/loan-list/list/loan-list.tsx +++ b/src/apps/loan-list/list/loan-list.tsx @@ -3,7 +3,8 @@ import { useSelector } from "react-redux"; import dayjs from "dayjs"; import { getAmountOfRenewableLoans, - getScrollClass + getScrollClass, + sortByDueDate } from "../../../core/utils/helpers/general"; import { getUrlQueryParam } from "../../../core/utils/helpers/url"; import { useText } from "../../../core/utils/text"; @@ -55,12 +56,14 @@ const LoanList: FC = ({ pageSize }) => { const [modalLoan, setModalLoan] = useState(null); const { fbs: { - loans: loansPhysical, + loans: fbsLoans, stackedMaterialsDueDates: stackedMaterialsDueDatesFbs, isLoading: isLoadingFbs }, - publizon: { loans: loansDigital, isLoading: isLoadingPublizon } + publizon: { loans: publizonLoans, isLoading: isLoadingPublizon } } = useLoans(); + const loansPhysical = sortByDueDate(fbsLoans); + const loansDigital = sortByDueDate(publizonLoans); const openLoanDetailsModal = useCallback( (loan: LoanType) => { setModalLoan(loan); diff --git a/src/components/GroupModal/GroupModalLoansList.tsx b/src/components/GroupModal/GroupModalLoansList.tsx index 21ccccc8d4..3456ceb90e 100644 --- a/src/components/GroupModal/GroupModalLoansList.tsx +++ b/src/components/GroupModal/GroupModalLoansList.tsx @@ -1,4 +1,4 @@ -import React, { FC, useState, useEffect } from "react"; +import React, { FC, useState, useEffect, useMemo } from "react"; import SelectableMaterial from "../../apps/loan-list/materials/selectable-material/selectable-material"; import { useText } from "../../core/utils/text"; import { isLoanType, LoanType } from "../../core/utils/types/loan-type"; @@ -8,6 +8,7 @@ import StatusBadge from "../../apps/loan-list/materials/utils/status-badge"; import { formatDate } from "../../core/utils/helpers/date"; import { ListType } from "../../core/utils/types/list-type"; import { getLoanDeliveryDate } from "../../apps/loan-list/utils/helpers"; +import { sortLoansByIsRenewableThenDueDate } from "../../core/utils/helpers/general"; export interface GroupModalLoansListProps { materials: LoanType[]; @@ -24,9 +25,11 @@ const GroupModalLoansList: FC = ({ selectMaterials, pageSize }) => { - // Show renewable materials first, then non-renewable - const groupedMaterials = materials.sort( - (a, b) => Number(!!b.isRenewable) - Number(!!a.isRenewable) + // Memoize groupedMaterials to prevent unnecessary re-computations + // Because groupedMaterials is a dependency of the useEffect hook + const groupedMaterials = useMemo( + () => sortLoansByIsRenewableThenDueDate(materials), + [materials] ); const t = useText(); const [displayedMaterials, setDisplayedMaterials] = useState([]); diff --git a/src/core/utils/helpers/general.ts b/src/core/utils/helpers/general.ts index c3f54e47b0..8ce32b3011 100644 --- a/src/core/utils/helpers/general.ts +++ b/src/core/utils/helpers/general.ts @@ -1,6 +1,6 @@ import { useEffect, useRef } from "react"; import dayjs from "dayjs"; -import { first, uniq } from "lodash"; +import { first, orderBy, uniq } from "lodash"; import { vi } from "vitest"; import { CoverProps } from "../../../components/cover/cover"; import { UseTextFunction } from "../text"; @@ -217,6 +217,31 @@ export const sortByReservationDate = (list: ReservationType[]) => { ); }; +export const sortByDueDate = (list: LoanType[]) => { + // We use orderBy from lodash to avoid mutating the original list + return orderBy( + list, + (item) => + item.dueDate ? dayjs(item.dueDate).startOf("day").valueOf() : Infinity, + "asc" + ); +}; + +export const sortLoansByIsRenewableThenDueDate = (list: LoanType[]) => { + // We use orderBy from lodash to avoid mutating the original list + return orderBy( + list, + [ + // First criterion: isRenewable (Use "desc" to put renewable items first) + (item) => Number(!!item.isRenewable), + // Second criterion: dueDate (earlier dates first) + (item) => + item.dueDate ? dayjs(item.dueDate).startOf("day").valueOf() : Infinity + ], + ["desc", "asc"] + ); +}; + export const getDueDatesForModal = (list: LoanType[], date: string) => { return list.filter(({ dueDate }) => dueDate === date); }; diff --git a/src/core/utils/useLoans.tsx b/src/core/utils/useLoans.tsx index 7520e3f3bf..263387beda 100644 --- a/src/core/utils/useLoans.tsx +++ b/src/core/utils/useLoans.tsx @@ -1,4 +1,3 @@ -import dayjs from "dayjs"; import { useGetLoansV2 } from "../fbs/fbs"; import { useGetV1UserLoans } from "../publizon/publizon"; import { daysBetweenTodayAndDate, materialIsOverdue } from "./helpers/general"; @@ -46,16 +45,6 @@ const getDueDatesLoan = (list: LoanType[]) => { ) as string[]; }; -const sortByDueDate = (list: LoanType[]) => { - // Todo figure out what to do if loan does not have loan date - // For now, its at the bottom of the list - return list.sort((a, b) => { - const dateA = a.dueDate ? dayjs(a.dueDate).valueOf() : Infinity; - const dateB = b.dueDate ? dayjs(b.dueDate).valueOf() : Infinity; - return dateA - dateB; - }); -}; - type Loans = { loans: LoanType[]; overdue: LoanType[]; @@ -74,6 +63,10 @@ type UseLoansType = { type UseLoans = () => UseLoansType; +// useLoans is a custom hook that fetches loans from both FBS and Publizon +// and combines them into lists. The loans are then divided into three +// categories: overdue, soon overdue, and far from overdue. +// The hook is NOT responsible for any sorting of the loans. const useLoans: UseLoans = () => { const { data: loansFbs, @@ -101,15 +94,12 @@ const useLoans: UseLoans = () => { : []; // Combine all loans from both FBS and Publizon - const loans = sortByDueDate([...mappedLoansFbs, ...mappedLoansPublizon]); + const loans = [...mappedLoansFbs, ...mappedLoansPublizon]; // Combine "overdue loans" from both FBS and Publizon const loansOverdueFBS = filterLoansOverdue(mappedLoansFbs); const LoansOverduePublizon = filterLoansOverdue(mappedLoansPublizon); - const loansOverdue = sortByDueDate([ - ...loansOverdueFBS, - ...LoansOverduePublizon - ]); + const loansOverdue = [...loansOverdueFBS, ...LoansOverduePublizon]; // combine "soon overdue" loans from both FBS and Publizon const loansSoonOverdueFBS = filterLoansSoonOverdue( @@ -120,10 +110,10 @@ const useLoans: UseLoans = () => { mappedLoansPublizon, threshold.warning ); - const loansSoonOverdue = sortByDueDate([ + const loansSoonOverdue = [ ...loansSoonOverdueFBS, ...loansSoonOverduePublizon - ]); + ]; // combine "far from overdue" loans from both FBS and Publizon const loansFarFromOverdueFBS = filterLoansNotOverdue( @@ -134,17 +124,12 @@ const useLoans: UseLoans = () => { mappedLoansPublizon, threshold.warning ); - const loansFarFromOverdue = sortByDueDate([ + const loansFarFromOverdue = [ ...loansFarFromOverdueFBS, ...loansFarFromOverduePublizon - ]); - - // The due dates are used for the stacked materials - // The stacked materials view shows materials stacked by - // due date, and for this we need a unique list of due dates - const loansSortedByDateFbs = sortByDueDate(mappedLoansFbs); - const loansSortedByDatePublizon = sortByDueDate(mappedLoansPublizon); + ]; + // This logic should be moved to a separate function useloans shuld only return the data // list of all due dates used for the stacked materials const stackedMaterialsDueDatesFbs = getDueDatesLoan(mappedLoansFbs); return { @@ -157,7 +142,7 @@ const useLoans: UseLoans = () => { isError: loansIsError }, fbs: { - loans: loansSortedByDateFbs, + loans: mappedLoansFbs, overdue: loansOverdueFBS, soonOverdue: loansSoonOverdueFBS, farFromOverdue: loansFarFromOverdueFBS, @@ -166,7 +151,7 @@ const useLoans: UseLoans = () => { isError: isErrorFbs }, publizon: { - loans: loansSortedByDatePublizon, + loans: mappedLoansPublizon, overdue: LoansOverduePublizon, soonOverdue: loansSoonOverduePublizon, farFromOverdue: loansFarFromOverduePublizon, From 57d5217e2e8777e60911532641c1979ac64e7ee0 Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 7 Oct 2024 14:50:13 +0200 Subject: [PATCH 3/7] Remove `stackedMaterialsDueDates` responsebility from `useLoans` Based on the previous commit, `stackedMaterialsDueDates` should not be part of `useLoans`. The purpose of `useLoans` is strictly to provide a list of loans. --- src/apps/loan-list/list/loan-list.tsx | 8 +++----- src/core/utils/helpers/general.ts | 11 +++++++++++ src/core/utils/useLoans.tsx | 16 ---------------- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/apps/loan-list/list/loan-list.tsx b/src/apps/loan-list/list/loan-list.tsx index 1387055907..85ef4365f6 100644 --- a/src/apps/loan-list/list/loan-list.tsx +++ b/src/apps/loan-list/list/loan-list.tsx @@ -3,6 +3,7 @@ import { useSelector } from "react-redux"; import dayjs from "dayjs"; import { getAmountOfRenewableLoans, + getDueDatesLoan, getScrollClass, sortByDueDate } from "../../../core/utils/helpers/general"; @@ -55,15 +56,12 @@ const LoanList: FC = ({ pageSize }) => { const [dueDate, setDueDate] = useState(null); const [modalLoan, setModalLoan] = useState(null); const { - fbs: { - loans: fbsLoans, - stackedMaterialsDueDates: stackedMaterialsDueDatesFbs, - isLoading: isLoadingFbs - }, + fbs: { loans: fbsLoans, isLoading: isLoadingFbs }, publizon: { loans: publizonLoans, isLoading: isLoadingPublizon } } = useLoans(); const loansPhysical = sortByDueDate(fbsLoans); const loansDigital = sortByDueDate(publizonLoans); + const stackedMaterialsDueDatesFbs = getDueDatesLoan(loansPhysical); const openLoanDetailsModal = useCallback( (loan: LoanType) => { setModalLoan(loan); diff --git a/src/core/utils/helpers/general.ts b/src/core/utils/helpers/general.ts index 8ce32b3011..3acecbf56b 100644 --- a/src/core/utils/helpers/general.ts +++ b/src/core/utils/helpers/general.ts @@ -242,6 +242,17 @@ export const sortLoansByIsRenewableThenDueDate = (list: LoanType[]) => { ); }; +export const getDueDatesLoan = (list: LoanType[]) => { + return Array.from( + new Set( + list + .filter(({ dueDate }) => dueDate !== (undefined || null)) + .map(({ dueDate }) => dueDate) + .sort() + ) + ) as string[]; +}; + export const getDueDatesForModal = (list: LoanType[], date: string) => { return list.filter(({ dueDate }) => dueDate === date); }; diff --git a/src/core/utils/useLoans.tsx b/src/core/utils/useLoans.tsx index 263387beda..9622f560f0 100644 --- a/src/core/utils/useLoans.tsx +++ b/src/core/utils/useLoans.tsx @@ -33,17 +33,6 @@ const filterLoansSoonOverdue = (loans: LoanType[], warning: number) => { ); }); }; -// -const getDueDatesLoan = (list: LoanType[]) => { - return Array.from( - new Set( - list - .filter(({ dueDate }) => dueDate !== (undefined || null)) - .map(({ dueDate }) => dueDate) - .sort() - ) - ) as string[]; -}; type Loans = { loans: LoanType[]; @@ -52,7 +41,6 @@ type Loans = { farFromOverdue: LoanType[]; isLoading: boolean; isError: boolean; - stackedMaterialsDueDates?: string[]; }; type UseLoansType = { @@ -129,9 +117,6 @@ const useLoans: UseLoans = () => { ...loansFarFromOverduePublizon ]; - // This logic should be moved to a separate function useloans shuld only return the data - // list of all due dates used for the stacked materials - const stackedMaterialsDueDatesFbs = getDueDatesLoan(mappedLoansFbs); return { all: { loans, @@ -146,7 +131,6 @@ const useLoans: UseLoans = () => { overdue: loansOverdueFBS, soonOverdue: loansSoonOverdueFBS, farFromOverdue: loansFarFromOverdueFBS, - stackedMaterialsDueDates: stackedMaterialsDueDatesFbs, isLoading: isLoadingFbs, isError: isErrorFbs }, From 6ac5126e7d2f45b4a7dad3b7049df4266ae9f64f Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 7 Oct 2024 14:56:26 +0200 Subject: [PATCH 4/7] Cleanup `DashboardNotificationList` by remove unnecessary `.concat` for loans The `useLoans` hook already provides a combined list of all loans (Publizon and FBS), making the `.concat` operation redundant. --- .../dashboard-notification-list.tsx | 43 ++++--------------- 1 file changed, 8 insertions(+), 35 deletions(-) diff --git a/src/apps/dashboard/dashboard-notification-list/dashboard-notification-list.tsx b/src/apps/dashboard/dashboard-notification-list/dashboard-notification-list.tsx index 113d8abe34..66d90e2fe2 100644 --- a/src/apps/dashboard/dashboard-notification-list/dashboard-notification-list.tsx +++ b/src/apps/dashboard/dashboard-notification-list/dashboard-notification-list.tsx @@ -49,17 +49,12 @@ const DashboardNotificationList: FC = ({ } } = useReservations(); const { - all: { loans, isLoading: isLoadingLoans }, + all: { loans, soonOverdue, farFromOverdue, isLoading: isLoadingLoans }, fbs: { overdue: loansOverduePhysical, soonOverdue: loansSoonOverduePhysical, farFromOverdue: loansFarFromOverduePhysical, isLoading: isLoadingLoansPhysical - }, - publizon: { - soonOverdue: loansSoonOverdueDigital, - farFromOverdue: loansFarFromOverdueDigital, - isLoading: isLoadingLoansDigital } } = useLoans(); @@ -125,16 +120,12 @@ const DashboardNotificationList: FC = ({ break; case soon: - setLoansToDisplay( - loansSoonOverduePhysical.concat(loansSoonOverdueDigital) - ); + setLoansToDisplay(soonOverdue); setModalHeader(t("loansSoonOverdueText")); break; case longer: - setLoansToDisplay( - loansFarFromOverduePhysical.concat(loansFarFromOverdueDigital) - ); + setLoansToDisplay(farFromOverdue); setModalHeader(t("loansNotOverdueText")); break; @@ -143,16 +134,7 @@ const DashboardNotificationList: FC = ({ } open(constructModalId(dueDateModal as string, [dueDateInput])); }, - [ - dueDateModal, - open, - loansFarFromOverduePhysical, - loansOverduePhysical, - loansSoonOverduePhysical, - loansSoonOverdueDigital, - loansFarFromOverdueDigital, - t - ] + [open, dueDateModal, loansOverduePhysical, t, soonOverdue, farFromOverdue] ); const dashboardNotificationsLoan = [ @@ -169,8 +151,7 @@ const DashboardNotificationList: FC = ({ : openDueDateModal(yesterday) }, { - listLength: - loansSoonOverduePhysical.length + loansSoonOverdueDigital.length, + listLength: soonOverdue.length, badge: t("statusBadgeWarningText"), header: t("loansSoonOverdueText"), color: "warning", @@ -182,8 +163,7 @@ const DashboardNotificationList: FC = ({ : openDueDateModal(soon) }, { - listLength: - loansFarFromOverduePhysical.length + loansFarFromOverdueDigital.length, + listLength: farFromOverdue.length, header: t("loansNotOverdueText"), dataCy: "loans-not-overdue", color: "neutral", @@ -231,11 +211,7 @@ const DashboardNotificationList: FC = ({ materialsCount={loans.length} header={t("physicalLoansText")} emptyListText={t("noPhysicalLoansText")} - isLoading={ - isLoadingLoans || - isLoadingLoansPhysical || - isLoadingLoansDigital - } + isLoading={isLoadingLoans || isLoadingLoansPhysical} linkText={t("dashboardLoansLinkText")} linkUrl={physicalLoansUrl} /> @@ -259,10 +235,7 @@ const DashboardNotificationList: FC = ({ ...dashboardNotificationsReservations ]} isLoading={ - isLoadingLoans || - isLoadingLoansPhysical || - isLoadingLoansDigital || - isLoadingReservations + isLoadingLoans || isLoadingLoansPhysical || isLoadingReservations } /> )} From f213d348b076a5996a8ae9441969a2a81c201978 Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 14 Oct 2024 15:06:54 +0200 Subject: [PATCH 5/7] Separate sorting logic into `sortByRenewable` and `sortByDueDate` Refactored the sorting logic by separating the `sortByRenewable` function from `sortLoansByIsRenewableThenDueDate`. The new implementation composes the `sortByRenewable` and `sortByDueDate` functions for clarity and to avoid duplication. --- src/core/utils/helpers/general.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/core/utils/helpers/general.ts b/src/core/utils/helpers/general.ts index 3acecbf56b..e5f8bc5073 100644 --- a/src/core/utils/helpers/general.ts +++ b/src/core/utils/helpers/general.ts @@ -227,19 +227,13 @@ export const sortByDueDate = (list: LoanType[]) => { ); }; +export const sortByRenewable = (list: LoanType[]) => { + return orderBy(list, (item) => Number(!!item.isRenewable), "desc"); +}; + export const sortLoansByIsRenewableThenDueDate = (list: LoanType[]) => { - // We use orderBy from lodash to avoid mutating the original list - return orderBy( - list, - [ - // First criterion: isRenewable (Use "desc" to put renewable items first) - (item) => Number(!!item.isRenewable), - // Second criterion: dueDate (earlier dates first) - (item) => - item.dueDate ? dayjs(item.dueDate).startOf("day").valueOf() : Infinity - ], - ["desc", "asc"] - ); + const sortedByDueDate = sortByDueDate(list); + return sortByRenewable(sortedByDueDate); }; export const getDueDatesLoan = (list: LoanType[]) => { From 8ec28bf62bcd1c80739422747ecdcc16a1e1ba09 Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 14 Oct 2024 15:08:02 +0200 Subject: [PATCH 6/7] Properly filter out undefined and null dueDate values Updated the filter logic to explicitly check for both undefined and null dueDate values. This ensures that both cases are correctly excluded from the resulting list of due dates. --- src/core/utils/helpers/general.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/utils/helpers/general.ts b/src/core/utils/helpers/general.ts index e5f8bc5073..c1cec2ea01 100644 --- a/src/core/utils/helpers/general.ts +++ b/src/core/utils/helpers/general.ts @@ -240,7 +240,7 @@ export const getDueDatesLoan = (list: LoanType[]) => { return Array.from( new Set( list - .filter(({ dueDate }) => dueDate !== (undefined || null)) + .filter(({ dueDate }) => dueDate !== undefined && dueDate !== null) .map(({ dueDate }) => dueDate) .sort() ) From 3c5cad6e0a3a06b9fcf78cae5c67b2a4e4dede2a Mon Sep 17 00:00:00 2001 From: Kasper Birch Date: Mon, 14 Oct 2024 15:13:53 +0200 Subject: [PATCH 7/7] Remove startOf("day") from dueDate sorting logic This change simplifies the code and ensures loans with the same due date but different times will be sorted accurately if needed. --- src/core/utils/helpers/general.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/utils/helpers/general.ts b/src/core/utils/helpers/general.ts index c1cec2ea01..34d07f0974 100644 --- a/src/core/utils/helpers/general.ts +++ b/src/core/utils/helpers/general.ts @@ -221,8 +221,7 @@ export const sortByDueDate = (list: LoanType[]) => { // We use orderBy from lodash to avoid mutating the original list return orderBy( list, - (item) => - item.dueDate ? dayjs(item.dueDate).startOf("day").valueOf() : Infinity, + (item) => (item.dueDate ? dayjs(item.dueDate).valueOf() : Infinity), "asc" ); };