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
Changes from 1 commit
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
Prev Previous commit
Account for both options and reports in unread check
puneetlath committed Oct 31, 2023
commit efcefdbec52e9a8e2a5b25e49791da2f44352e90
32 changes: 16 additions & 16 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
@@ -1334,6 +1334,21 @@ function isWaitingForAssigneeToCompleteTask(report, parentReportAction = {}) {
return isTaskReport(report) && isReportManager(report) && isOpenTaskReport(report, parentReportAction);
}

/**
* @param {Object} report
* @returns {Boolean}
*/
function isUnreadWithMention(report) {
if (!report) {
return false;
}

// lastMentionedTime and lastReadTime are both datetime strings and can be compared directly
const lastMentionedTime = report.lastMentionedTime || '';
const lastReadTime = report.lastReadTime || '';
return lastReadTime < lastMentionedTime;
}

/**
* 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
@@ -1353,7 +1368,7 @@ function requiresAttentionFromCurrentUser(option, parentReportAction = {}) {
return false;
}

if (option.isUnreadWithMention) {
if (option.isUnreadWithMention || isUnreadWithMention(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 fine. Though - someone could possibly do something silly like add just one of the option.lastReadTime or option.lastMentionedTime. Can protect against this by only calling isUnreadWithMention() with option when option.isUnreadWithMention is undefined.

Not gonna block on that, but I think this review has kind of highlighted that the code needs reworking or something.

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 created this follow up to try and find a way to make this better: #30668

return true;
}

@@ -3105,21 +3120,6 @@ function isUnread(report) {
return lastReadTime < lastVisibleActionCreated;
}

/**
* @param {Object} report
* @returns {Boolean}
*/
function isUnreadWithMention(report) {
if (!report) {
return false;
}

// lastMentionedTime and lastReadTime are both datetime strings and can be compared directly
const lastMentionedTime = report.lastMentionedTime || '';
const lastReadTime = report.lastReadTime || '';
return lastReadTime < lastMentionedTime;
}

/**
* @param {Object} report
* @param {Object} allReportsDict