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

Feature: refactor requiresAttentionFromCurrentUser function #32140

Merged
merged 6 commits into from
Dec 11, 2023
Merged

Feature: refactor requiresAttentionFromCurrentUser function #32140

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
071db61
refactor requiresAttentionFromCurrentUserfunction
waterim Nov 28, 2023
14c7e9b
fix
waterim Nov 29, 2023
ffe819a
change
waterim Dec 4, 2023
03c3dce
Merge remote-tracking branch 'upstream/main' into feat-30668-refactor…
waterim Dec 4, 2023
88bee57
updates
waterim Dec 6, 2023
a4dcc97
updated
waterim Dec 11, 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
25 changes: 14 additions & 11 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1519,7 +1519,12 @@ function isUnreadWithMention(report: OnyxEntry<Report> | OptionData): boolean {
// lastMentionedTime and lastReadTime are both datetime strings and can be compared directly
const lastMentionedTime = report.lastMentionedTime ?? '';
const lastReadTime = report.lastReadTime ?? '';
return lastReadTime < lastMentionedTime;
return Boolean('isUnreadWithMention' in report && report.isUnreadWithMention) || lastReadTime < lastMentionedTime;
waterim marked this conversation as resolved.
Show resolved Hide resolved
waterim marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this just be this:

Suggested change
return Boolean('isUnreadWithMention' in report && report.isUnreadWithMention) || lastReadTime < lastMentionedTime;
return Boolean(report.isUnreadWithMention) || lastReadTime < lastMentionedTime;

Because if report.UnreadWithMention is undefined or null it'll be false? Or will it throw?

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, no, that was added because isUnreadWithMention is not exist in OnyxEntry and thats a verification to pass TS.
Boolean(report.isUnreadWithMention) - this will cause an error: Property 'isUnreadWithMention' does not exist on type 'Report | OptionData'.
Property 'isUnreadWithMention' does not exist on type 'Report'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh ok, I see. Perhaps then, instead of calling the argument report we should call it reportOrOption. So that it's more clear that the thing we are doing these checks on could be a report or option.

}

// Type guard to check the type of an object and it's an option
function isOption(option: OnyxEntry<Report> | OptionData): option is OptionData {
return (option as OptionData).isUnreadWithMention !== undefined;
waterim marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -1536,20 +1541,18 @@ function requiresAttentionFromCurrentUser(option: OnyxEntry<Report> | OptionData
return false;
}

if (isArchivedRoom(option)) {
return false;
}

if (isArchivedRoom(getReport(option.parentReportID))) {
if (isArchivedRoom(option) || isArchivedRoom(getReport(option.parentReportID))) {
return false;
}

if (Boolean('isUnreadWithMention' in option && option.isUnreadWithMention) || isUnreadWithMention(option)) {
return true;
}
if (isOption(option)) {
if (isUnreadWithMention(option)) {
waterim marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

if (isWaitingForAssigneeToCompleteTask(option, parentReportAction)) {
return true;
if (isWaitingForAssigneeToCompleteTask(option, 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 isWaitingForAssigneeToCompleteTask expects the first param to be report not an option.

https://github.com/Expensify/App/pull/32140/files#diff-577fcc5f3a5e4930916ff310e61c38edfa093792fe3ab55b9a30d2aecd8811dbR1511

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 only for option it was used

Copy link
Contributor

Choose a reason for hiding this comment

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

I raised this because first param report of isWaitingForAssigneeToCompleteTask has type OnyxEntry<Report>

function isWaitingForAssigneeToCompleteTask(report: OnyxEntry<Report>, parentReportAction: OnyxEntry<ReportAction> | EmptyObject = {}): boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

Restricting the use of isWaitingForAssigneeToCompleteTask will change order of the report as requiresAttentionFromCurrentUser is being used in SidebarUtils to get ordered reportIDs (getOrderedReportIDs). This is the comparison between staging and PR.

Staging

Screen.Recording.2023-12-05.at.21.03.59.mov

PR

Screen.Recording.2023-12-05.at.21.04.31.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a correct behaviour, this one related to this issue which this PR should fix!

Copy link
Contributor

@sobitneupane sobitneupane Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can notice change in other chats with green dot indicator as well. Please let me know if it is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted tasks were created from "fds"?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the content of task fds
Screenshot 2023-12-06 at 20 10 16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, agree, it was an issue, researched into the old code(before rewriting this function at all)
There was an issue before that in SidebarUtils getOrderedReportIDs we didnt use at all the "isWaitingForAssigneeToCompleteTask", but its in the function, but another part of the SidebarUtils were using "isWaitingForAssigneeToCompleteTask", but I removed it fully for reports, thats why this issue were happening.
Updated the code, removed isOption verification as its not needed anymore, added "reportAction" to "getOrderedReportIDs" for a correct behaviour, please retest and let me know

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update @waterim.

return true;
}
}

// Has a child report that is awaiting action (e.g. approve, pay, add bank account) from current user
Expand Down
1 change: 1 addition & 0 deletions tests/unit/ReportUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ describe('ReportUtils', () => {
...LHNTestUtils.getFakeReport(),
type: CONST.REPORT.TYPE.TASK,
managerID: currentUserAccountID,
isUnreadWithMention: false,
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS.OPEN,
};
Expand Down
Loading