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.52k stars 2.87k forks source link

[$250] mWeb - IOU - In description field, mentions are shown in preview but not shown after saved #47195

Closed IuliiaHerets closed 1 month ago

IuliiaHerets commented 2 months 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!


Version Number: 9.0.18 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Tap fab -- submit expense
  3. Enter an amount and tap next
  4. Select a user
  5. Enter a mention #23 in description field and tap save

Expected Result:

In description field, mentions are shown in preview and same way must be shown after saved.

Actual Result:

In description field, mentions are shown in preview but not shown after saved.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/32b020f8-eefb-4c7f-8ca4-d766d1c0834c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017a5e03ee4d6aec11
  • Upwork Job ID: 1824172920701419661
  • Last Price Increase: 2024-08-15
  • Automatic offers:
    • alitoshmatov | Reviewer | 103587305
melvin-bot[bot] commented 2 months ago

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

IuliiaHerets commented 2 months ago

We think that this bug might be related to #vip-vsb

IuliiaHerets commented 2 months ago

@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

daledah commented 2 months ago

Proposal

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

What is the root cause of that problem?

so the mention style is not applied.

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

and then in here: https://github.com/Expensify/App/blob/93dc1c91437ea8cb9bd48a2a65f395e6da3c678b/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx#L60 use:

    const contextMenuContext = useContext(ShowContextMenuContext);
    const currentReportID = contextMenuContext?.currentReportID ? {currentReportID: contextMenuContext?.currentReportID} : useCurrentReportID();
alexpensify commented 2 months ago

I was OOO on Friday and will add this one to my testing list.

bernhardoj commented 2 months ago

Proposal

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

In money request, mention is rendered in description text input but not shown in the field preview.

What is the root cause of that problem?

In description page, the text input enables the live markdown, so it will always render the mention report if the pattern is #{roomName}. But for the description field in confirmation page, the mention is rendered (by MenuItem > RenderHTML) but the style isn't applied because isGroupPolicyReport is false.

https://github.com/Expensify/App/blob/c5feb89f9f02cfb303e86246408f40c4f5a23119/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx#L86-L88

isGroupPolicyReport is false because the currentReportID is empty. https://github.com/Expensify/App/blob/c5feb89f9f02cfb303e86246408f40c4f5a23119/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx#L60-L65

currentReportID contains the topmost reportID of report screen which isn't available when opening the money request from FAB in small screen or from settings page in large screen.

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

First, we need to fix the description input live markdown so it will only render report mention if the user is creating the money request on a policy report. To do that, we need to accept a new prop called excludedMarkdownStyle in BaseTextInput. Then, in IOURequestStepDescription, pass the mention report as the excluded style if the report is not a policy report.

const isReportInGroupPolicy = !!report?.policyID && report.policyID !== CONST.POLICY.ID_FAKE;
...
excludedMarkdownStyle={!isReportInGroupPolicy ? ['mentionReport'] : []}

Then, we need to do the similar thing for the confirmation description menu item. Accept a new props called excludedMarkdownStyle in MenuItem and pass it to the replace method. https://github.com/Expensify/App/blob/c010e8c73cb8335a2a94fd5de91f4eb05f2d8fb9/src/components/MenuItem.tsx#L456-L460

return Parser.replace(title, {shouldEscapeText, disabledRules: excludedMarkdownStyle});

And then pass the excluded style if the report is not a policy report. https://github.com/Expensify/App/blob/c010e8c73cb8335a2a94fd5de91f4eb05f2d8fb9/src/components/MoneyRequestConfirmationListFooter.tsx#L299-L305

excludedMarkdownStyle={!policy ? ['reportMentions'] : []}

Last, we can't rely on useCurrentReportID context to get the reportID, https://github.com/Expensify/App/blob/c010e8c73cb8335a2a94fd5de91f4eb05f2d8fb9/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx#L60

so we need to create a new context to pass the reportID where the component is currently being used.

  1. Create MentionReportContext

    const MentionReportContext = createContext({currentReportID: ''});
    export {MentionReportContext};
  2. Wrap the description component in confirmation page with the new context.

    <MentionReportContext.Provider value={{currentReportID: reportID}}>
    <MenuItemWithTopDescription
  3. In the mention report renderer component, receive the context value.

    
    const {currentReportID: currentReportIDContext} = useContext(MentionReportContext);
    const currentReportIDValue = currentReportIDContext ?? currentReportID?.currentReportID;

// replaces all currentReportID?.currentReportID usages with currentReportIDValue

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

alexpensify commented 2 months ago

@alitoshmatov, when you get a chance, can you please review whether one of these proposals will fix this issue? Thanks!

alitoshmatov commented 2 months ago

@daledah Thank you for your proposal, root cause in your proposal is correct, however I don't think you solution is correct it looks more like workaround just to keep currentReportID's value. Also there is some problems like calling useCurrentReportID hook conditionally which is not correct

alitoshmatov commented 2 months ago

@bernhardoj Thank you for your proposal, your RCA is correct. Your solution solves the issue and correctly fixes when to show report mention

We can go with @bernhardoj 's proposal

C+ reviewed ๐ŸŽ€ ๐Ÿ‘€ ๐ŸŽ€

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

๐Ÿ“ฃ @alitoshmatov ๐ŸŽ‰ 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 2 months ago

PR is ready

cc: @alitoshmatov

alexpensify commented 2 months ago

Awesome, thanks @bernhardoj!

melvin-bot[bot] commented 2 months ago

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

alexpensify commented 2 months ago

Next Steps

This PR is waiting for a review. @alitoshmatov, please reply here if you are unable to review the PR, and we can find another C+ for help. @RachCHopkins, I need your help to confirm that this PR is moving along. Thanks!

Heads up, I will be offline until Tuesday, September 3, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

alitoshmatov commented 2 months ago

Finish the review today

alexpensify commented 2 months ago

Thanks for the update!

melvin-bot[bot] commented 2 months ago

โš ๏ธ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

bernhardoj commented 2 months ago

The previous PR is reverted, I have opened up a new PR

cc: @alitoshmatov

alexpensify commented 2 months ago

Catching up from being OOO, I see the PR is moving along.

RachCHopkins commented 2 months ago

Will let you continue @alexpensify - I've really done nothing but watch it.

alexpensify commented 1 month ago

Whoops, my bad @RachCHopkins. Sorry, I forgot to unassign you.

alexpensify commented 1 month ago

@bernhardoj - can you please confirm this is the new PR you mentioned that you added for this issue?

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

I'm trying to identify the payment date since automation failed. Thanks!

bernhardoj commented 1 month ago

@alexpensify correct!

alexpensify commented 1 month ago

Thanks, I need to manually work on the payment summary since automation failed and the payment date has passed.

alexpensify commented 1 month ago

Payouts due: 2024-09-13

Upwork job is here. I kept the amounts the same since this was a unique one to catch and reverting was the only option. There was also swift action to fix the issue found.

bernhardoj commented 1 month ago

@alexpensify Thanks!

Requested in ND.

JmillsExpensify commented 1 month ago

$250 approved for @bernhardoj