Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.3k stars 2.74k forks source link

[HOLD for payment 2024-08-07] [$250] Add deep links for CTA links #45350

Closed marcochavezf closed 1 month ago

marcochavezf commented 1 month 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!


We want to add a couple of deep links for the following actions, where each link will open the corresponding views:

Action Link View
Track Expense /track-expense Screenshot 2024-07-12 at 11 30 51 a m
Submit Expense /submit-expense Screenshot 2024-07-12 at 11 31 51 a m

Potentially, we'd want to create something like a /concierge deep link, where we create a component that navigates to the Concierge chat. In this case, we would start the corresponding money request action using IOU.startMoneyRequest.

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01967a3704af99de82
  • Upwork Job ID: 1811816585616897261
  • Last Price Increase: 2024-07-12
  • Automatic offers:
    • ikevin127 | Reviewer | 103142789
Issue OwnerCurrent Issue Owner: @dylanexpensify
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @dylanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 month ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

melvin-bot[bot] commented 1 month ago

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

marcochavezf commented 1 month ago

This issue is frontend implementation for Harpoons for NewDot, specifically for this internal issue.

dannymcclain commented 1 month ago

Looks like nothing in the UI is actually changing for this right? I probably don't need to be assigned since this "new feature" basically has nothing to do with design—but I will wait for confirmation!

nkdengineer commented 1 month ago

Proposal

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

Add deep links for CTA links

What is the root cause of that problem?

This is a new feature

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

We can use the same implementation as we do on the concierge page. For each track-expense and submit expense create a navigation page with the corresponding route . This page will be a RHP and we can display a loading page while waiting for the navigation or something like this (this can be confirmed by design team)

And then on this page create a useEffect to navigate to the money request flow by using IOU.startMoneyRequest

Here is an example for track-expense page

function TrackExpensePage({session}: TrackExpenseProps) {
    const styles = useThemeStyles();
    const isUnmounted = useRef(false);

    useFocusEffect(() => {
        if (session && 'authToken' in session) {
            App.confirmReadyToOpenApp();
            Navigation.isNavigationReady().then(() => {
                IOU.startMoneyRequest(CONST.IOU.TYPE.TRACK, ReportUtils.findSelfDMReportID() ?? '-1')
            });
        } else {
            Navigation.navigate();
        }
    });

    return (
        <FullScreenLoadingIndicator />;
    );
}

What alternative solutions did you explore? (Optional)

kabeer95 commented 1 month ago

Proposal: Add Deep Links for CTA Links on Expensify

Problem Statement:

The current implementation of CTA links on Expensify does not provide deep links, resulting in a poor user experience. Users are not able to navigate directly to the relevant section of the report.

Root Cause:

The lack of deep links in the CTA links is due to the absence of a proper linking mechanism.

Proposed Solution:

To resolve this issue, we recommend adding deep links to the CTA links on Expensify. This can be achieved by:

Modifying the CTA link generation: Update the CTA link generation logic to include deep links that point to the relevant section of the report. Using anchor tags: Utilize anchor tags () to create deep links that navigate to the desired section of the report.

Code: import React from 'react';

interface CTALinkProps { reportId: string; sectionId: string; children: React.ReactNode; }

const CTALink: React.FC = ({ reportId, sectionId, children }) => { const href = #${sectionId};

return ( <a href={href} onClick={() => console.log(Navigate to ${sectionId} section)}> {children} ); };

export default CTALink;

const Report = () => { return (

Report

View Expenses View Income

); };

export default Report;

export default CtaLink; Alternative Solutions:

We explored the following alternative solutions:

Using a URL parameter: Pass the section ID as a URL parameter, allowing the report to navigate to the desired section. Implementing a custom linking mechanism: Develop a custom linking mechanism that uses a unique identifier to navigate to the relevant section of the report.

ikevin127 commented 1 month ago

@nkdengineer Thanks for your proposal.

I implementing the solution you proposed locally for testing purposes and while the redirect part works, when submitting the track expense -> the API call fails (responding with Auth TrackExpense returned an error), and the track expense is not created, an empty avatar report appears in LHN which goes away after dismissing the errors.

Additionally, your TrackExpensePage provided code would keep the central pane loading indefinitely, so I had to remove the following block:

if (isUnmounted.current) {
    return;
}

in order to get the redirect working and open the routed RHP.

In order for me to move on with assignment here I need proof of at least one of the deep link routes working as expected, functionality wise as well - meaning the whole track expense flow working as it would when triggered from the FAB button.

ikevin127 commented 1 month ago

