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

[NoQA] Add policyID to getOrderedReportIDs and sortReportsByLastRead #33302

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,7 @@ const CONST = {
CUSTOM_UNIT_RATE_BASE_OFFSET: 100,
OWNER_EMAIL_FAKE: '_FAKE_',
OWNER_ACCOUNT_ID_FAKE: 0,
ID_FAKE: '_FAKE_',
},

CUSTOM_UNITS: {
Expand Down
13 changes: 12 additions & 1 deletion src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,19 @@ const getLastAccessedReportID = (
isFirstTimeNewExpensifyUser: OnyxEntry<boolean>,
openOnAdminRoom: boolean,
reportMetadata: OnyxCollection<ReportMetadata>,
policyID?: string,
policyMemberAccountIDs?: string[],
): string | undefined => {
const lastReport = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, !!isFirstTimeNewExpensifyUser, openOnAdminRoom, reportMetadata);
const lastReport = ReportUtils.findLastAccessedReport(
reports,
ignoreDefaultRooms,
policies,
!!isFirstTimeNewExpensifyUser,
openOnAdminRoom,
reportMetadata,
policyID,
policyMemberAccountIDs,
);
return lastReport?.reportID;
};

Expand Down
62 changes: 52 additions & 10 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -621,17 +621,22 @@ function isDraftExpenseReport(report: OnyxEntry<Report> | EmptyObject): boolean
}

/**
* Given a collection of reports returns them sorted by last read
* Checks if the supplied report has a common policy member with the array passed in params.
*/
function sortReportsByLastRead(reports: OnyxCollection<Report>, reportMetadata: OnyxCollection<ReportMetadata>): Array<OnyxEntry<Report>> {
return Object.values(reports ?? {})
.filter((report) => !!report?.reportID && !!(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]?.lastVisitTime ?? report?.lastReadTime))
.sort((a, b) => {
const aTime = new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${a?.reportID}`]?.lastVisitTime ?? a?.lastReadTime ?? '');
const bTime = new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${b?.reportID}`]?.lastVisitTime ?? b?.lastReadTime ?? '');
function hasReportCommonPolicyMemberWithArray(report: Report, policyMembersAccountIDs: string[]) {
mountiny marked this conversation as resolved.
Show resolved Hide resolved
if (!report.participantAccountIDs) {
return false;
}

return aTime.valueOf() - bTime.valueOf();
});
const policyMembersAccountIDsSet = new Set(policyMembersAccountIDs);

for (const reportParticipant of report.participantAccountIDs) {
if (policyMembersAccountIDsSet.has(reportParticipant.toString())) {
return true;
}
}

return false;
}

/**
Expand Down Expand Up @@ -831,6 +836,40 @@ function isConciergeChatReport(report: OnyxEntry<Report>): boolean {
return report?.participantAccountIDs?.length === 1 && Number(report.participantAccountIDs?.[0]) === CONST.ACCOUNT_ID.CONCIERGE && !isChatThread(report);
}

/**
* Checks if the supplied report belongs to workspace based on the provided params.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
*/
function doesReportBelongToWorkspace(report: Report, policyID: string, policyMembersAccountIDs: string[]) {
return (
isConciergeChatReport(report) ||
(report.policyID === CONST.POLICY.ID_FAKE || !report.policyID ? hasReportCommonPolicyMemberWithArray(report, policyMembersAccountIDs) : report.policyID === policyID)
);
}

/**
* Given a collection of reports, return them sorted by the last read timestamp. Filters the sorted reports by a policy ID and policyMemberAccountIDs, if provided.
*/
function sortReportsByLastRead(
reports: OnyxCollection<Report>,
reportMetadata: OnyxCollection<ReportMetadata>,
policyID?: string,
policyMemberAccountIDs: string[] = [],
): Array<OnyxEntry<Report>> {
let reportsValues = Object.values(reports ?? {});
if (policyID) {
reportsValues = reportsValues.filter((report) => !!report && doesReportBelongToWorkspace(report, policyID, policyMemberAccountIDs));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this before the sortReportsByLastRead method/ outside? This is now changing the purpose of the method which was only to sort so I fee like any filtering should be done before that and we only pass the collection of the reports we know we want to sort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created the separated method to filter reports which is invoked before the sortReportsByLastRead, but to achieve that I had to move parsing OnyxCollection to the array to the parent function. Is it ok? This change required adjusting types in the function params.


return reportsValues
.filter((report) => !!report?.reportID && !!(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]?.lastVisitTime ?? report?.lastReadTime))
.sort((a, b) => {
const aTime = new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${a?.reportID}`]?.lastVisitTime ?? a?.lastReadTime ?? '');
const bTime = new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${b?.reportID}`]?.lastVisitTime ?? b?.lastReadTime ?? '');

return aTime.valueOf() - bTime.valueOf();
});
}

