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

[$500] 'Hmm its not here' displayed when refreshing expensify classic page #38865

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

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 23, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.56-0
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): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Issue found when executing PR #37955

Action Performed:

  1. Navigate to Settings - Switch to Expensify Classic
  2. Choose any option
  3. Click on the Next button
  4. Verify input is auto-focused
  5. Now refresh page

Expected Result:

'Hmm its not here' should not be displayed when refreshing expensify classic page

Actual Result:

'Hmm its not here' displayed when refreshing expensify classic page

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

Bug6423438_1711128399760.2024-03-22_22-16-14.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c71608c41784a351
  • Upwork Job ID: 1772068230275534848
  • Last Price Increase: 2024-03-25
  • Automatic offers:
    • hoangzinh | Reviewer | 0
    • GandalfGwaihir | Contributor | 0
Issue OwnerCurrent Issue Owner: @adelekennedy
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 23, 2024
Copy link

melvin-bot bot commented Mar 23, 2024

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

@lanitochka17
Copy link
Author

@adelekennedy FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@ZhenjaHorbach
Copy link
Contributor

I think this should be fixed in this PR
#38856

@allgandalf
Copy link
Contributor

allgandalf commented Mar 23, 2024

Proposal

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

The real problem here is that the back screen central pane gets to the chats window and doesn't stay on settings page

What is the root cause of that problem?

We do not map the Switch to expensify screen to Central Pane:

const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = {
[SCREENS.SETTINGS.PROFILE.ROOT]: [
SCREENS.SETTINGS.PROFILE.DISPLAY_NAME,
SCREENS.SETTINGS.PROFILE.CONTACT_METHODS,
SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_DETAILS,
SCREENS.SETTINGS.PROFILE.NEW_CONTACT_METHOD,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_DATE,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_TIME,
SCREENS.SETTINGS.PROFILE.STATUS,
SCREENS.SETTINGS.PROFILE.PRONOUNS,
SCREENS.SETTINGS.PROFILE.TIMEZONE,
SCREENS.SETTINGS.PROFILE.TIMEZONE_SELECT,
SCREENS.SETTINGS.PROFILE.LEGAL_NAME,
SCREENS.SETTINGS.PROFILE.DATE_OF_BIRTH,
SCREENS.SETTINGS.PROFILE.ADDRESS,
SCREENS.SETTINGS.PROFILE.ADDRESS_COUNTRY,
SCREENS.SETTINGS.SHARE_CODE,
],

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

Add the Screen SCREENS.SETTINGS.EXIT_SURVEY.REASON to the CENTRAL_PANE_TO_RHP_MAPPING and this issue will be solved

Also add subpages:

},
[SCREENS.SETTINGS.EXIT_SURVEY.REASON]: {
path: ROUTES.SETTINGS_EXIT_SURVEY_REASON,
},
[SCREENS.SETTINGS.EXIT_SURVEY.RESPONSE]: {
path: ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.route,
},
[SCREENS.SETTINGS.EXIT_SURVEY.CONFIRM]: {
path: ROUTES.SETTINGS_EXIT_SURVEY_CONFIRM.route,
},

What alternative solutions did you explore? (Optional)

N/A

@allgandalf
Copy link
Contributor

Note

@adelekennedy , the real bug is actually different over here, can you please check the video, the Central Pane changes on refresh

@ishpaul777
Copy link
Contributor

i dont think this is related to #38856, here issue is the background page is not persisting after the refresh which is what we should fix by adding these routes to CENTRAL_PANE_TO_RHP_MAPPING(@GandalfGwaihir's proposal)

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Mar 24, 2024

@ishpaul777
There are 2 different problems here
First related with the background page

Second with 'Hmm its not here' (which relevant for Report screen when any RHP page is open) and I think this should be fixed in #38856

For example :

Screen.Recording.2024-03-24.at.22.19.13.mov

@allgandalf
Copy link
Contributor

allgandalf commented Mar 24, 2024

@ZhenjaHorbach , I checked over here, if we implement my proposal we don't have this problem as now we have mapped the Central Pane to RHP, thanks :)

Video:

simplescreenrecorder-2024-03-25_03.27.23.mp4

