[HOLD FOR #26538][$500] Web - Date/Merchant is disabled for split, but still accessible with deeplink

[kbecciv]

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. Click plus button => Split, enter amount, press Next.
2. Choose few members, press Next.
3. At Confirmation Page, change the link from /split/new/confirmation/ to /split/new/date/ or /split/new/merchant/
4. Notice these pages are accessible.

Expected Result:

Merchant and Date shouldn't be accessible through deeplink

Actual Result:

Merchant and Date are accessible through deeplink

Workaround:

Unknown

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.70.5 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): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/875654cc-1a74-4e9c-a816-2cc736900fb9

https://github.com/Expensify/App/assets/93399543/5590bf53-9212-40a3-af8f-3f5a0eb2ee58

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c67f584444b66580
• Upwork Job ID: 1702782136315203584
• Last Price Increase: 2023-10-06

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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[bot]]

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

[melvin-bot[bot]]

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

[rayane-djouah]

Proposal

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

The Merchant and Date pages within the "Split Bill" flow are accessible via deeplinks, even though they should only be accessible in the "Request Money (IOU)" flow.

Here in MoneyRequestConfirmationList component: We determine if the IOU type is a Request: https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/components/MoneyRequestConfirmationList.js#L189-L189 We disable the Date and the Merchant fields if the IOU type is not a Request: https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/components/MoneyRequestConfirmationList.js#L490-L490 https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/components/MoneyRequestConfirmationList.js#L500-L500 https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/components/MoneyRequestConfirmationList.js#L510-L510

However, when accessing the Merchant and Date pages via deeplinks, there isn't any condition to check the iouType before rendering the pages.

What is the root cause of that problem?

The MoneyRequestMerchantPage and MoneyRequestDatePage components do not have a condition to check the iouType and prevent rendering when the type is not "request". This oversight allows users to access these pages via deeplinks even when they shouldn't be accessible.

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

To address this issue, we should wrap the content of both the MoneyRequestMerchantPage and MoneyRequestDatePage components in the FullPageNotFoundView component when the iouType is not "request".

import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView';

function MoneyRequestMerchantPage({iou, route}) {
    const iouType = lodashGet(route, 'params.iouType', '');

    return (
        <FullPageNotFoundView shouldShow={iouType !== CONST.IOU.MONEY_REQUEST_TYPE.REQUEST}>
            {/* Rest of the component's content */}
        </FullPageNotFoundView>
    );
}

function MoneyRequestDatePage({iou, route}) {
    const iouType = lodashGet(route, 'params.iouType', '');

    return (
        <FullPageNotFoundView shouldShow={iouType !== CONST.IOU.MONEY_REQUEST_TYPE.REQUEST}>
            {/* Rest of the component's content */}
        </FullPageNotFoundView>
    );
}

Reference from CONST: https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/CONST.ts#L1048-L1052

By making these changes, when a user tries to access the Merchant or Date pages with the iouType "split" or "send" via deeplinks, they will be presented with a "Not Found" page, while the rest of the content remains wrapped and won't be displayed.

Result:

https://github.com/Expensify/App/assets/77965000/4d6138da-88ce-4ff6-9234-7ca79d64a220

What alternative solutions did you explore? (Optional)

An alternative solution would be to conditionally navigate the user back to the previous page or the main page of the application.

useEffect(() => {

    // Add this condition to check if iouType is 'split' and navigate back
    if (iouType !== CONST.IOU.MONEY_REQUEST_TYPE.REQUEST) {
        Navigation.goBack();
        return;
    }

However, displaying a "Not Found" page provides a clearer indication to the user that the page they are trying to access is not available.

[rayane-djouah]

@ntdiary could you please review my proposal, thank you!

[hungvu193]

Seems @kbecciv forgot to post my proposal where I'm the reporter:

Proposal

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

Date/Merchant is disabled for split, but still accessible with deeplink

What is the root cause of that problem?

We only disabled the MenuItem for the split but didn't have the logic to goBack or prevent user from accessing the MoneyRequestDatePage and MoneyRequestMerchantPage when they create the split then access the merchant, date deeplink https://github.com/Expensify/App/blob/c36ac0c98cf8a92a5b7136bbf090550cca5ea90c/src/components/MoneyRequestConfirmationList.js#L490 https://github.com/Expensify/App/blob/c36ac0c98cf8a92a5b7136bbf090550cca5ea90c/src/components/MoneyRequestConfirmationList.js#L510

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

Create a new variable isTypeRequest.

 const isTypeRequest = iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST;

Update our useEffect to goBack when our request is not money request.

if (!isDistanceRequest && (_.isEmpty(iou.participants) || (iou.amount === 0 && !iou.receiptPath) || shouldReset || !isTypeRequest)) {
  Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType, reportID), true);
}

We can do it similar to other pages like merchant, distance.

We can also wrap our view with FullScreenNotFound when the request is not money request.

What alternative solutions did you explore? (Optional)

N/A

[neonbhai]

Proposal

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

Date/Merchant is disabled for split, but still accessible with deeplink

What is the root cause of that problem?

We don't have the disable logic on the merchant and date page.

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

• For Merchant Page (MoneyRequestMerchantPage.js)

  Instead of not showing the whole page, we can just disable the TextInput here: https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/pages/iou/MoneyRequestMerchantPage.js#L110-L119

  by passing the disabled prop or making the editable prop false if iouType is SPLIT or DISTANCE

  editable={iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST}

  This gives the impression, that the (merchant even though it exists) is uneditable.