/**
* Returns true if report is still being processed
*/
Expand Down Expand Up @@ -901,13 +940,15 @@ function findLastAccessedReport(
isFirstTimeNewExpensifyUser: boolean,
openOnAdminRoom = false,
reportMetadata: OnyxCollection<ReportMetadata> = {},
policyID?: string,
policyMemberAccountIDs?: string[],
): OnyxEntry<Report> {
// If it's the user's first time using New Expensify, then they could either have:
// - just a Concierge report, if so we'll return that
// - their Concierge report, and a separate report that must have deeplinked them to the app before they created their account.
// If it's the latter, we'll use the deeplinked report over the Concierge report,
// since the Concierge report would be incorrectly selected over the deep-linked report in the logic below.
let sortedReports = sortReportsByLastRead(reports, reportMetadata);
let sortedReports = sortReportsByLastRead(reports, reportMetadata, policyID, policyMemberAccountIDs);

let adminReport: OnyxEntry<Report> | undefined;
if (openOnAdminRoom) {
Expand Down Expand Up @@ -4612,6 +4653,7 @@ export {
getReportFieldTitle,
shouldDisplayThreadReplies,
shouldDisableThread,
doesReportBelongToWorkspace,
getChildReportNotificationPreference,
};

Expand Down
18 changes: 16 additions & 2 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,22 @@ function getOrderedReportIDs(
policies: Record<string, Policy>,
priorityMode: ValueOf<typeof CONST.PRIORITY_MODE>,
allReportActions: OnyxCollection<ReportAction[]>,
currentPolicyID = '',
policyMembersAccountIDs: string[] = [],
): string[] {
// Generate a unique cache key based on the function arguments
const cachedReportsKey = JSON.stringify(
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
[currentReportId, allReports, betas, policies, priorityMode, allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${currentReportId}`]?.length || 1],
[
currentReportId,
allReports,
betas,
policies,
priorityMode,
allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${currentReportId}`]?.length ?? 1,
currentPolicyID,
policyMembersAccountIDs,
],
(key, value: unknown) => {
/**
* Exclude some properties not to overwhelm a cached key value with huge data,
Expand All @@ -151,7 +162,7 @@ function getOrderedReportIDs(
const isInDefaultMode = !isInGSDMode;
const allReportsDictValues = Object.values(allReports);
// Filter out all the reports that shouldn't be displayed
const reportsToDisplay = allReportsDictValues.filter((report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId ?? '', isInGSDMode, betas, policies, true));
let reportsToDisplay = allReportsDictValues.filter((report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId ?? '', isInGSDMode, betas, policies, true));

if (reportsToDisplay.length === 0) {
// Display Concierge chat report when there is no report to be displayed
Expand All @@ -175,6 +186,9 @@ function getOrderedReportIDs(
const nonArchivedReports: Report[] = [];
const archivedReports: Report[] = [];

if (currentPolicyID || policyMembersAccountIDs.length > 0) {
reportsToDisplay = reportsToDisplay.filter((report) => ReportUtils.doesReportBelongToWorkspace(report, currentPolicyID, policyMembersAccountIDs));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi 👋 Coming from #35963

When the IOU report and details page are not highlighted in the LHN, the filter removes the current viewed report. So to fix it we decided to add a check where it's past the currently viewed report.

// There are a few properties that need to be calculated for the report which are used when sorting reports.
reportsToDisplay.forEach((report) => {
// Normally, the spread operator would be used here to clone the report and prevent the need to reassign the params.
Expand Down
13 changes: 13 additions & 0 deletions src/libs/actions/Policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,18 @@ function openWorkspaceReimburseView(policyID) {
API.read('OpenWorkspaceReimburseView', {policyID}, onyxData);
}

function openWorkspace(policyID, clientMemberAccountIDs) {
if (!policyID || !clientMemberAccountIDs) {
Log.warn('openWorkspace invalid params', {policyID, clientMemberAccountIDs});
return;
}

API.read('OpenWorkspace', {
policyID,
clientMemberAccountIDs: JSON.stringify(clientMemberAccountIDs),
});
}

function openWorkspaceMembersPage(policyID, clientMemberEmails) {
if (!policyID || !clientMemberEmails) {
Log.warn('openWorkspaceMembersPage invalid params', {policyID, clientMemberEmails});
Expand Down Expand Up @@ -1945,6 +1957,7 @@ export {
createWorkspace,
openWorkspaceMembersPage,
openWorkspaceInvitePage,
openWorkspace,
removeWorkspace,
createWorkspaceFromIOUPayment,
setWorkspaceInviteMembersDraft,
Expand Down
Loading