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

[HOLD] Feat: Update how GBR is determined for IOU/expense reports #29778

Merged
merged 15 commits into from
Oct 31, 2023
Merged

[HOLD] Feat: Update how GBR is determined for IOU/expense reports #29778

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 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
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
View file
Open in desktop
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.shouldShowGBR(optionItem);

/**
* Show the ReportActionContextMenu modal popover.
Expand Down
30 changes: 11 additions & 19 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,12 +1314,12 @@ 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)
* Determines if a report child has an outstanding request that is waiting for an action from the current user (either Pay or Add a credit bank account)
waterim marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {Object} report (chatReport or iouReport)
* @returns {boolean}
*/
function isWaitingForIOUActionFromCurrentUser(report) {
function shouldShowGBR(report) {
if (!report) {
return false;
}
Expand All @@ -1328,20 +1328,12 @@ function isWaitingForIOUActionFromCurrentUser(report) {
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;
}
if (report.isUnreadWithMention) {
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 (report.isWaitingForTaskCompleteFromAssignee) {
return true;
}

// Money request waiting for current user to add their credit bank account
Copy link
Contributor

Choose a reason for hiding this comment

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

@waterim actually, we've decided to treat this condition like the others. If a report is waiting on bank account, we will add the hasOutstandingChildRequest to the parent for the submitter and show the green dot on the parent. So let's remove this condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

@waterim I think we should remove this whole block:

    // 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) {
        return true;
    }

Expand All @@ -1350,8 +1342,8 @@ function isWaitingForIOUActionFromCurrentUser(report) {
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)) {
// Child report that is awaiting for current user to Pay
waterim marked this conversation as resolved.
Show resolved Hide resolved
if (report.hasOutstandingChildRequest) {
return true;
}

Expand Down Expand Up @@ -3283,7 +3275,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas,
}

// 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)) {
if (report.hasDraft || shouldShowGBR(report) || isWaitingForTaskCompleteFromAssignee(report)) {
waterim marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
Expand Down Expand Up @@ -4135,7 +4127,7 @@ export {
isCurrentUserTheOnlyParticipant,
hasAutomatedExpensifyAccountIDs,
hasExpensifyGuidesEmails,
isWaitingForIOUActionFromCurrentUser,
shouldShowGBR,
isIOUOwnedByCurrentUser,
getMoneyRequestReimbursableTotal,
getMoneyRequestSpendBreakdown,
Expand Down
2 changes: 1 addition & 1 deletion src/libs/SidebarUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p
reportsToDisplay.forEach((report) => {
if (report.isPinned) {
pinnedReports.push(report);
} else if (ReportUtils.isWaitingForIOUActionFromCurrentUser(report)) {
} else if (ReportUtils.shouldShowGBR(report)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a separate section for outstandingIOUReports. Can we just include this in the pinned reports group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will cause multiple changes in tests, but sure, will do that

Copy link
Contributor

Choose a reason for hiding this comment

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

If you prefer to handle it in a separate PR that is also fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to have it sorted together?
I mean we always want to have pinned and just after green dots

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd just do if (report.isPinned || ReportUtils.shouldShowGBR(report)) pinnedReports.push(report);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what if green dot report will be before the pinned report, is it a correct behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's fine. We want them to be treated the same from an ordering perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's fine. We want them to be treated the same from an ordering perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and updated few tests as it doesnt sort IOU reports and pinned and IOU reports have the same order priority

outstandingIOUReports.push(report);
} else if (report.hasDraft) {
draftReports.push(report);
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/sidebar/SidebarLinksData.js
View file
Open in desktop
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
View file
Open in desktop
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 child has an outstanding request */
waterim marked this conversation as resolved.
Show resolved Hide resolved
hasOutstandingChildRequest?: boolean;

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

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

describe('isWaitingForIOUActionFromCurrentUser', () => {
describe('shouldShowGBR', () => {
it('returns false when there is no report', () => {
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser()).toBe(false);
expect(ReportUtils.shouldShowGBR()).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.shouldShowGBR(report)).toBe(false);
});
it('returns false when the linked iou report has an oustanding IOU', () => {
const report = {
Expand All @@ -268,7 +268,7 @@ describe('ReportUtils', () => {
ownerAccountID: 99,
hasOutstandingIOU: true,
}).then(() => {
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.shouldShowGBR(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', () => {
Expand All @@ -278,7 +278,7 @@ describe('ReportUtils', () => {
ownerAccountID: currentUserAccountID,
isWaitingOnBankAccount: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(true);
expect(ReportUtils.shouldShowGBR(report)).toBe(true);
});
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.shouldShowGBR(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,16 @@ describe('ReportUtils', () => {
ownerAccountID: 97,
isWaitingOnBankAccount: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(false);
expect(ReportUtils.shouldShowGBR(report)).toBe(false);
});
it('returns true when the report has oustanding IOU', () => {
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.shouldShowGBR(report)).toBe(true);
});
});

Expand Down
27 changes: 13 additions & 14 deletions tests/unit/SidebarOrderTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ describe('Sidebar', () => {
};
const report3 = {
...LHNTestUtils.getFakeReport([5, 6], 1),
hasOutstandingIOU: false,
hasOutstandingChildRequest: false,

// This has to be added after the IOU report is generated
iouReportID: null,
Expand All @@ -395,7 +395,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 2,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 10000,
currency: 'USD',
chatReportID: report3.reportID,
Expand All @@ -404,7 +404,6 @@ describe('Sidebar', () => {
const currentReportId = report2.reportID;
const currentlyLoggedInUserAccountID = 9;
LHNTestUtils.getDefaultRenderedSidebarLinks(currentReportId);

return (
waitForBatchedUpdates()
// When Onyx is updated with the data and the sidebar re-renders
Expand All @@ -431,7 +430,7 @@ describe('Sidebar', () => {
expect(screen.queryAllByTestId('Pin Icon')).toHaveLength(1);
expect(screen.queryAllByTestId('Pencil Icon')).toHaveLength(1);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('One, Two');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Email Two owes $100.00');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Email Two paid $100.00');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this change from owes to paid if all we're doing in this PR is determining whether or not the GBR should be displayed?

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 was thinking about the same thing, but tests were failing, because they were receiving this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably more places where “ hasOutstandingIOU” was doing something
Cc: @puneetlath

Copy link
Contributor

@akinwale akinwale Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I assumed tests failing meant something probably broke in the logic. 😅

Will wait for input from @puneetlath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akinwale updated, changed to previous, hasOutstandingIOU was needed for a correct behaviour

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this change is no longer in the code? I didn't see it when reviewing.

expect(lodashGet(displayNames, [2, 'props', 'children'])).toBe('Three, Four');
})
);
Expand Down Expand Up @@ -733,7 +732,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 2,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 10000,
currency: 'USD',
chatReportID: report3.reportID,
Expand All @@ -743,7 +742,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 3,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 10000,
currency: 'USD',
chatReportID: report3.reportID,
Expand All @@ -753,7 +752,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 4,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 100000,
currency: 'USD',
chatReportID: report3.reportID,
Expand All @@ -763,7 +762,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 5,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 10000,
currency: 'USD',
chatReportID: report3.reportID,
Expand All @@ -773,7 +772,7 @@ describe('Sidebar', () => {
type: CONST.REPORT.TYPE.IOU,
ownerAccountID: 2,
managerID: 6,
hasOutstandingIOU: true,
hasOutstandingChildRequest: true,
total: 10000,
currency: 'USD',
chatReportID: report3.reportID,
Expand Down Expand Up @@ -814,11 +813,11 @@ describe('Sidebar', () => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(5);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Email Four owes $1,000.00');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Email Five owes $100.00');
expect(lodashGet(displayNames, [2, 'props', 'children'])).toBe('Email Six owes $100.00');
expect(lodashGet(displayNames, [3, 'props', 'children'])).toBe('Email Three owes $100.00');
expect(lodashGet(displayNames, [4, 'props', 'children'])).toBe('Email Two owes $100.00');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Email Four paid $1,000.00');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Email Five paid $100.00');
expect(lodashGet(displayNames, [2, 'props', 'children'])).toBe('Email Six paid $100.00');
expect(lodashGet(displayNames, [3, 'props', 'children'])).toBe('Email Three paid $100.00');
expect(lodashGet(displayNames, [4, 'props', 'children'])).toBe('Email Two paid $100.00');
})
);
});
Expand Down
Loading