[$250] Web- Search - Central pane is Search while LHN is inbox after clicking back button on card RHP

IuliiaHerets commented 2 weeks ago

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: v9.0.25-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com/search/filters/card
Click on the RHP back button.

Expected Result:

Central pane will remain as chat report.

Actual Result:

Central pane changes to Search, while LHN remain as Inbox.

Workaround:

Unknown

Platforms:

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/71271345-d149-4ff9-83dc-b95d1d667404

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0116f769d438f95ddb
Upwork Job ID: 1829370703781172325
Last Price Increase: 2024-08-30
Automatic offers:

jjcoffee | Reviewer | 103755576
Nodebrute | Contributor | 103755578

Issue Owner

Current Issue Owner: @jjcoffee

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @VictoriaExpensify (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.

IuliiaHerets commented 2 weeks ago

We think that this bug might be related to #wave-control

IuliiaHerets commented 2 weeks ago

@VictoriaExpensify 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

Nodebrute commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-27 11:35:35 UTC.

Proposal

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

Central pane is Search while LHN is inbox after clicking back button on card RHP

What is the root cause of that problem?

When navigating to links such as https://dev.new.expensify.com:8082/search/filters/merchant, the background correctly shows the search central pane and search side bar. However, this consistency is not observed on the card link (https://dev.new.expensify.com:8082/search/filters/card). We are not adding SCREENS.SEARCH.ADVANCED_FILTERS_CARD_RHP in https://github.com/Expensify/App/blob/9ddca5c4ac8168ad36da570192a6e4ef10820e67/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts#L44

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

We should add SCREENS.SEARCH.ADVANCED_FILTERS_CARD_RHP here for consistent behaviour.

    [SCREENS.SEARCH.CENTRAL_PANE]: [
        SCREENS.SEARCH.REPORT_RHP,
        SCREENS.SEARCH.TRANSACTION_HOLD_REASON_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_CURRENCY_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_DATE_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_DESCRIPTION_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_MERCHANT_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_REPORT_ID_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_AMOUNT_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_CATEGORY_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_KEYWORD_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_TAX_RATE_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_EXPENSE_TYPE_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_TAG_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_FROM_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_TO_RHP,
        SCREENS.SEARCH.ADVANCED_FILTERS_CARD_RHP
    ],

We should also investigate other areas where this error might be occurring.

What alternative solutions did you explore? (Optional)

VictoriaExpensify commented 2 weeks ago

I agree this is an issue that we should solve. I think wave_control is the best place for it

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~0116f769d438f95ddb

melvin-bot[bot] commented 2 weeks ago

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

jjcoffee commented 2 weeks ago

@Nodebrute's proposal LGTM, I guess we can have a look to see if there are any other SCREENS.SEARCH that need to be added there if appropriate.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

📣 @jjcoffee 🎉 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

melvin-bot[bot] commented 2 weeks ago

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

VictoriaExpensify commented 5 days ago

"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:

[ ] [@jjcoffee ] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@jjcoffee ] 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:
[ ] [@jjcoffee ] 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:
[ ] [@jjcoffee ] Determine if we should create a regression test for this bug.
[ ] [@jjcoffee ] 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.
[ ] [@VictoriaExpensify ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:"

VictoriaExpensify commented 5 days ago

Payment summary: Contributor: @Nodebrute paid $250 via Upwork

@jjcoffee I'll process your payment once the checklist actions are complete. Thanks!

jjcoffee commented 4 days ago

The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/46666
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: https://github.com/Expensify/App/pull/46666#issuecomment-2343061434
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: N/A - testing should've caught it
Determine if we should create a regression test for this bug. Yes
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 Proposal

Go to staging.new.expensify.com/search/filters/card.
On desktop (web/app): Verify that the central panel is the search panel.
Click on the RHP back button.
Verify the central panel is still the search panel.

Do we agree 👍 or 👎

jjcoffee commented 4 days ago

@VictoriaExpensify All done!

Expensify / App