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

Show local time in money request and 1:1 chat threads #26845

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Show local time in money request and 1:1 chat threads #26845

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0be9688
Show local time in money request and 1:1 chat threads
huzaifa-99 Sep 6, 2023
0c5d253
Merge branch 'main' into 24228-fix-user-local-time
huzaifa-99 Sep 11, 2023
24e0a47
Simplify logic for participant local time in report utils
huzaifa-99 Sep 11, 2023
723b331
Merge branch 'main' into 24228-fix-user-local-time
huzaifa-99 Sep 19, 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
32 changes: 30 additions & 2 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,8 +833,36 @@ function hasAutomatedExpensifyAccountIDs(accountIDs) {
* @returns {Array}
*/
function getReportRecipientAccountIDs(report, currentLoginAccountID) {
const participantAccountIDs = isTaskReport(report) ? [report.managerID] : lodashGet(report, 'participantAccountIDs');
const reportParticipants = _.without(participantAccountIDs, currentLoginAccountID);
let finalReport = report;
// In 1:1 chat threads, the participants will be the same as parent report. If a report is specifically a 1:1 chat thread then we will
// get parent report and use its participants array.
if (isThread(report) && !(isTaskReport(report) || isMoneyRequestReport(report))) {
const parentReport = lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`]);
if (hasSingleParticipant(parentReport)) {
finalReport = parentReport;
}
}
Comment on lines +847 to +854
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recap: For 1:1 chat threads, we are not showing the local time of User B to User A, until User B replies in User A's thread.

The logic here will show the local time of User B to User A, even if they don't reply in User A's thread. Again this is in case of 1:1 chat b/w User A and User B


// Most report types will have the same initial participants
const initialParticipantAccountIDs = lodashGet(finalReport, 'participantAccountIDs');

// Most reports like the money request and task reports don't add the `ownerAccountId` in `participantAccountIDs`, for those types of
// reports we will include `ownerAccountId` in the `finalParticipantAccountIDs` array.
let finalParticipantAccountIDs = initialParticipantAccountIDs;

// For money requests i.e the IOU (1:1 person) and Expense (1:* person) reports, use the full `initialParticipantAccountIDs` array
// and add the `ownerAccountId`.
if (isMoneyRequestReport(report)) {
finalParticipantAccountIDs = _.union(initialParticipantAccountIDs, [report.ownerAccountID]);
}

// Task reports `managerID` will change when assignee is changed, in that case the old `managerID` is still present in `participantAccountIDs`
// array along with the new one. We only need the `managerID` as a participant here.
if (isTaskReport(report)) {
finalParticipantAccountIDs = [report.managerID];
}
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 haven't changed how local time calculation for task reports works. This is because there was some work done in #25534 a few days ago that added this behavior. But for reference, I did include the task report behavior in test videos.

Just for a recap, I originally suggested that we show User A's local time to User B and User B's local time to User A for task reports (along with other suggestions) but #25534 added the behavior that will always show the current task assignee's local time to all users who open the task report. If the assigned user themselves opens the task report they won't see the local time message.

It looks like, for showing local time in Task reports we are giving preference to assigned user instead of share destination. So if a User assigns a task to another User, and then opens the task report, they should be able to see the assigned User's local time, regardless of where they share the task.

I believe the current behavior is good but curious as to what you think.

cc: @ArekChr @roryabraham

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it works, I think we could simplify this code like this.

Suggested change
// Most report types will have the same initial participants
const initialParticipantAccountIDs = lodashGet(finalReport, 'participantAccountIDs');
// Most reports like the money request and task reports don't add the `ownerAccountId` in `participantAccountIDs`, for those types of
// reports we will include `ownerAccountId` in the `finalParticipantAccountIDs` array.
let finalParticipantAccountIDs = initialParticipantAccountIDs;
// For money requests i.e the IOU (1:1 person) and Expense (1:* person) reports, use the full `initialParticipantAccountIDs` array
// and add the `ownerAccountId`.
if (isMoneyRequestReport(report)) {
finalParticipantAccountIDs = _.union(initialParticipantAccountIDs, [report.ownerAccountID]);
}
// Task reports `managerID` will change when assignee is changed, in that case the old `managerID` is still present in `participantAccountIDs`
// array along with the new one. We only need the `managerID` as a participant here.
if (isTaskReport(report)) {
finalParticipantAccountIDs = [report.managerID];
}
let finalParticipantAccountIDs = [];
if (isMoneyRequestReport(report)) {
// For money requests i.e the IOU (1:1 person) and Expense (1:* person) reports, use the full `initialParticipantAccountIDs` array
// and add the `ownerAccountId`.
finalParticipantAccountIDs = _.union(lodashGet(finalReport, 'participantAccountIDs'), [report.ownerAccountID]);
} else if (isTaskReport(report)) {
// Task reports `managerID` will change when assignee is changed, in that case the old `managerID` is still present in `participantAccountIDs`
// array along with the new one. We only need the `managerID` as a participant here.
finalParticipantAccountIDs = [report.managerID];
} else {
// Most reports like the money request and task reports don't add the `ownerAccountId` in `participantAccountIDs`, for those types of
// reports we will include `ownerAccountId` in the `finalParticipantAccountIDs` array.
finalParticipantAccountIDs = lodashGet(finalReport, 'participantAccountIDs');
}

Copy link
Contributor Author

@huzaifa-99 huzaifa-99 Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@ArekChr Done here. I also removed a comment as it felt redundant


const reportParticipants = _.without(finalParticipantAccountIDs, currentLoginAccountID);
const participantsWithoutExpensifyAccountIDs = _.difference(reportParticipants, CONST.EXPENSIFY_ACCOUNT_IDS);
return participantsWithoutExpensifyAccountIDs;
}
Expand Down
Loading