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

Web - Magic input showing along with delivery failure page for suspended accounts #23544

Closed
1 of 6 tasks
kbecciv opened this issue Jul 25, 2023 · 9 comments
Closed
1 of 6 tasks
Assignees
Labels
Engineering Hourly KSv2 Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Jul 25, 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. Signout if you are signed in
  2. Use a suspended email to sign in (carora755@gmail.com)
  3. Enter the email
  4. Click next

Expected Result:

Only delivery error page should be shown

Actual Result:

Magic input showing along with delivery failure page for suspended accounts

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: Dev 1.3.45.1
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

Screen.Recording.2023-07-24.at.8.17.36.PM.mov
Recording.3897.mp4

Expensify/Expensify Issue URL:
Issue reported by: @chiragxarora
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690210023703719

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 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

@kbecciv
Copy link
Author

kbecciv commented Jul 25, 2023

Proposal

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

magic input showing along with delivery failure page for suspended accounts

What is the root cause of that problem?

Root cause of the issue is the wrong logic or we could say missed check in getRenderOptions() in SignInPage.

const shouldShowEmailDeliveryFailurePage = hasLogin && hasEmailDeliveryFailure;
const isUnvalidatedSecondaryLogin = hasLogin && !isPrimaryLogin && !isAccountValidated;
const shouldShowValidateCodeForm = hasAccount && (hasLogin || hasValidateCode) && !isUnvalidatedSecondaryLogin;

Here when in response to api call to /beginsignin we receive hasEmailDeliveryFailure true for the suspended emails, then we are showing delivery failure page using shouldShowEmailDeliveryFailurePage. And for showing magic input form we have boolean shouldShowValidateCodeForm and as visible in the code above, it does not check for the suspended emails. That is why we see both magic input and delivery failure page together since both booleans evaluate to true in such case.

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

We need to check for suspended emails too before showing magic input form and for that we can add an additional boolean AND to the shouldShowValidateCodeForm and render the code form only when shouldShowEmailDeliveryFailurePage (or hasEmailDeliveryFailure) is false meaning the account is not suspended.

Results
### What alternative solutions did you explore? (Optional) Not sure if welcome header is supposed to be shown with delivery error page, if not, similar condition can be added to that too

@hungvu193
Copy link
Contributor

hungvu193 commented Jul 25, 2023

dupe #23466 cc @jliexpensify

@kbecciv kbecciv added DeployBlockerCash This issue or pull request should block deployment and removed Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 25, 2023
@kbecciv kbecciv changed the title Dev: Web - Magic input showing along with delivery failure page for suspended accounts Web - Magic input showing along with delivery failure page for suspended accounts Jul 25, 2023
@OSBotify
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.

@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2023

Triggered auto assignment to @amyevans (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@amyevans
Copy link
Contributor

As previously stated this is a duplicate of #23466 so no need for payments (unassigning you @jliexpensify!). Since the PR fix (#23512) is still open and unreviewed, I'll hop in there to review and move things along quicker.

@amyevans
Copy link
Contributor

Looks like the fix for this has been CPed, I'll test on staging to confirm

@amyevans
Copy link
Contributor

Looks good:

Screenshot 2023-07-25 at 2 58 46 PM

Closing this out and I'll check it off the deploy checklist

@marcaaron marcaaron removed the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Hourly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

6 participants