@kabeer95 Thanks for your proposal. To be reviewed by Contributor+, please update your proposal comment using the proposal template.

nyomanjyotisa commented 1 month ago

Proposal

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

Add deep links for CTA links

What is the root cause of that problem?

new feature

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

Create a new route for track-expense and submit-expense and a utility page for each of them just like the ConciergePage

for submit-expense on navigation ready, start submit expense with generate report ID

IOU.startMoneyRequest(CONST.IOU.TYPE.SUBMIT, ReportUtils.generateReportID());

Then for track-expense on navigation ready, start track expense with self DM report ID

ReportUtils.getSelfDMReportID().then((reportID) => {
    IOU.startMoneyRequest(CONST.IOU.TYPE.TRACK, reportID ?? '-1');
});

we need to create new function ReportUtils.getSelfDMReportID() to make sure the self DM report ID retrieved after the allReports is not undefined. If we don't do this, we will get and error after submit track expense if we access track-expense deep link when user not logged in yet

function getSelfDMReportID(): Promise<string | undefined> {
    return new Promise((resolve) => {
        let interval = setInterval(function() {
            const allReports = ReportConnection.getAllReports();

            if (typeof allReports === 'undefined') return;
            clearInterval(interval);

            const selfDMReport = Object.values(allReports).find((report) => isSelfDM(report) && !isThread(report));
            resolve(selfDMReport?.reportID);
        }, 100);
    });
}

Or we can modify findSelfDMReportID function with this code, and update usage of findSelfDMReportID in other place

Have created a branch for the proof, please check and test this branch

RESULT

track-expense, the user already logged in

https://github.com/user-attachments/assets/64d906a1-ae17-43c2-a57a-f99180286e8d

track-expense, the user not logged in

https://github.com/user-attachments/assets/b913c513-cc71-4246-8deb-a34b4c26e5a5

Additionally, if we want to display track-training on the track-expense deep link, we can add the following code after startMoneyRequest

                if (!hasSeenTrackTraining && !isOffline) {
                    setTimeout(() => {
                        Navigation.navigate(ROUTES.TRACK_TRAINING_MODAL);
                    }, CONST.ANIMATED_TRANSITION);
                }

What alternative solutions did you explore? (Optional)

nyomanjyotisa commented 1 month ago

Proposal updated

nkdengineer commented 1 month ago

@ikevin127 I created a test branch here.

nkdengineer commented 1 month ago

I implementing the solution you proposed locally for testing purposes and while the redirect part works, when submitting the track expense -> the API call fails (responding with Auth TrackExpense returned an error), and the track expense is not created, an empty avatar report appears in LHN which goes away after dismissing the errors.

For track-expense we should find the self DM instead of generating a new reportID because only selft can create track-expense. I updated proposal https://github.com/Expensify/App/issues/45350#issuecomment-2226093175.

nyomanjyotisa commented 1 month ago

Proposal updated

marcochavezf commented 1 month ago

Looks like nothing in the UI is actually changing for this right? I probably don't need to be assigned since this "new feature" basically has nothing to do with design—but I will wait for confirmation!

Yup, no UI changes are needed for this one, I will unassign you :)

ikevin127 commented 1 month ago

@nkdengineer Thanks for the proposal update. Unfortunately the test branch implementation still does not work as expected, what I mean by this is that while submitting the track expense works now, there's a weird flicker when the RHP opens and the central pane report seems to load indefinitely.

Video https://github.com/user-attachments/assets/ed9c9249-274e-48b6-b326-23ac78ada0e9

Note: I tested your branch with both CONFIG.USE_REACT_STRICT_MODE true and false and got the same behaviour.

ikevin127 commented 1 month ago

@nyomanjyotisa's updated proposal looks good enough to me in order to move on with assignment, given that this is a NewFeature I think the details can be worked on during PR. The proposed solution addresses the issue accordingly and I appreciate the attention to detail like:

Video https://github.com/user-attachments/assets/9aa917f5-e9d2-4961-b9e2-95ef0d3732c5

One issue I found with the solution: on /submit-expense, the submission flow fails with error message There is a previously existing chat between these users. (see video above).

Regarding solution code:

Note: Because of CONFIG.USE_REACT_STRICT_MODE being true, this block of code will always have isUnmounted as true because of the double rendering StrictMode does, so for local development while working on the feature you can set CONFIG.USE_REACT_STRICT_MODE to false (just make sure to set it back before pushing the code to PR).

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @marcochavezf is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

marcochavezf commented 1 month ago

Nice, thanks for the review @ikevin127! Assigning @nyomanjyotisa 🚀

melvin-bot[bot] commented 1 month ago