• For Date Page (MoneyRequestDatePage.js)

  We can only edit the TextInput via the picker, but we currently don't have the mechanism to disable showing the picker. We can add this by adding an optional showPicker prop to the NewDatePicker component: https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/pages/iou/MoneyRequestDatePage.js#L100-L104 The picker below textinput will not show if we want the date TextInput to be uneditable. (Making datePicker uneditable is a useful functionality that the component is missing)

These changes will result in us showing the default and uneditable values for deeplinked merchant and date pages.

What alternative solutions did you explore? (Optional)

xx

[kbecciv]

Proposal by: @hungvu193 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694749185834509

Proposal

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

Date/Merchant is disabled for split, but still accessible with deeplink

What is the root cause of that problem?

We only disabled the MenuItem for the split but didn't have the logic to goBack or prevent user from accessing the MoneyRequestDatePage and MoneyRequestMerchantPage when they create the split then access the merchant, date deeplink https://github.com/Expensify/App/blob/c36ac0c98cf8a92a5b7136bbf090550cca5ea90c/src/components/MoneyRequestConfirmationList.js#L490 https://github.com/Expensify/App/blob/c36ac0c98cf8a92a5b7136bbf090550cca5ea90c/src/components/MoneyRequestConfirmationList.js#L510

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

Create a new variable isTypeRequest.

 const isTypeRequest = iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST;

Update our useEffect to goBack when our request is not money request.

if (!isDistanceRequest && (_.isEmpty(iou.participants) || (iou.amount === 0 && !iou.receiptPath) || shouldReset || !isTypeRequest)) {
  Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType, reportID), true);
}

We can do it similar to other pages like merchant, distance.

We can also wrap our view with FullScreenNotFound when the request is not money request.

What alternative solutions did you explore? (Optional)

N/A

[bfitzexpensify]

A couple of proposals for you to review when you get a chance @ntdiary

[ntdiary]

will review soon. : )

[ntdiary]

@bfitzexpensify, personally, I think it's okay to display "it's not here" if the page is not accessible (i.e., @rayane-djouah's proposal). Do we need to confirm this expected result with the design team again?

[bfitzexpensify]

@ntdiary yes, I think we have a well-established principle of using the "it's not here" page when a link is incorrect, so that should be fine here too

[puneetlath]

We're having a bit of a discussion in this thread about a broader solution to the issue of using a direct link to go to a specific page on a form that you shouldn't be able to access without filling in prior info: https://expensify.slack.com/archives/C049HHMV9SM/p1695064187785649?thread_ts=1694439235.983099&cid=C049HHMV9SM

[bfitzexpensify]

Nice. Looks like that thread ended without a solid conclusion, so bumped Tienifr to move their proposal forward to get a consensus. Once that's reached, I'll come back here to clarify the expected behaviour for this issue

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[bfitzexpensify]

We're still figuring out in https://expensify.slack.com/archives/C01GTK53T8Q/p1695209109095579 if we can standardize on a single form of UX

[bfitzexpensify]

Same update

[melvin-bot[bot]]

@ntdiary @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@ntdiary, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[bfitzexpensify]

Bumped that thread again

[melvin-bot[bot]]

@ntdiary @bfitzexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot[bot]]

@ntdiary, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[bfitzexpensify]

OK, from that thread, it sounds like there's general buy-in to go with this approach:

create a useScreenGuard(validationRule, fallbackScreen) hook. This will also use the same props and Onyx data available to the page to validate.

Essentially, the idea is that if you deep link to page 2 but page 1 isn’t filled, we'll fall back on page 1

@tienifr as the author of that proposal, does that sound correct?

[bfitzexpensify]

Bump on the above @tienifr

[bfitzexpensify]

OK, so we'll want to use a similar approach as shared by @tienifr in https://github.com/Expensify/App/issues/27572#issuecomment-1756999099. I'm guessing we'll need proposals rewritten given that context @ntdiary?

[melvin-bot[bot]]

@ntdiary @bfitzexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @ntdiary is eligible for the Internal assigner, not assigning anyone new.

[ntdiary]

OK, so we'll want to use a similar approach as shared by @tienifr in #27572 (comment). I'm guessing we'll need proposals rewritten given that context @ntdiary?

This has not been fully confirmed in Slack yet

[melvin-bot[bot]]

@ntdiary, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bfitzexpensify]

@ntdiary just seeing your latest comment in that Slack thread - do you feel we have enough of a consensus to move forward with a particular approach?

[ntdiary]

@bfitzexpensify, wow, I just realized we are also planning to refactor the money request flow (the PR #28618 @tgolen mentioned in that slack thread). And I don't think this issue is very critical and urgent, so it should also be fine to put this on hold for issue #26538. 🙂

[bfitzexpensify]

Cool - updating title. I agree this isn't an urgent issue. Going to move this to weekly to match that holding issue

[bfitzexpensify]

Still holding on https://github.com/Expensify/App/issues/26538 - though that itself has been taken off hold now, which is good

[bfitzexpensify]

PR being reviewed in the holding issue, we're closing to testing this again

[bfitzexpensify]

Testing about to start on that PR in the holding issue

[bfitzexpensify]

still held

[bfitzexpensify]

Still on hold, that issue is moving

[bfitzexpensify]

With the refactor, this is no longer happening. Closing this out.