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

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

Closed
1 of 6 tasks
IuliiaHerets opened this issue Aug 10, 2024 · 30 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 10, 2024

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:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6567516_1723273458070.Screenrecorder-2024-08-10-12-21-55-642_compress_1.mp4

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
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 10, 2024
Copy link

melvin-bot bot commented Aug 10, 2024

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

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

@IuliiaHerets
Copy link
Author

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

daledah commented Aug 11, 2024

Proposal

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

  • mWeb - IOU - In description field, mentions are shown in preview but not shown after saved

What is the root cause of that problem?

so the mention style is not applied.

  • Still there is the case currentReportID is not -1, that is when we open confirmation page in desktop.

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

<ShowContextMenuContext.Provider value={contextValue}></ShowContextMenuContext.Provider>

with:

    const contextValue = useMemo(
        () => ({
            currentReportID: reportID,
        }),
        [reportID],
    );

and then in here:

const currentReportID = useCurrentReportID();

use:

    const contextMenuContext = useContext(ShowContextMenuContext);
    const currentReportID = contextMenuContext?.currentReportID ? {currentReportID: contextMenuContext?.currentReportID} : useCurrentReportID();
  • With these changes, we can get the correct currentReportID when we are in confirmation page.

What alternative solutions did you explore? (Optional)

  • As we can see in RCA, we need to fix the inconsistency that sometimes the mention style is applied, sometimes is not and the expectation is that the mention style should not be applied in confirmation page.

  • To do it, we can update:

    <ShowContextMenuContext.Consumer>
    {() => (
    <Text
    // eslint-disable-next-line react/jsx-props-no-spreading
    {...defaultRendererProps}
    style={
    isGroupPolicyReport
    ? [styles.link, styleWithoutColor, StyleUtils.getMentionStyle(isCurrentRoomMention), {color: StyleUtils.getMentionTextColor(isCurrentRoomMention)}]
    : []
    }
    suppressHighlighting
    onPress={
    navigationRoute && isGroupPolicyReport
    ? (event) => {
    event.preventDefault();
    Navigation.navigate(navigationRoute);
    }
    : undefined
    }
    role={isGroupPolicyReport ? CONST.ROLE.LINK : undefined}
    accessibilityLabel={isGroupPolicyReport ? `/${navigationRoute}` : undefined}
    >
    #{mentionDisplayText}
    </Text>
    )}
    </ShowContextMenuContext.Consumer>

    to:

    <ShowContextMenuContext.Consumer>
+          {({report}) => (
                <Text
                    style={
+                      report && isGroupPolicyReport
                            ? [styles.link, styleWithoutColor, StyleUtils.getMentionStyle(isCurrentRoomMention), {color: StyleUtils.getMentionTextColor(isCurrentRoomMention)}]
                            : []
                    }
                >
                    #{mentionDisplayText}
                </Text>
            )}
        </ShowContextMenuContext.Consumer>
  • Optional: We can handle the onPress function as well.

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@alexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
@bernhardoj
Copy link
Contributor

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.

isGroupPolicyReport
? [styles.link, styleWithoutColor, StyleUtils.getMentionStyle(isCurrentRoomMention), {color: StyleUtils.getMentionTextColor(isCurrentRoomMention)}]
: []

isGroupPolicyReport is false because the currentReportID is empty.

const currentReportID = useCurrentReportID();
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const [currentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${currentReportID?.currentReportID || -1}`);
// When we invite someone to a room they don't have the policy object, but we still want them to be able to see and click on report mentions, so we only check if the policyID in the report is from a workspace
const isGroupPolicyReport = useMemo(() => currentReport && !isEmptyObject(currentReport) && !!currentReport.policyID && currentReport.policyID !== CONST.POLICY.ID_FAKE, [currentReport]);

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.

const html = useMemo(() => {
if (!title || !shouldParseTitle) {
return '';
}
return Parser.replace(title, {shouldEscapeText});

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

And then pass the excluded style if the report is not a policy report.

{
item: (
<MenuItemWithTopDescription
key={translate('common.description')}
shouldShowRightIcon={!isReadOnly}
shouldParseTitle
title={iouComment}

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

Last, we can't rely on useCurrentReportID context to get the reportID,

const currentReportID = useCurrentReportID();

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};
  1. Wrap the description component in confirmation page with the new context.
<MentionReportContext.Provider value={{currentReportID: reportID}}>
    <MenuItemWithTopDescription
  1. 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 melvin-bot bot added the Overdue label Aug 15, 2024
@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 15, 2024
@melvin-bot melvin-bot bot changed the title mWeb - IOU - In description field, mentions are shown in preview but not shown after saved [$250] mWeb - IOU - In description field, mentions are shown in preview but not shown after saved Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 15, 2024
@alexpensify
Copy link
Contributor

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

@alitoshmatov
Copy link
Contributor

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

@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 🎀 👀 🎀

Copy link

melvin-bot bot commented Aug 18, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

📣 @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

@melvin-bot melvin-bot bot added the Weekly KSv2 label Aug 20, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @alitoshmatov

@alexpensify
Copy link
Contributor

Awesome, thanks @bernhardoj!

@alexpensify alexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 26, 2024
@alexpensify
Copy link
Contributor

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

Finish the review today

@alexpensify
Copy link
Contributor

Thanks for the update!

Copy link

melvin-bot bot commented Aug 27, 2024

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

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

cc: @alitoshmatov

@alexpensify
Copy link
Contributor

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

@RachCHopkins
Copy link
Contributor

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

@RachCHopkins RachCHopkins removed their assignment Sep 6, 2024
@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

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

#48140

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

@bernhardoj
Copy link
Contributor

@alexpensify correct!

@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

alexpensify commented Sep 16, 2024

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

bernhardoj commented Sep 16, 2024

@alexpensify Thanks!

Requested in ND.

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

8 participants