anmurali commented 3 weeks ago

Problem: When a user switches to classic, we open www.expensify.com in a new tab. It isn't intuitive to them to close that tab on web. As a result they switch over and over again.

Solution: When a user switches to Classic, open www.expensify.com in the same tab, rather than a new tab

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021854652609104860244
Upwork Job ID: 1854652609104860244
Last Price Increase: 2024-11-07
Automatic offers:

allgandalf | Reviewer | 104852713

Issue Owner

Current Issue Owner: @kadiealexander

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

truph01 commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-11-08 08:54:50 UTC.

Proposal

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

Switch to classic in the same tab

What is the root cause of that problem?

In:

https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/libs/asyncOpenURL/index.website.ts#L19

we always call Linking.openURL which will open a new tab.

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

In: https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/libs/asyncOpenURL/index.website.ts#L9 we should introduce new param shouldOpenInSameTab:

const asyncOpenURL: AsyncOpenURL = (promise, url, shouldSkipCustomSafariLogic, shouldOpenInSameTab = false) => {

and use it in:

https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/libs/asyncOpenURL/index.website.ts#L19

                if (shouldOpenInSameTab) {
                    window.location.href = typeof url === 'string' ? url : url(params);
                    return;
                }
                Linking.openURL(typeof url === 'string' ? url : url(params));

Then we should update:

https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/libs/actions/Link.ts#L72

function openOldDotLink(url: string, shouldOpenInSameTab = false) {

and

https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/libs/actions/Link.ts#L84

        (oldDotURL) => oldDotURL, undefined, shouldOpenInSameTab

Finally, update:

https://github.com/Expensify/App/blob/7f55e030d9cb3aca84a991c5a058011b9ba9e5eb/src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx#L86

                        Link.openOldDotLink(CONST.OLDDOT_URLS.INBOX, true);

What alternative solutions did you explore? (Optional)

anmurali commented 3 weeks ago

@allgandalf this is a critical issue as we have a lot of confused users, so appreciate you picking a proposal from above as soon as possible!

Anaslancer commented 3 weeks ago

Proposal

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

Switch to classic in the same tab

What is the root cause of that problem?

To open a new URL, we use Linking.openURL finally. Btw, this function opens URLs in the device’s default browser or a compatible app, without options to specify the tab. So we should add new logic for opening URLs at the same tab.

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

I changed this function; https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/libs/actions/Link.ts#L72-L86 The changed function is following;

function openOldDotLink(url: string, shouldOpenInSameTab: boolean = false) {
    if (isNetworkOffline) {
        buildOldDotURL(url).then((oldDotURL) => openExternalLink(oldDotURL));
        return;
    }

    // If shortLivedAuthToken is not accessible, fallback to opening the link without the token.
    // eslint-disable-next-line rulesdir/no-api-side-effects-method
    const promise = API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.OPEN_OLD_DOT_LINK, {}, {})
                        .then((response) => (response ? buildOldDotURL(url, response.shortLivedAuthToken) : buildOldDotURL(url)))
                        .catch(() => buildOldDotURL(url));

    if (shouldOpenInSameTab && Platform.OS === 'web') {
        promise.then((oldDotURL) => window.open(oldDotURL, '_self'));
    } else {
        asyncOpenURL(
            promise,
            (oldDotURL) => oldDotURL,
        );
    }
}

Next, we should pass a param for opening URLs at same tab. https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx#L86 Link.openOldDotLink(CONST.OLDDOT_URLS.INBOX, true);

What alternative solutions did you explore? (Optional)

N/A

Contributor details

Your Expensify account email: anasup1995@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

melvin-bot[bot] commented 3 weeks ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

Anaslancer commented 3 weeks ago

https://github.com/user-attachments/assets/cf99c312-2050-4ab1-a7cd-00560bb7bee3

bernhardoj commented 3 weeks ago

Proposal

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

We want to open the expensify classic on the same tab.

What is the root cause of that problem?

We use Linking.openURL which by default always opens the URL on a new tab.

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

First, add a new param to tell whether to open the URL on the same tab or not. Let's call it shouldOpenInSameTab. https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx#L86

We will pass this param to openOldDotLink, openExternalLink, and asyncOpenURL. https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/libs/actions/Link.ts#L68-L86

In asyncOpenURL (the .website.ts file), if shouldOpenInSameTab is true, we will always use the Linking.openURL (the if block). https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/libs/asyncOpenURL/index.website.ts#L14-L25

Then, pass _self as the target if shouldOpenInSameTab is true. target param for Linking.openURL is available for react-native-web.

Linking.openURL(typeof url === 'string' ? url : url(params), shouldOpenInSameTab ? '_self' : undefined);

truph01 commented 3 weeks ago

Updated proposal

allgandalf commented 3 weeks ago

Thanks for the proposals everyone 🙏

Let's go with @bernhardoj's proposal here, their RCA is correct and solution makes sense to me

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 weeks ago

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

neil-marcellini commented 3 weeks ago

Heads up I'm OOO next week and won't have time to review this before then.

anmurali commented 3 weeks ago

@allgandalf - I unassigned Neil. Can you re-add the C+ reviewed message so we can get someone else assigned?

allgandalf commented 3 weeks ago

🎀👀🎀 C+ reviewed, summary here @chiragsalian

melvin-bot[bot] commented 3 weeks ago

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

chiragsalian commented 3 weeks ago

Proposal LGTM, feel free to create the PR @bernhardoj.

melvin-bot[bot] commented 3 weeks ago

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

bernhardoj commented 3 weeks ago

PR is ready

cc: @allgandalf

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

https://github.com/Expensify/App/pull/52352

If no regressions arise, payment will be issued on 2024-11-29. :confetti_ball:

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

@bernhardoj requires payment through NewDot Manual Requests
@allgandalf requires payment automatic offer (Reviewer)

allgandalf commented 6 days ago

BugZero Checklist:

[x] [Contributor] Classify the bug:

Bug classification

Source of bug: - [x] 1a. Result of the original design (eg. a case wasn't considered) - [ ] 1b. Mistake during implementation - [ ] 1c. Backend bug - [ ] 1z. Other: Where bug was reported: - [ ] 2a. Reported on production - [ ] 2b. Reported on staging (deploy blocker) - [x] 2c. Reported on both staging and production - [ ] 2d. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [x] 3b. Expensify employee - [ ] 3c. Contributor - [ ] 3d. QA - [ ] 3z. Other:

[x] [Contributor] 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: N/A (It always worked this way, we decided to change the behaviour)
[x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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
[x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[x] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue: https://github.com/Expensify/Expensify/issues/448657

Regression Test Proposal

Precondition:

None

Test:

Open Settings > Switch to Expensify Classic
Proceed to the last page and press Switch to Expensify Classis

For web/mWeb,

Verify the expensify classic is shown on the current tab

For desktop/iOS/Android,

Verify the expensify classic is shown on a new window/tab

Do we agree 👍 or 👎

allgandalf commented 6 days ago

@anmurali can you please assign a BZ for payment or pay out this one :) thanks

melvin-bot[bot] commented 6 days ago

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

trjExpensify commented 6 days ago

@anmurali can you please assign a BZ for payment or pay out this one :) thanks

Done, payment due tomorrow!

kadiealexander commented 6 days ago

Payouts due:

[ ] Contributor: $150 @bernhardoj via manual request
[x] Reviewer: $150 @allgandalf (automatic offer)

Upwork job is here.

bernhardoj commented 5 days ago

Requested in ND.

Expensify / App

[HOLD for payment 2024-11-29] [$150] Switch to classic in the same tab #52225

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Contributor details

Proposal

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

What is the root cause of that problem?

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

Updated proposal

BugZero Checklist:

Regression Test Proposal

Precondition:

Test: