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

kbecciv · 2023-09-15T20:31:09Z

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

Action Performed:

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

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

Screen.Recording.2023-09-15.at.10.41.25.mov

Recording.4516.mp4

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

The text was updated successfully, but these errors were encountered:

melvin-bot · 2023-09-15T20:31:13Z

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

melvin-bot · 2023-09-15T20:31:14Z

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

melvin-bot · 2023-09-15T20:31:15Z

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

melvin-bot · 2023-09-15T20:31:18Z

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

melvin-bot · 2023-09-15T20:31:24Z

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

rayane-d · 2023-09-16T00:50:31Z

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:

App/src/components/MoneyRequestConfirmationList.js

Line 189 in 76c1559

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

We disable the Date and the Merchant fields if the IOU type is not a Request:

App/src/components/MoneyRequestConfirmationList.js

Line 490 in 76c1559

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

App/src/components/MoneyRequestConfirmationList.js

Line 500 in 76c1559

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

App/src/components/MoneyRequestConfirmationList.js

Line 510 in 76c1559

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

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:

App/src/CONST.ts

Lines 1048 to 1052 in 76c1559

    
           MONEY_REQUEST_TYPE: { 
        
               SEND: 'send', 
        
               SPLIT: 'split', 
        
               REQUEST: 'request', 
        
           },

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:

28.New.Expensify.-.Google.Chrome.2023-09-16.00-57-52.mp4

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-d · 2023-09-16T00:51:04Z

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

hungvu193 · 2023-09-16T01:16:41Z

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

App/src/components/MoneyRequestConfirmationList.js

Line 490 in c36ac0c

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

App/src/components/MoneyRequestConfirmationList.js

Line 510 in c36ac0c

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

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 · 2023-09-16T01:40:53Z

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:

App/src/pages/iou/MoneyRequestMerchantPage.js

Lines 110 to 119 in 76c1559

    
           <TextInput 
        
               inputID="moneyRequestMerchant" 
        
               name="moneyRequestMerchant" 
        
               defaultValue={iou.merchant} 
        
               maxLength={CONST.MERCHANT_NAME_MAX_LENGTH} 
        
               label={translate('common.merchant')} 
        
               accessibilityLabel={translate('common.merchant')} 
        
               accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT} 
        
               ref={(el) => (inputRef.current = el)} 
        
           />

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:

App/src/pages/iou/MoneyRequestDatePage.js

Lines 100 to 104 in 76c1559

    
           <NewDatePicker 
        
               inputID="moneyRequestCreated" 
        
               label={translate('common.date')} 
        
               defaultValue={iou.created} 
        
               maxDate={new Date()}

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 · 2023-09-16T14:04:30Z

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

App/src/components/MoneyRequestConfirmationList.js

Line 490 in c36ac0c

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

App/src/components/MoneyRequestConfirmationList.js

Line 510 in c36ac0c

disabled={didConfirm || props.isReadOnly || !isTypeRequest}

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 · 2023-09-18T12:20:08Z

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

ntdiary · 2023-09-18T12:34:25Z

will review soon. : )

ntdiary · 2023-09-18T12:59:38Z

@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 · 2023-09-18T15:58:24Z

@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 · 2023-09-18T20:18:34Z

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 · 2023-09-20T10:36:15Z

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 · 2023-09-22T16:00:18Z

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

bfitzexpensify · 2023-09-26T09:38:05Z

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

ntdiary · 2023-10-13T13:37:21Z

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 · 2023-10-17T12:23:55Z

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

bfitzexpensify · 2023-10-18T15:46:30Z

@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 · 2023-10-20T03:21:36Z

@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 · 2023-10-20T18:23:58Z

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

bfitzexpensify · 2023-11-01T00:21:13Z

Still holding on #26538 - though that itself has been taken off hold now, which is good

bfitzexpensify · 2023-11-09T21:04:33Z

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

bfitzexpensify · 2023-11-21T23:21:19Z

Testing about to start on that PR in the holding issue

bfitzexpensify · 2023-12-03T12:48:18Z

still held

bfitzexpensify · 2023-12-12T21:16:27Z

Still on hold, that issue is moving

bfitzexpensify · 2023-12-22T13:30:51Z

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

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023

melvin-bot bot assigned bfitzexpensify Sep 15, 2023

melvin-bot bot changed the title ~~Web - Date/Merchant is disabled for split, but still accessible with deeplink~~ [$500] Web - Date/Merchant is disabled for split, but still accessible with deeplink Sep 15, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 15, 2023

melvin-bot bot assigned puneetlath Sep 15, 2023

melvin-bot bot assigned ntdiary Sep 15, 2023

melvin-bot bot added the Overdue label Sep 18, 2023

melvin-bot bot removed the Overdue label Sep 18, 2023

