-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD #299672] [$1000] Report Recipient Local Time does not appear in sender #23836
Comments
Triggered auto assignment to @CortneyOfstad ( |
Bug0 Triage Checklist (Main S/O)
|
NOTE: even after user B sent back the message there is no recipient time Screen.Recording.2023-07-29.at.10.20.50.mov |
Job added to Upwork: https://www.upwork.com/jobs/~0109b85d32df0f6e6f |
Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ArekChr ( |
@ArekChr Couple of comments above for your review 👍 TIA! |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)
|
@CortneyOfstad I think this might be a backend issue |
Okay, going to get engineering eyes on this, just in case. I'm heading OoO until 8/14 so reapply the BZ label as well to keep eyes on this while I'm out 👍 |
Triggered auto assignment to @jliexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to @PauloGasparSv ( |
Hey same here so no updates! I made progress on the issues I was working on so I'll be able to return to this ASAP, sorry for the delay! |
Thks for the help, I investigated this a bit further today. Btw, Re-opening the report (triggering a call to I'll have to start a discussion on slack about this to get some help and also discuss wether or not this is worth fixing/changing! |
Discussed this with the team and since we may implement a new NVP for checking known users I'm putting this on HOLD for https://github.com/Expensify/Expensify/issues/299672! |
Changing this to weekly, the issue we are holding had some updates but is still in progress. |
Can take this back now that I'm back in office — thanks for holding down the fort @jliexpensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.We want the local time to correctly show local time on both the sender and receiver side in threads What is the root cause of that problem?We handle participant ids differently in the case of task and money request reports. For these types of reports, we only include the assignee/requestee id in the participant ids, the assigner/requester id gets stored in the What changes do you think we should make in order to solve the problem?We can update the way we calculate report participant's here and here to include cases for task/money/simple-thread report types. // pseudocode
const initialIDs = lodashGet(report, 'participantAccountIDs', []);
const updatedIDs = (isTaskReport(report) || isMoneyRequestReport(report)) ? _.union(initialIDs, [report.ownerAccountID]) : initialIDs
const reportParticipants = _.without(updatedIDs, currentUserAccountID) and in the case of simple threads, we can get the parent report participants (we could also update the participant list upon thread creation) // pseudocode
report = isThread(report) && !(isTaskReport(report) || isMoneyRequestReport(report)) ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`]) : report and abstract all of this logic into ReportUtils. // pseudocode
function getReportParticipantIDs(report, currentUserAccountID){
const updatedReport = isThread(report) && !(isTaskReport(report) || isMoneyRequestReport(report)) ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`]) : report
const initialIDs = lodashGet(updatedReport, 'participantAccountIDs', []);
const updatedIDs = (isTaskReport(updatedReport) || isMoneyRequestReport(updatedReport)) ? _.union(initialIDs, [updatedReport.ownerAccountID]) : initialIDs
return _.without(updatedIDs, currentUserAccountID);
} I am not sure if we want to show the local time in case of workspace threads, if we don't we can add What alternative solutions did you explore? (Optional)We could also update how we set the participant ids for money/task and simple threads, but this would require some decent refactoring. |
I'm starting to get confused with one thing, more than 1 proposal sent here mentioned IOU's and Tasks's specifically but I'm pretty sure this is happening for normal 1:1 chats as long as the user's are unknown to each other no? That's why I think this fix should be done in the API and should be applied to all Chat Types. Please LMK if I'm not understanding something here : ) |
@PauloGasparSv I just came from this slack bug report (but it was a dupe) And these are 2 separate issues actually
I think we should reopen #24228. |
Thks so much for explaining @huzaifa-99 that makes total sense
They may be separate issues but I also think that fixing I think it's ok to keep that issue close for now and re-open it once this one is off HOLD in case the problem persists if that's ok : ) |
Thanks @PauloGasparSv! |
Reassigning engineering in replacement of Paulo 👍 |
Triggered auto assignment to @mountiny ( |
I think this is actually the case, its expected that you dont see the timezone of unknown user, then when they message back, iI think its fine that we only show the time when you open the report again. @CortneyOfstad @ArekChr I would close this issue as actually not a bug we need to fix. The time is correctly updated after another |
@mountiny #24228 was closed as a dupe of this issue but it looks like a different one since the users are known there. I left a comment about this here.
Can you please check if we should reopen #24228? Thank you! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
Recipient Local Time Must appear in both user A and B
Actual Result:
Recipient Local Time appears only in recipients.
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.47-3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen.Recording.2023-07-28.at.00.43.26.mov
Recording.1398.mp4
Expensify/Expensify Issue URL:
Issue reported by: @namhihi237
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690480211518189
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: