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 2023-07-12] [$1000] mWeb - Chat - 'Hmm... it's not here' page is displayed when we go back from concierge after we open invalid user #19996

Closed
1 of 6 tasks
kbecciv opened this issue Jun 1, 2023 · 66 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Jun 1, 2023

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


Action Performed:

  1. Open the app
  2. Go to search and search for any invalid phone or email
    An invalid phone number meaning a non-mobile phone line. (Landline)
  3. Click on result
  4. Close the error
  5. On concierge page, click on back of phone or swipe back

Expected Result:

App should not display its not here page after getting back from concierge after opening invalid user

Actual Result:

App displays 'Hmm... it's not here' page when we get back from concierge after opening invalid user

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.22.0

Reproducible in staging?: n/a

Reproducible in production?: n/a

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

VID-20230602-WA0003.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685645535899559?thread_ts=1685385849.445229&cid=C049HHMV9SM

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0148400a911dd6cf26
  • Upwork Job ID: 1666097434807554048
  • Last Price Increase: 2023-06-27
@kbecciv kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jun 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 1, 2023

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 1, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Overdue label Jun 5, 2023
@johncschuster
Copy link
Contributor

I've asked in Slack for clarity about what qualifies as an invalid phone number/email address so we can update the repro steps.

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2023
@johncschuster
Copy link
Contributor

From Slack:

I used landline numbers (eg: +916541237890) as we can't start chat with them. Not sure landline number format in US though.

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Jun 6, 2023
@melvin-bot melvin-bot bot changed the title mWeb - Chat - 'Hmm... it's not here' page is displayed when we go back from concierge after we open invalid user [$1000] mWeb - Chat - 'Hmm... it's not here' page is displayed when we go back from concierge after we open invalid user Jun 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0148400a911dd6cf26

@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2023

Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2023

Triggered auto assignment to @robertjchen (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@ShogunFire
Copy link
Contributor

Is it just me that can't arrive on the error page ? This is what I have for mweb:

2023-06-07.18-15-57.mp4

I also tried on prod and same result...

On Android native I have the error page

@parasharrajat
Copy link
Member

Two things:

  1. I don't think this is a bug. You were on the invalid chat before navigating away to the concierge. Clicking back will obviously try to take you back which never existed being invalid.

  2. @ShogunFire I tested and yes it's you only. I faced a little different behavior where the error page Hmm...its not here was never visible to me(Different issue https://expensify.slack.com/archives/C049HHMV9SM/p1686204807304499). After using the wrong phone number, I successfully landed on the chat page with an error.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 8, 2023

The expected result section is incomplete. If the error page should not be shown, then what should happen? @dhanashree-sawant


Pushing this back to Slack https://expensify.slack.com/archives/C049HHMV9SM/p1685645535899559?thread_ts=1685385849.445229&cid=C049HHMV9SM.

@dhanashree-sawant
Copy link

dhanashree-sawant commented Jun 8, 2023

Hi @parasharrajat, generally when we go back from concierge, we go back to main home page i.e. reports archive page. I think we should display that to make it feel consistent with normal flow. From users perspective, if we already have closed the error and are taken to concierge page, I won't expect it to show 'Hmm... its not there' page. That would be confusing. Also now, it displays just a plain skeleton view which I understand you have reported about it recently.

@bernhardoj
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we start a chat with an invalid user, it will show a create chat error and show a close button. The close button will delete the invalid chat and navigate to concierge chat.

App/src/libs/actions/Report.js

Lines 1267 to 1270 in 9f1c0e0

function navigateToConciergeChatAndDeleteReport(reportID) {
navigateToConciergeChat();
deleteReport(reportID);
}

When going back, it will show the deleted invalid chat.

What is the root cause of that problem?

When we navigate to concierge chat, the deleted invalid chat is still in the stack ([LHN, Deleted Invalid Chat, Concierge]). So, when we go back, we will land on deleted invalid chat.

What changes do you think we should make in order to solve the problem?

When we navigate to concierge, we should pop the deleted invalid chat from stack.

App/src/libs/actions/Report.js

Lines 1267 to 1270 in 9f1c0e0

function navigateToConciergeChatAndDeleteReport(reportID) {
navigateToConciergeChat();
deleteReport(reportID);
}

(add Navigation.goBack() before navigateToConciergeChat)

What alternative solutions did you explore? (Optional)

Add a way to replace the navigation instead of push. So, the deleted invalid chat will be replaced with concierge chat.
(I prefer the main solution as it's much simpler)

@melvin-bot
Copy link

melvin-bot bot commented Jun 10, 2023

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@parasharrajat
Copy link
Member

@bernhardoj We have a conclusion on the Slack discussion https://expensify.slack.com/archives/C049HHMV9SM/p1685645535899559?thread_ts=1685385849.445229&cid=C049HHMV9SM. Can you confirm if your proposal is inline with it?

@melvin-bot melvin-bot bot removed the Overdue label Jun 12, 2023
@bernhardoj
Copy link
Contributor

Yes, my proposal will have the expected result

@robertjchen
Copy link
Contributor

I believe we're waiting on the BZ checklist for this one? #19996 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Jul 17, 2023
@parasharrajat
Copy link
Member

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:

  • [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: This is present from the start and it is a missing piece of our Navigation implementation.
  • [@parasharrajat] 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: NA
  • [@parasharrajat] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1689574851090789
  • [@parasharrajat] Determine if we should create a regression test for this bug. Yes
  • [@parasharrajat] 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.

Regression Test Steps

  1. Start a new chat with an invalid user, for example, +916541237890
  2. Verify you get an error creating the new chat
  3. Remove the error by pressing the close icon on the Report message error.
  4. Verify you are navigated to the Concierge chat.
  5. Go back.
  6. In iOS/Android/mWeb, verify you are brought back to LHN.
  7. In Web/Desktop, verify you are brought back to the previous chat that is opened before creating the new chat with +916541237890

Do you agree 👍 or 👎 ?

@melvin-bot melvin-bot bot added the Overdue label Jul 19, 2023
@johncschuster
Copy link
Contributor

The job has been closed on Upwork. I'll get a new one going.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 19, 2023
@robertjchen
Copy link
Contributor

@johncschuster Got it, should we close this one out then?

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2023
@github-project-automation github-project-automation bot moved this from Review Proposals to Done in Navigation Refactor Follow-ups Jul 24, 2023
@robertjchen robertjchen reopened this Jul 24, 2023
@johncschuster
Copy link
Contributor

I'm having trouble with Upwork (no surprises there) with creating the new job. I'm going to keep this issue open while I work on that so I don't miss paying everyone.

@parasharrajat
Copy link
Member

Requested payment 1500.

@johncschuster
Copy link
Contributor

@dhanashree-sawant and @bernhardoj I have sent contracts to both of you. Can you please comment here when you've accepted so I can issue payment? Thank you!

@melvin-bot melvin-bot bot removed the Overdue label Jul 26, 2023
@dhanashree-sawant
Copy link

Thanks @johncschuster, Offer accepted.

@bernhardoj
Copy link
Contributor

@johncschuster Accepted

@JmillsExpensify
Copy link

Reviewed details for @parasharrajat. This is accurate based on summary from the thumbs up from the Business Reviewer and approved for payment in NewDot.

@johncschuster
Copy link
Contributor

Summary of payment:

External issue reporter - @dhanashree-sawant - $250
Contributor that fixed the issue - @bernhardoj - $1500
Contributor+ that helped on the issue and/or PR - @parasharrajat - $1500

@melvin-bot melvin-bot bot added the Overdue label Jul 30, 2023
@johncschuster
Copy link
Contributor

@JmillsExpensify, I've issued payment on Upwork for @dhanashree-sawant and @bernhardoj and it looks like payment should be issued to @parasharrajat via NewDot. Do we need to keep this issue open until that payment is issued or are we good to close this out?

@melvin-bot melvin-bot bot removed the Overdue label Jul 30, 2023
@JmillsExpensify
Copy link

You can close it. I've approved payments.

@melvin-bot melvin-bot bot added the Overdue label Aug 1, 2023
@johncschuster
Copy link
Contributor

Sweet! Thank you!

@melvin-bot melvin-bot bot removed the Overdue label Aug 1, 2023
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. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Development

No branches or pull requests

9 participants