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

Feat: Update how GBR is determined for IOU/expense reports #30655

Merged
merged 27 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
cb3f3d0
add hasOutstandingChildRequest verification
waterim Oct 17, 2023
ddba0ec
adjusted and removed unused parts
waterim Oct 18, 2023
4fca049
clean up show GBR
waterim Oct 20, 2023
7523bd6
Merge remote-tracking branch 'upstream/main' into feat-29595-Update-G…
waterim Oct 20, 2023
b8b6752
Merge remote-tracking branch 'upstream/main' into feat-29595-Update-G…
waterim Oct 23, 2023
f6b79e6
tests
waterim Oct 24, 2023
f4cb009
fix lint
waterim Oct 24, 2023
63f6bd8
tests fix
waterim Oct 24, 2023
647efb8
updated text
waterim Oct 25, 2023
de9e121
fix GBR display
waterim Oct 27, 2023
2bb9228
Merge remote-tracking branch 'upstream/main' into feat-29595-Update-G…
waterim Oct 27, 2023
bfb06ae
changes
waterim Oct 27, 2023
a9c9ef2
remove iou ordering
waterim Oct 30, 2023
0ce0891
lint fix
waterim Oct 30, 2023
dd4a7bb
Merge remote-tracking branch 'upstream/main' into feat-29595-Update-G…
waterim Oct 30, 2023
c79c9f1
Add task logic to shouldShowGBR
puneetlath Oct 31, 2023
c497922
Use or instead of null coalescing in getOrderedReportIDs
puneetlath Oct 31, 2023
8e1ad6d
Update pinned logic for linter
puneetlath Oct 31, 2023
1195a83
Add tests for various shouldShowGBR cases
puneetlath Oct 31, 2023
3e938c6
Remove unneeded import
puneetlath Oct 31, 2023
056b7d5
Update comment about pinned/gbr reports
puneetlath Oct 31, 2023
943bc80
Rename functions to separate UI from function logic
puneetlath Oct 31, 2023
d6eff10
Update src/libs/ReportUtils.js
puneetlath Oct 31, 2023
fb9b40d
Rename shouldShowGBR to requiresAttentionFromCurrentUser in tests
puneetlath Oct 31, 2023
cc74887
Rename shouldShowGBR in one more place
puneetlath Oct 31, 2023
b35d1da
Rename report to option in requiresAttentionFromCurrentUser
puneetlath Oct 31, 2023
efcefdb
Account for both options and reports in unread check
puneetlath Oct 31, 2023
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
3 changes: 1 addition & 2 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ function OptionRowLHN(props) {

const hasBrickError = optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
const defaultSubscriptSize = optionItem.isExpenseRequest ? CONST.AVATAR_SIZE.SMALL_NORMAL : CONST.AVATAR_SIZE.DEFAULT;
const shouldShowGreenDotIndicator =
!hasBrickError && (optionItem.isUnreadWithMention || optionItem.isWaitingForTaskCompleteFromAssignee || ReportUtils.isWaitingForIOUActionFromCurrentUser(optionItem));
const shouldShowGreenDotIndicator = !hasBrickError && ReportUtils.requiresAttentionFromCurrentUser(optionItem, optionItem.parentReportAction);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we are using either the report (in this case, "option") or the parentReportAction to determine if the task is canceled. Why do we only pass the parentReportAction here?


/**
* Show the ReportActionContextMenu modal popover.
Expand Down
68 changes: 29 additions & 39 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -1324,61 +1324,51 @@ function getLastVisibleMessage(reportID, actionsToMerge = {}) {
}

/**
* Determines if a report has an IOU that is waiting for an action from the current user (either Pay or Add a credit bank account)
* Checks if a report is an open task report assigned to current user.
*
* @param {Object} report (chatReport or iouReport)
* @param {Object} report
* @param {Object} [parentReportAction] - The parent report action of the report (Used to check if the task has been canceled)
* @returns {Boolean}
*/
function isWaitingForAssigneeToCompleteTask(report, parentReportAction = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one could also be an "option"

Copy link
Contributor

Choose a reason for hiding this comment

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

Idk though maybe we can do a follow up to address as long as all of these other methods are referencing fields that will exist on either a report or an option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like for the most part all the things we need are there 🤷

return isTaskReport(report) && isReportManager(report) && isOpenTaskReport(report, parentReportAction);
}

/**
* Determines if the option requires action from the current user. This can happen when it:
- is unread and the user was mentioned in one of the unread comments
- is for an outstanding task waiting on the user
- has an outstanding child money request that is waiting for an action from the current user (e.g. pay, approve, add bank account)
*
* @param {Object} option (report or optionItem)
* @param {Object} parentReportAction (the report action the current report is a thread of)
* @returns {boolean}
*/
function isWaitingForIOUActionFromCurrentUser(report) {
if (!report) {
function requiresAttentionFromCurrentUser(option, parentReportAction = {}) {
if (!option) {
return false;
}

if (isArchivedRoom(getReport(report.parentReportID))) {
if (isArchivedRoom(getReport(option.parentReportID))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@puneetlath did you think about the case of archived options too? see #30824 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I'm not sure why we did it this way. Thanks for handling!

return false;
}

const policy = getPolicy(report.policyID);
if (policy.type === CONST.POLICY.TYPE.CORPORATE) {
// If the report is already settled, there's no action required from any user.
if (isSettled(report.reportID)) {
return false;
}

// Report is pending approval and the current user is the manager
if (isReportManager(report) && !isReportApproved(report)) {
return true;
}

// Current user is an admin and the report has been approved but not settled yet
return policy.role === CONST.POLICY.ROLE.ADMIN && isReportApproved(report);
if (option.isUnreadWithMention) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will a report ever have this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap, no it will not. Updated to check. Alternate would be to add the lastReadTime and lastMentionedTime to the optionItem. But I don't really love that either.

return true;
}

// Money request waiting for current user to add their credit bank account
// hasOutstandingIOU will be false if the user paid, but isWaitingOnBankAccount will be true if user don't have a wallet or bank account setup
if (!report.hasOutstandingIOU && report.isWaitingOnBankAccount && report.ownerAccountID === currentUserAccountID) {
if (isWaitingForAssigneeToCompleteTask(option, parentReportAction)) {
return true;
}

// Money request waiting for current user to Pay (from expense or iou report)
if (report.hasOutstandingIOU && report.ownerAccountID && (report.ownerAccountID !== currentUserAccountID || currentUserAccountID === report.managerID)) {
// Has a child report that is awaiting action (e.g. approve, pay, add bank account) from current user
if (option.hasOutstandingChildRequest) {
return true;
}

return false;
}

/**
* Checks if a report is an open task report assigned to current user.
*
* @param {Object} report
* @param {Object} parentReportAction - The parent report action of the report (Used to check if the task has been canceled)
* @returns {Boolean}
*/
function isWaitingForTaskCompleteFromAssignee(report, parentReportAction = {}) {
return isTaskReport(report) && isReportManager(report) && isOpenTaskReport(report, parentReportAction);
}

/**
* Returns number of transactions that are nonReimbursable
*
Expand Down Expand Up @@ -3296,8 +3286,8 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas,
return true;
}

// Include reports that are relevant to the user in any view mode. Criteria include having a draft, having an outstanding IOU, or being assigned to an open task.
if (report.hasDraft || isWaitingForIOUActionFromCurrentUser(report) || isWaitingForTaskCompleteFromAssignee(report)) {
// Include reports that are relevant to the user in any view mode. Criteria include having a draft or having a GBR showing.
if (report.hasDraft || requiresAttentionFromCurrentUser(report)) {
return true;
}
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
Expand Down Expand Up @@ -4155,7 +4145,7 @@ export {
isCurrentUserTheOnlyParticipant,
hasAutomatedExpensifyAccountIDs,
hasExpensifyGuidesEmails,
isWaitingForIOUActionFromCurrentUser,
requiresAttentionFromCurrentUser,
isIOUOwnedByCurrentUser,
getMoneyRequestReimbursableTotal,
getMoneyRequestSpendBreakdown,
Expand Down Expand Up @@ -4275,7 +4265,7 @@ export {
hasNonReimbursableTransactions,
hasMissingSmartscanFields,
getIOUReportActionDisplayMessage,
isWaitingForTaskCompleteFromAssignee,
isWaitingForAssigneeToCompleteTask,
isGroupChat,
isReportDraft,
shouldUseFullTitleToDisplay,
Expand Down
39 changes: 16 additions & 23 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,26 +168,23 @@ function getOrderedReportIDs(
report.iouReportAmount = ReportUtils.getMoneyRequestReimbursableTotal(report, allReports);
});

// The LHN is split into five distinct groups, and each group is sorted a little differently. The groups will ALWAYS be in this order:
// 1. Pinned - Always sorted by reportDisplayName
// 2. Outstanding IOUs - Always sorted by iouReportAmount with the largest amounts at the top of the group
// 3. Drafts - Always sorted by reportDisplayName
// 4. Non-archived reports and settled IOUs
// The LHN is split into four distinct groups, and each group is sorted a little differently. The groups will ALWAYS be in this order:
// 1. Pinned/GBR - Always sorted by reportDisplayName
// 2. Drafts - Always sorted by reportDisplayName
// 3. Non-archived reports and settled IOUs
// - Sorted by lastVisibleActionCreated in default (most recent) view mode
// - Sorted by reportDisplayName in GSD (focus) view mode
// 5. Archived reports
// 4. Archived reports
// - Sorted by lastVisibleActionCreated in default (most recent) view mode
// - Sorted by reportDisplayName in GSD (focus) view mode
const pinnedReports: Report[] = [];
const outstandingIOUReports: Report[] = [];
const pinnedAndGBRReports: Report[] = [];
const draftReports: Report[] = [];
const nonArchivedReports: Report[] = [];
const archivedReports: Report[] = [];
reportsToDisplay.forEach((report) => {
if (report.isPinned) {
pinnedReports.push(report);
} else if (ReportUtils.isWaitingForIOUActionFromCurrentUser(report)) {
outstandingIOUReports.push(report);
const isPinned = report.isPinned ?? false;
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report)) {
pinnedAndGBRReports.push(report);
} else if (report.hasDraft) {
draftReports.push(report);
} else if (ReportUtils.isArchivedRoom(report)) {
Expand All @@ -198,12 +195,7 @@ function getOrderedReportIDs(
});

// Sort each group of reports accordingly
pinnedReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));
outstandingIOUReports.sort((a, b) => {
const compareAmounts = a?.iouReportAmount && b?.iouReportAmount ? b.iouReportAmount - a.iouReportAmount : 0;
const compareDisplayNames = a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0;
return compareAmounts || compareDisplayNames;
});
pinnedAndGBRReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));
draftReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));

if (isInDefaultMode) {
Expand All @@ -221,7 +213,7 @@ function getOrderedReportIDs(

// Now that we have all the reports grouped and sorted, they must be flattened into an array and only return the reportID.
// The order the arrays are concatenated in matters and will determine the order that the groups are displayed in the sidebar.
const LHNReports = [...pinnedReports, ...outstandingIOUReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
setWithLimit(reportIDsCache, cachedReportsKey, LHNReports);
return LHNReports;
}
Expand Down Expand Up @@ -254,6 +246,7 @@ type OptionData = {
searchText?: string | null;
isPinned?: boolean | null;
hasOutstandingIOU?: boolean | null;
hasOutstandingChildRequest?: boolean | null;
iouReportID?: string | null;
isIOUReportOwner?: boolean | null;
iouReportAmount?: number | null;
Expand All @@ -267,8 +260,8 @@ type OptionData = {
isAllowedToComment?: boolean | null;
isThread?: boolean | null;
isTaskReport?: boolean | null;
isWaitingForTaskCompleteFromAssignee?: boolean | null;
parentReportID?: string | null;
parentReportAction?: ReportAction;
notificationPreference?: string | number | null;
displayNamesWithTooltips?: DisplayNamesWithTooltip[] | null;
chatType?: ValueOf<typeof CONST.REPORT.CHAT_TYPE> | null;
Expand Down Expand Up @@ -338,6 +331,7 @@ function getOptionData(
searchText: null,
isPinned: false,
hasOutstandingIOU: false,
hasOutstandingChildRequest: false,
iouReportID: null,
isIOUReportOwner: null,
iouReportAmount: 0,
Expand All @@ -357,9 +351,7 @@ function getOptionData(
result.isThread = ReportUtils.isChatThread(report);
result.isChatRoom = ReportUtils.isChatRoom(report);
result.isTaskReport = ReportUtils.isTaskReport(report);
if (result.isTaskReport) {
result.isWaitingForTaskCompleteFromAssignee = ReportUtils.isWaitingForTaskCompleteFromAssignee(report, parentReportAction);
}
result.parentReportAction = parentReportAction;
result.isArchivedRoom = ReportUtils.isArchivedRoom(report);
result.isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
result.isExpenseRequest = ReportUtils.isExpenseRequest(report);
Expand All @@ -382,6 +374,7 @@ function getOptionData(
result.keyForList = String(report.reportID);
result.tooltipText = ReportUtils.getReportParticipantsTitle(report.participantAccountIDs ?? []);
result.hasOutstandingIOU = report.hasOutstandingIOU;
result.hasOutstandingChildRequest = report.hasOutstandingChildRequest;
result.parentReportID = report.parentReportID ?? null;
result.isWaitingOnBankAccount = report.isWaitingOnBankAccount;
result.notificationPreference = report.notificationPreference ?? null;
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/sidebar/SidebarLinksData.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ const chatReportSelector = (report) =>
total: report.total,
nonReimbursableTotal: report.nonReimbursableTotal,
hasOutstandingIOU: report.hasOutstandingIOU,
hasOutstandingChildRequest: report.hasOutstandingChildRequest,
isWaitingOnBankAccount: report.isWaitingOnBankAccount,
statusNum: report.statusNum,
stateNum: report.stateNum,
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ type Report = {
/** Whether there is an outstanding amount in IOU */
hasOutstandingIOU?: boolean;

/** Whether the report has a child that is an outstanding money request that is awaiting action from the current user */
hasOutstandingChildRequest?: boolean;

/** List of icons for report participants */
icons?: OnyxCommon.Icon[];

Expand Down
38 changes: 28 additions & 10 deletions tests/unit/ReportUtilsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,17 @@ describe('ReportUtils', () => {
});
});

describe('isWaitingForIOUActionFromCurrentUser', () => {
describe('requiresAttentionFromCurrentUser', () => {
it('returns false when there is no report', () => {
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser()).toBe(false);
expect(ReportUtils.requiresAttentionFromCurrentUser()).toBe(false);
});
it('returns false when the matched IOU report does not have an owner accountID', () => {
const report = {
...LHNTestUtils.getFakeReport(),
ownerAccountID: undefined,
hasOutstandingIOU: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(false);
});
it('returns false when the linked iou report has an oustanding IOU', () => {
const report = {
Expand All @@ -268,17 +268,17 @@ describe('ReportUtils', () => {
ownerAccountID: 99,
hasOutstandingIOU: true,
}).then(() => {
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(false);
});
});
it('returns true when the report has no outstanding IOU but is waiting for a bank account and the logged user is the report owner', () => {
it('returns false when the report has no outstanding IOU but is waiting for a bank account and the logged user is the report owner', () => {
const report = {
...LHNTestUtils.getFakeReport(),
hasOutstandingIOU: false,
ownerAccountID: currentUserAccountID,
isWaitingOnBankAccount: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(true);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(false);
});
it('returns false when the report has outstanding IOU and is not waiting for a bank account and the logged user is the report owner', () => {
const report = {
Expand All @@ -287,7 +287,7 @@ describe('ReportUtils', () => {
ownerAccountID: currentUserAccountID,
isWaitingOnBankAccount: false,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(false);
});
it('returns false when the report has no oustanding IOU but is waiting for a bank account and the logged user is not the report owner', () => {
const report = {
Expand All @@ -296,16 +296,34 @@ describe('ReportUtils', () => {
ownerAccountID: 97,
isWaitingOnBankAccount: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(false);
});
it('returns true when the report has oustanding IOU', () => {
it('returns true when the report has an unread mention', () => {
const report = {
...LHNTestUtils.getFakeReport(),
isUnreadWithMention: true,
};
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(true);
});
it('returns true when the report is an outstanding task', () => {
const report = {
...LHNTestUtils.getFakeReport(),
type: CONST.REPORT.TYPE.TASK,
managerID: currentUserAccountID,
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS.OPEN,
};
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(true);
});
it('returns true when the report has oustanding child request', () => {
const report = {
...LHNTestUtils.getFakeReport(),
ownerAccountID: 99,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
isWaitingOnBankAccount: false,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(true);
expect(ReportUtils.requiresAttentionFromCurrentUser(report)).toBe(true);
});
});

Expand Down
Loading
Loading