c.c. @adelekennedy

@ishpaul777
Copy link
Contributor

ishpaul777 commented Mar 24, 2024

@ZhenjaHorbach good catch, i understand issue now. i think we can fix that in that PR.

@allgandalf
Copy link
Contributor

@ZhenjaHorbach good catch, i understand issue, i think we can fix that in that PR.

Yes @ishpaul777 , that issue for will surely get fixed in #38856 as it happens mostly for report pages, but we need to address the current bug which we found in this issue :)

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Mar 25, 2024
@melvin-bot melvin-bot bot changed the title 'Hmm its not here' displayed when refreshing expensify classic page [$500] 'Hmm its not here' displayed when refreshing expensify classic page Mar 25, 2024
Copy link

melvin-bot bot commented Mar 25, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01c71608c41784a351

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 25, 2024
Copy link

melvin-bot bot commented Mar 25, 2024

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

@roryabraham
Copy link
Contributor

updated #38856, this will be partially fixed by it. You'll no longer see Hmmm its not here but you will still see the account settings change to a report on refresh, which is unexpected. More changes would also be needed to fix this the rest of the way. Making this external

@roryabraham roryabraham self-assigned this Mar 25, 2024
@roryabraham
Copy link
Contributor

assigning myself since this appears to be a bug in the exit survey, which I built. @GandalfGwaihir's proposal LGTM and tests well 👍🏼

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

melvin-bot bot commented Mar 25, 2024

📣 @hoangzinh 🎉 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 Mar 25, 2024

📣 @GandalfGwaihir 🎉 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 📖

@melvin-bot melvin-bot bot added the Weekly KSv2 label Mar 25, 2024
@allgandalf
Copy link
Contributor

PR ready for review @hoangzinh :)

c.c. @roryabraham

@roryabraham
Copy link
Contributor

roryabraham commented Apr 12, 2024

PR was merged through some mysterious GitHub anomaly - there was no internal GH approver and @MonilBhavsar claims he did not merge the PR 😕

Anyways, there was an bug reported on the PR, but I think it was fixed in #38856.

So I want to do some extra testing but I think we're done here, pending payments

@roryabraham
Copy link
Contributor

That bug is still reproducible, but it's separate from this one. We can proceed with payment here since this is fixed

@roryabraham roryabraham added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Apr 12, 2024
@allgandalf
Copy link
Contributor

Thanks :)

@roryabraham roryabraham added Daily KSv2 and removed Weekly KSv2 labels Apr 12, 2024
@adelekennedy
Copy link

Payment sent!

@roryabraham
Copy link
Contributor

reported bug the other bug: https://expensify.slack.com/archives/C049HHMV9SM/p1713213269308389

@allgandalf
Copy link
Contributor

Proposal

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

hmm not here is displayed on refresh on exit survey page page

What is the root cause of that problem?

When we display hmm not here page we are only checking for the current state immediately after refresh, the useNetwork gives the value of isOffline true for the first time, we never check if the content is loading.

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

Subscribe to the ONYXKEYS.IS_SWITCHING_TO_OLD_DOT key by variable isLoading like we have already subscribed for the one of the exit survey page:

isLoading: {
key: ONYXKEYS.IS_SWITCHING_TO_OLD_DOT,
},

Similarly subscribe in other two pages and update the offline condition check below;

{isOffline && <ExitSurveyOffline />}

To:

{isOffline && isLoading && <ExitSurveyOffline />}
Result video
Screen.Recording.2024-04-16.at.2.15.43.AM.mov

@allgandalf
Copy link
Contributor

allgandalf commented Apr 15, 2024

reported bug the other bug: https://expensify.slack.com/archives/C049HHMV9SM/p1713213269308389

@roryabraham as the issue was discovered during the PR, can I have a dibs on the proposal whenever that issue comes out? I have a proposal ready as you can see above :) ?

@roryabraham
Copy link
Contributor

Posted a link to your proposal in the slack thread where the bug was reported

@allgandalf
Copy link
Contributor

@roryabraham , due to weird behaviour we faced from github during the #38907, the linked PR was never put on staging nor production, the PR was only merged in Main, can you CP it to staging please ?

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants