Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
marcochavezf opened this issue Jul 12, 2024 · 43 comments
Closed

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

marcochavezf opened this issue Jul 12, 2024 · 43 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@marcochavezf
Copy link
Contributor

marcochavezf commented Jul 12, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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
@marcochavezf marcochavezf added Daily KSv2 NewFeature Something to build that is a new item. labels Jul 12, 2024
@marcochavezf marcochavezf self-assigned this Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

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 melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

⚠️ 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 ⚠️

@marcochavezf marcochavezf added External Added to denote the issue can be worked on by a contributor Daily KSv2 and removed Weekly KSv2 labels Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

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

@melvin-bot melvin-bot bot changed the title Add deep links for CTA links [$250] Add deep links for CTA links Jul 12, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

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

Copy link

melvin-bot bot commented Jul 12, 2024

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

@marcochavezf
Copy link
Contributor Author

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

@dannymcclain
Copy link
Contributor

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
Copy link
Contributor

nkdengineer commented Jul 12, 2024

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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

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

@nyomanjyotisa
Copy link
Contributor

nyomanjyotisa commented Jul 14, 2024

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

New-Expensify.9.mp4

track-expense, the user not logged in

New-Expensify.11.mp4

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
Copy link
Contributor

Proposal updated

@nkdengineer
Copy link
Contributor

@ikevin127 I created a test branch here.

@nkdengineer
Copy link
Contributor

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 #45350 (comment).

@nyomanjyotisa
Copy link
Contributor

Proposal updated

@marcochavezf
Copy link
Contributor Author

marcochavezf commented Jul 22, 2024

Hi @nyomanjyotisa, is the issue still happening?

@melvin-bot melvin-bot bot removed the Overdue label Jul 22, 2024
@nyomanjyotisa
Copy link
Contributor

Hi @marcochavezf

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

Expensify-magic-sign-in-code-138578---jyotisa1616-gmail-com---Gmail.mp4

@ikevin127
Copy link
Contributor

ikevin127 commented Jul 23, 2024

@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
Copy link
Contributor

Ongoing

@dylanexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 24, 2024
@nyomanjyotisa
Copy link
Contributor

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:

-1-New-Expensify.8.mp4

@ikevin127
Copy link
Contributor

ikevin127 commented Jul 24, 2024

@marcochavezf I think it's safe to move forward with the PR review despite this issue mentioned in #45350 (comment) (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
Copy link
Contributor Author

@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 melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 31, 2024
@melvin-bot melvin-bot bot changed the title [$250] Add deep links for CTA links [HOLD for payment 2024-08-07] [$250] Add deep links for CTA links Jul 31, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

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

Copy link

melvin-bot bot commented Jul 31, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 31, 2024

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] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@ikevin127
Copy link
Contributor

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
Copy link
Contributor

Payment coming up!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 7, 2024
@ikevin127

This comment has been minimized.

@dylanexpensify
Copy link
Contributor

Payment summary:

Please apply or request!

@dylanexpensify
Copy link
Contributor

@ikevin127 payment sent, @nyomanjyotisa offer sent!

@nyomanjyotisa
Copy link
Contributor

offer accepted

@dylanexpensify
Copy link
Contributor

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants