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

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

Merged
merged 15 commits into from
Oct 31, 2023
+42 −48
Merged

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

Show file tree
Hide file tree
Changes from 2 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 && (optionItem.isUnreadWithMention || optionItem.isWaitingForTaskCompleteFromAssignee || ReportUtils.shouldShowGBR(optionItem));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be cleaner if we move isUnreadWithMention and isWaitingForTaskCompleteFromAssignee into the shouldShowGBR function. So all the logic of when to show GBR is in there.


/**
* Show the ReportActionContextMenu modal popover.
Expand Down
28 changes: 6 additions & 22 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,12 +1213,12 @@ function getDisplayNamesWithTooltips(personalDetailsList, isMultipleParticipantR
}

/**
* 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 @@ -1227,29 +1227,13 @@ 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;
}

// Current user is an admin and the report has been approved but not settled yet
return policy.role === CONST.POLICY.ROLE.ADMIN && isReportApproved(report);
}

// 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;
    }

if (report.hasOutstandingIOU && report.ownerAccountID === currentUserAccountID && report.isWaitingOnBankAccount) {
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 @@ -3136,7 +3120,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 @@ -3948,7 +3932,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
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 false when the report has no oustanding 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(false);
expect(ReportUtils.shouldShowGBR(report)).toBe(false);
});
it('returns true when the report has oustanding IOU and is 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: true,
};
expect(ReportUtils.isWaitingForIOUActionFromCurrentUser(report)).toBe(true);
expect(ReportUtils.shouldShowGBR(report)).toBe(true);
});
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
Loading