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

[HOLD for payment 2024-05-09] [$250] [DEV]User invited by mentioning from a concierge whisper can't see parent message #39982

Closed
1 of 6 tasks
m-natarajan opened this issue Apr 10, 2024 · 39 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 10, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: Main
Reproducible in staging?: Main
Reproducible in production?: Main
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
Expensify/Expensify Issue URL:
Issue reported by: @rayane-djouah
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712697976186759

Action Performed:

  1. Click on FAB > Start chat
  2. Create a group chat with Users B and C
  3. Send a message in the group chat
  4. Right-click on the message > Reply in the thread
  5. Mention User D and invite User D from the Concierge whisper
  6. As User D, navigate to the thread

Expected Result:

User D can see the parent ancestor message in the thread

Actual Result:

User D can not see the parent ancestor message in the thread.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01453399d887f12ed6
  • Upwork Job ID: 1779911560255762432
  • Last Price Increase: 2024-04-22
  • Automatic offers:
    • fedirjh | Reviewer | 0
    • rayane-djouah | Contributor | 0
Issue OwnerCurrent Issue Owner: @zanyrenney
@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Apr 10, 2024
Copy link

melvin-bot bot commented Apr 10, 2024

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@brkdavis
Copy link

brkdavis commented Apr 10, 2024

@m-natarajan : Tried steps in latest version of Chrome. I am not able to reproduce the issue in staging with email addresses created using same parent email (ones created using adding '+' in the email) as well as an entirely independent email address. I can see the parent of the thread for both users. Please see screenshot below.

image

But from a usability standpoint, it doesn't look like a parent - child tree view, rather is a flat list stating 'Thread' and 'Replies'. As an end user, I would have liked to have this view as a configurable entry.

@rayane-djouah
Copy link
Contributor

@brkdavis this bug is currently only reproducible on main, not on staging

@rayane-djouah
Copy link
Contributor

@m-natarajan @zanyrenney this is reproducible now on staging; it may be a deploy blocker

@m-natarajan m-natarajan added the DeployBlockerCash This issue or pull request should block deployment label Apr 10, 2024
Copy link

melvin-bot bot commented Apr 10, 2024

Triggered auto assignment to @tylerkaraszewski (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Apr 10, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@Julesssss Julesssss added Daily KSv2 and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment labels Apr 11, 2024
@Julesssss
Copy link
Contributor

This is pretty minor and should not block our deploy

@rayane-djouah
Copy link
Contributor

This is blocking this PR: #39343

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01453399d887f12ed6

@melvin-bot melvin-bot bot changed the title [DEV]User invited by mentioning from a concierge whisper can't see parent message [$250] [DEV]User invited by mentioning from a concierge whisper can't see parent message Apr 15, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

@melvin-bot melvin-bot bot removed the Overdue label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 24, 2024

@tylerkaraszewski @fedirjh @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@zanyrenney
Copy link
Contributor

@rayane-djouah do you want to have a look at the feedback provided and see if you can amend your proposal and resubmit?

@rayane-djouah
Copy link
Contributor

I will give an update today

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Apr 25, 2024

Proposal

Updated

@fedirjh, the changes didn't work in for you because parentReport can be undefined, and it's passed to ReportActionsUtils.isCurrentActionUnread which has giving a type error. I updated my proposal to add a {} fallback to parentReport when calling ReportActionsUtils.isCurrentActionUnread.

User D can not see the parent ancestor message in the thread.
Does the user D have access to the parent report? If user D doesn’t have access to the parent report then, then how will they obtain the data for the header (the parent ancestor message)? isn't the data retrieved from the report actions inside the parent report?

the backend send reportActions_ data only with one report action (the parent report action) and doesn't send report_ data.

Screenshot 2024-04-25 at 5 14 27 PM Screenshot 2024-04-25 at 5 14 52 PM Screenshot 2024-04-25 at 5 15 59 PM

@fedirjh
Copy link
Contributor

fedirjh commented Apr 29, 2024

@rayane-djouah Thank you for the update. Now it is working for me. The root cause makes sense to me. For the solution, I think we shouldn't break when the parentReport is empty or undefined, we should keep the current behavior as it is. Just we should update it like this :

For getAllAncestorReportActions

// keep same logic for parentReportAction and remove parentReport check.
if (!parentReportAction || ReportActionsUtils.isTransactionThread(parentReportAction)) {
    break;
}

const isParentReportActionUnread = ReportActionsUtils.isCurrentActionUnread(parentReport ?? {}, parentReportAction);

allAncestors.push({
    report: currentReport,
    reportAction: parentReportAction,
    shouldDisplayNewMarker: isParentReportActionUnread,
});

if (!parentReport) {
    break;
}

Similar for getAllAncestorReportActionIDs

// keep same logic for parentReportAction and remove parentReport check.
if (!parentReportActionID || (!includeTransactionThread && ReportActionsUtils.isTransactionThread(parentReportAction))) {
    break;
}

allAncestorIDs.reportIDs.push(parentReportID ?? '');
allAncestorIDs.reportActionsIDs.push(parentReportActionID ?? '');

if (!parentReport) {
    break;
}

Let me know if that makes sense to you.

@fedirjh
Copy link
Contributor

fedirjh commented Apr 29, 2024

I think we can move with @rayane-djouah Proposal. The proposal looks good to me, the root cause is accurate and solution as well. Will request these little changes in the PR.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 29, 2024

Current assignee @tylerkaraszewski is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@rayane-djouah
Copy link
Contributor

Thank you for the review @fedirjh, The suggested changes makes sense, I updated my proposal accordingly.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Apr 29, 2024

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@zanyrenney
Copy link
Contributor

hey @rayane-djouah assigning you to this job. Please accept the upwork offers for both roles cc. @fedirjh so we can payout asap upon fix.

@rayane-djouah
Copy link
Contributor

PR ready for review

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 2, 2024
@melvin-bot melvin-bot bot changed the title [$250] [DEV]User invited by mentioning from a concierge whisper can't see parent message [HOLD for payment 2024-05-09] [$250] [DEV]User invited by mentioning from a concierge whisper can't see parent message May 2, 2024
Copy link

melvin-bot bot commented May 2, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 2, 2024
Copy link

melvin-bot bot commented May 2, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-05-09. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented May 2, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@fedirjh / @rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
  • [@fedirjh / @rayane-djouah] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@fedirjh / @rayane-djouah] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@fedirjh / @rayane-djouah] Determine if we should create a regression test for this bug.
  • [@fedirjh / @rayane-djouah] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@zanyrenney
Copy link
Contributor

Payment summary
@fedirjh requires payment automatic offer (Reviewer) - paid $250 via Upwork
@rayane-djouah requires payment automatic offer (Contributor) paid $250 via Upwork

@zanyrenney
Copy link
Contributor

Oh oops. I am a day early actually @fedirjh I don't imagine there will be a regression from this but please LMK if there is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants