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

fix: task not working in self dm through shortcut #39107

Closed
wants to merge 10 commits into from
Closed

fix: task not working in self dm through shortcut #39107

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
44d2e1c
fix: task not working in self dm through shortcut
nexarvo Mar 27, 2024
a748fbc
fix: lint issue
nexarvo Mar 27, 2024
e4cd5d2
change paramter name
nexarvo Mar 27, 2024
b68937c
use key to get report
nexarvo Mar 27, 2024
aeeb77b
Merge branch 'main' into fix/39049-self-dm-task-shortcut
nexarvo Mar 28, 2024
6e12a35
pass report to setAssigneeValue from components itself
nexarvo Mar 28, 2024
3cf9606
remove getChatByReportID as it is not needed
nexarvo Mar 28, 2024
45d8dd9
update conditions if chat report not available
nexarvo Mar 28, 2024
cdc9371
fix: prettier and lint issues
nexarvo Mar 28, 2024
282cd58
fix: lint
nexarvo Mar 28, 2024
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
9 changes: 9 additions & 0 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4423,7 +4423,7 @@
return true;
}

/**

Check failure on line 4426 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`
* Attempts to find a report in onyx with the provided list of participants. Does not include threads, task, money request, room, and policy expense chat.
*/
function getChatByParticipants(newParticipantList: number[], reports: OnyxCollection<Report> = allReports): OnyxEntry<Report> {
Expand All @@ -4449,6 +4449,14 @@
);
}

function getChatByReportID(reportID: string, reports: OnyxCollection<Report> = allReports) {
return (
Object.values(reports ?? {}).find((report) => {
return report?.reportID === reportID;
}) ?? null
Copy link
Contributor

Choose a reason for hiding this comment

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

I am very confused by this... I think it's doing nothing? ie: if it is null then default to null

Copy link
Contributor

@ishpaul777 ishpaul777 Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why not allReports[${ONYXKEYS.COLLECTION.REPORT}${reportID}] ?

return (
 allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it sends null then the existing behaviour create new optimistic report which is expected because there is no report found with that reportID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishpaul777 changing it to your suggestion as it returns in O(1) and works as expected also

Copy link
Contributor

Choose a reason for hiding this comment

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

I am just curious, why do we need a reports param 🤔

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 copied the above function and forgot to remove the reports param 😅

Copy link
Contributor

@ishpaul777 ishpaul777 Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually we already have this utility

https://github.com/Expensify/App/blob/e542ba7c2d9e216c2cd401de0de8ed225fff4a7b/src/libs/ReportUtils.ts#L532C10-L532C19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishpaul777 I tested every case where we create task. And in one case the shareToReportID is empty:

App/src/libs/actions/Task.ts

Line 652 in b68937c

function setAssigneeValue(assigneeEmail: string, assigneeAccountID: number, shareToReportID: string, isCurrentUser = false): OnyxEntry<OnyxTypes.Report> {

In that case we can use the old existing behaviour. And if shareToReportID is available we will get report directly by ID using utility function which is the cases we are solving i.e. selfDM

The code:

App/src/libs/actions/Task.ts

Line 659 in e542ba7

chatReport = ReportUtils.getChatByParticipants([assigneeAccountID]);

will become:
Screenshot 2024-03-28 at 3 03 16 AM

);
}

/**
* Attempts to find a report in onyx with the provided list of participants in given policy
*/
Expand Down Expand Up @@ -5735,6 +5743,7 @@
shouldCreateNewMoneyRequestReport,
isTrackExpenseReport,
hasActionsWithErrors,
getChatByReportID,
};

export type {
Expand Down
3 changes: 2 additions & 1 deletion src/libs/actions/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,8 @@ function setAssigneeValue(assigneeEmail: string, assigneeAccountID: number, shar
let chatReport: OnyxEntry<OnyxTypes.Report> = null;

if (!isCurrentUser) {
chatReport = ReportUtils.getChatByParticipants([assigneeAccountID]);
const reportID = shareDestination;
Copy link
Contributor

Choose a reason for hiding this comment

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

The param name shareDestination is a reportID? If so, can we rename this to something like shareToReportID?

Copy link
Contributor Author

@nexarvo nexarvo Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's reportID. Changing that to shareToReportID

chatReport = ReportUtils.getChatByReportID(reportID);
Copy link
Contributor

@ishpaul777 ishpaul777 Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

orrr we already load allReports in actions/Task we can just use it here

Suggested change
chatReport = ReportUtils.getChatByReportID(reportID);
chatReport = allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]

if (!chatReport) {
chatReport = ReportUtils.buildOptimisticChatReport([assigneeAccountID]);
chatReport.isOptimisticReport = true;
Expand Down
Loading