📣 @ikevin127 🎉 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 1 month ago

📣 @nyomanjyotisa You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

trjExpensify commented 1 month ago

This is harpoon project related, right? Moving to #vip-vsb

marcochavezf commented 1 month ago

Not overdue, moved to #vip-vsb

marcochavezf commented 1 month ago

Hi @nyomanjyotisa, is the issue still happening?

nyomanjyotisa commented 1 month ago

Hi @marcochavezf

The latest main on desktop Chrome works perfectly, but the issue persists on mWeb Chrome. Got not found page after login

https://github.com/user-attachments/assets/7f5f89c0-1f17-4ec7-8860-71d25e76ee02

ikevin127 commented 1 month ago

@nyomanjyotisa That's fine - you can safely open the PR if that's the only issue - as it's expected to get the Not Found page when using ngrok and this happens regardless of your changes, happens to everybody that uses tunneling - including on adhoc build since we're not using the dev / staging / prod domains when we tunnel.

To test this without ngrok (on iOS only) you can setup the iPhone Simulator by running npm run setupNewDotWebForEmulators and once this is successful on iOS you will be able to connect to web on Simulator using https://dev.new.expensify.com:8082 directly in the simulator.

[!note] On your PR author checklist, for Android: mWeb just post the video even if it's showing the Not Found page as reviewers know that's not a bug and just a tunneling issue.

dylanexpensify commented 1 month ago

Ongoing

dylanexpensify commented 1 month ago

@marcochavezf mind giving us a quick update when you have a free moment 🙇‍♂️ ❤️

nyomanjyotisa commented 1 month ago

PR opened @ikevin127

And for this problem I believe we should handle it in another issue, since it gives the same error when we create an expense from the FAB button. It only works without error if we submit expense to user or workspace that are in the Recents list, or user that we never start a chat:

https://github.com/user-attachments/assets/c68976ce-b304-4d36-821a-13df8166d438

ikevin127 commented 1 month ago

@marcochavezf I think it's safe to move forward with the PR review despite this issue mentioned in https://github.com/Expensify/App/issues/45350#issuecomment-2247282205 (above) as to me this seems like a BE issue as the error message details - issue which is already happening currently via the FAB submit expense flow, therefore I think it's out of scope for this issue and should be fixed for both FAB / deep link flows once addressed in another issue.

What do you think ?

Update:

After additional testing, turns out the issue is only happening locally due to StrictMode. I figured this out by testing the submit expense flow on staging and noticed that it's not happening there. Next, I went on local dev and set CONFIG.USE_REACT_STRICT_MODE to false and the issue doesn't happen anymore, regardless on submitting to Recents workspaces / users or non-Recents.

Therefore this shouldn't be a concern since it's only happening on local dev due to StrictMode, and the flow would work without any issues once deployed on staging / production.

marcochavezf commented 1 month ago

@marcochavezf mind giving us a quick update when you have a free moment 🙇‍♂️ ❤️

Apologies for the delay here. The PR has been merged!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

If no regressions arise, payment will be issued on 2024-08-07. :confetti_ball:

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

melvin-bot[bot] commented 1 month ago

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

ikevin127 commented 1 month ago

Regression Test Proposal

A. Track expense deep link

  1. Once logged-in, append /track-expense after the expensify domain (e.g. https://staging.new.expensify.com).
  2. Verify that the Track Training is displayed if the conditions match (first time Tracking an expense / never clicked Don't show me this again on Track Training before).
  3. Click Got It.
  4. Verify that the Track Expense page is displayed.
  5. Enter amount on manual track expense.
  6. Click Next.
  7. Click Track Expense.
  8. Verify that the Track Expense request is created successfully and the Track Expense related Concierge whisper is displayed.

B. Submit expense deep link

  1. Once logged-in, append /submit-expense after the expensify domain (e.g. https://staging.new.expensify.com).
  2. Verify that the Submit Expense page is displayed.
  3. Enter amount on manual submit expense.
  4. Click Next.
  5. Select any user or workspace.
  6. (Optional) Enter Merchant (for workspace).
  7. Click Submit.
  8. Verify that the Submit Expense request is created successfully.

Do we agree 👍 or 👎.

dylanexpensify commented 1 month ago

Payment coming up!

ikevin127 commented 1 month ago

cc @dylanexpensify

dylanexpensify commented 1 month ago

Payment summary:

Please apply or request!

dylanexpensify commented 1 month ago

@ikevin127 payment sent, @nyomanjyotisa offer sent!

nyomanjyotisa commented 1 month ago

offer accepted

dylanexpensify commented 1 month ago

Done!