bfitzexpensify unassigned puneetlath Sep 18, 2023

melvin-bot bot added the Overdue label Sep 22, 2023

melvin-bot bot added the Overdue label Oct 16, 2023

melvin-bot bot removed the Overdue label Oct 18, 2023

ntdiary mentioned this issue Oct 20, 2023

[HOLD #26538] [$500] Settings - Using deep link, admin can request money to user workspace chat even though its not allowed #26523

Closed

6 tasks

bfitzexpensify changed the title ~~[$500] Web - Date/Merchant is disabled for split, but still accessible with deeplink~~ [HOLD FOR #26538][$500] Web - Date/Merchant is disabled for split, but still accessible with deeplink Oct 20, 2023

bfitzexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 20, 2023

melvin-bot bot added the Overdue label Oct 30, 2023

melvin-bot bot removed the Overdue label Nov 1, 2023

melvin-bot bot added the Overdue label Nov 9, 2023

melvin-bot bot removed the Overdue label Nov 9, 2023

melvin-bot bot added the Overdue label Nov 20, 2023

melvin-bot bot removed the Overdue label Nov 21, 2023

melvin-bot bot added the Overdue label Dec 1, 2023

melvin-bot bot removed the Overdue label Dec 3, 2023

melvin-bot bot added the Overdue label Dec 11, 2023

melvin-bot bot removed the Overdue label Dec 12, 2023

melvin-bot bot added the Overdue label Dec 21, 2023

bfitzexpensify closed this as completed Dec 22, 2023

melvin-bot bot removed the Overdue label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kbecciv commented Sep 15, 2023 •

edited

Loading

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023 •

edited by bfitzexpensify

Loading

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

rayane-d commented Sep 16, 2023

rayane-d commented Sep 16, 2023

hungvu193 commented Sep 16, 2023

neonbhai commented Sep 16, 2023 •

edited

Loading

For Merchant Page (MoneyRequestMerchantPage.js)

For Date Page (MoneyRequestDatePage.js)

kbecciv commented Sep 16, 2023

bfitzexpensify commented Sep 18, 2023

ntdiary commented Sep 18, 2023

ntdiary commented Sep 18, 2023 •

edited

Loading

bfitzexpensify commented Sep 18, 2023

puneetlath commented Sep 18, 2023

bfitzexpensify commented Sep 20, 2023

melvin-bot bot commented Sep 22, 2023

bfitzexpensify commented Sep 26, 2023

ntdiary commented Oct 13, 2023

melvin-bot bot commented Oct 17, 2023

bfitzexpensify commented Oct 18, 2023

ntdiary commented Oct 20, 2023

bfitzexpensify commented Oct 20, 2023

bfitzexpensify commented Nov 1, 2023

bfitzexpensify commented Nov 9, 2023 •

edited

Loading

bfitzexpensify commented Nov 21, 2023

bfitzexpensify commented Dec 3, 2023

bfitzexpensify commented Dec 12, 2023

bfitzexpensify commented Dec 22, 2023

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

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

Comments

kbecciv commented Sep 15, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023 • edited by bfitzexpensify Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

rayane-d commented Sep 16, 2023

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)

rayane-d commented Sep 16, 2023

hungvu193 commented Sep 16, 2023

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)

neonbhai commented Sep 16, 2023 • edited Loading

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?

For Merchant Page (MoneyRequestMerchantPage.js)

For Date Page (MoneyRequestDatePage.js)

What alternative solutions did you explore? (Optional)

kbecciv commented Sep 16, 2023

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)

bfitzexpensify commented Sep 18, 2023

ntdiary commented Sep 18, 2023

ntdiary commented Sep 18, 2023 • edited Loading

bfitzexpensify commented Sep 18, 2023

puneetlath commented Sep 18, 2023

bfitzexpensify commented Sep 20, 2023

melvin-bot bot commented Sep 22, 2023

bfitzexpensify commented Sep 26, 2023

ntdiary commented Oct 13, 2023

melvin-bot bot commented Oct 17, 2023

bfitzexpensify commented Oct 18, 2023

ntdiary commented Oct 20, 2023

bfitzexpensify commented Oct 20, 2023

bfitzexpensify commented Nov 1, 2023

bfitzexpensify commented Nov 9, 2023 • edited Loading

bfitzexpensify commented Nov 21, 2023

bfitzexpensify commented Dec 3, 2023

bfitzexpensify commented Dec 12, 2023

bfitzexpensify commented Dec 22, 2023

kbecciv commented Sep 15, 2023 •

edited

Loading

melvin-bot bot commented Sep 15, 2023 •

edited by bfitzexpensify

Loading

neonbhai commented Sep 16, 2023 •

edited

Loading

ntdiary commented Sep 18, 2023 •

edited

Loading

bfitzexpensify commented Nov 9, 2023 •

edited

Loading