HOLD for payment 2023-11-17 - [$1000] Web - Incorrect comment formatting after approval

[kbecciv]

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:

1. Log in to Expensify.com as user A
2. Create a policy (Settings > Policies > New Policy > Collect or Control)
3. In the policy settings go to People (Settings > Policies > "Policy name" > People tab)
4. Invite another user B
5. In the People tab of the policy ensure the admin is set as the "Submit reports to:" and the Approval Mode is "Submit & Close"
6. Create and submit a report as user B
7. Approve user B's report as user A
8. Log in to new.expensify.com as user A
9. View the report's room (You can use the report ID in the new.expensify.com ULR if needed e.g. new.expensify.com/r/1234568910)
10. Check Approval comment

Expected Result:

Correct comment formatting after approval

Actual Result:

Incorrect comment formatting after approval

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.70.5
Reproducible in staging?: n/a
Reproducible in production?: n/a
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

Expensify/Expensify Issue URL:
Issue reported by:@joaniew
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694460444917739

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01729ed746f5c9f54c
• Upwork Job ID: 1703882814739632128
• Last Price Increase: 2023-10-10
• Automatic offers:
• jjcoffee | Reviewer | 27332302
• Victor-Nyagudi | Contributor | 27332305

[melvin-bot]

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

[melvin-bot]

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

[MitchExpensify]

Haven't been able to reproduce yet https://expensify.slack.com/archives/C049HHMV9SM/p1695071461773509?thread_ts=1694460444.917739&cid=C049HHMV9SM

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01729ed746f5c9f54c

[melvin-bot]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[MitchExpensify]

Updated the "Action Performed" to make this easier.. hopefully!

[zukilover]

Proposal

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

Incorrect comment formatting after approval

What is the root cause of that problem?

Text fragment is applying message header for the approval comment:

App/src/styles/styles.js

Lines 1640 to 1642 in 2ad2d29

	fontSize: variables.fontSizeNormal,
	fontWeight: fontWeightBold,
	lineHeight: variables.lineHeightXLarge,

this also happens because the TEXT fragment is treated as user details tooltip:

App/src/pages/home/report/ReportActionItemFragment.js

Lines 169 to 182 in 6d67552

	case 'TEXT':
	return (
	<UserDetailsTooltip
	accountID={props.accountID}
	delegateAccountID={props.delegateAccountID}
	icon={props.actorIcon}
	>
	<Text
	numberOfLines={props.isSingleLine ? 1 : undefined}
	style={[styles.chatItemMessageHeaderSender, props.isSingleLine ? styles.pre : styles.preWrap]}
	>
	{props.fragment.text}
	</Text>
	</UserDetailsTooltip>

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

In this case, there are 2 text fragments that contain accountID props and are treated as separate elements, when they should be composed as a single line so that it reads nicely, e.g. : You final approved this report.

There are two things to update here:

1. Make sure the chat wrapper displays both fragments in a single line by using flex-direction: row. We can develop a StyleUtils method that confirms whether both fragments should be inline. With the limited information on the message fragments, at this point we can only confirm by checking if the first part of the segments is meant to be emphasized (strong), e.g.:

function getReportCommentStyle(messages: any[]): ViewStyle | CSSProperties {
    if (messages.length > 1 && messages?.[0]?.style === 'strong') {
        return {
            ...styles.flexRow,
        }
    }
    return {}
}

2. If the text fragment contains accountID prop and is not to be emphasized (style = normal), treat it as regular text. Otherwise, this must be a user tooltip fragment:

case 'TEXT':
            if (props.fragment.style === 'normal') {
                return (
                    <Text
                        numberOfLines={props.isSingleLine ? 1 : undefined}
                        style={[props.isSingleLine ? styles.pre : styles.preWrap]}
                    >
                        {props.fragment.text}
                    </Text>
                )
            }
            return (
                <UserDetailsTooltip
                    accountID={props.accountID}
                    delegateAccountID={props.delegateAccountID}
                    icon={props.actorIcon}
                >
                    <Text
                        numberOfLines={props.isSingleLine ? 1 : undefined}
                        style={[styles.chatItemMessageHeaderSender, props.isSingleLine ? styles.pre : styles.preWrap]}
                    >
                        {props.fragment.text}
                    </Text>
                </UserDetailsTooltip>
            );

What alternative solutions did you explore? (Optional)

Approval comment fragment comes with a defined text style. We can simply use that instead to style the font-weight, e.g.:

                   <Text
                        numberOfLines={props.isSingleLine ? 1 : undefined}
                        style={[
                            styles.chatItemMessageHeaderSender, 
                            props.isSingleLine ? styles.pre : styles.preWrap,
                            {
                                fontWeight: props.fragment.style,
                            }
                        ]}
                    >
                        {props.fragment.text}
                    </Text>

[zukilover]

Proposal

Updated

[jjcoffee]

@zukilover Thanks for your proposal! I think your solution is too much of a workaround (we can't be sure what the TEXT fragment is based on its style) and I think the RCA is also not quite getting to the core of the issue.

Waiting on more proposals!

[melvin-bot]

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

[MitchExpensify]

Still waiting on more proposals here

[melvin-bot]

@jjcoffee @MitchExpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

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

[MitchExpensify]

@zukilover do you have any ideas for a new proposal based on @jjcoffee 's feedback?

[melvin-bot]

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

[lakchote]

@Victor-Nyagudi proposal LGTM. I like how you approached to solve this in an efficient way. Plus, you've fixed the SUBMITTED message types.

[melvin-bot]

📣 @jjcoffee 🎉 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]

📣 @Victor-Nyagudi 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Victor-Nyagudi]

Working on the PR right now.

I aim to have it up and ready in roughly 2 to 4 hours.

Thank you for the assignment.

[Victor-Nyagudi]

Limiting using the APPROVED actionName is what I was looking for. Is there any reason we wouldn't want to also use that to decide whether or not to wrap ReportActionItemFragment in a Text component?

@jjcoffee With regard to your question on whether we could use the actionName to conditionally wrap the ReportActionItemFrament in a Text component, what do you think of this proposition?

We'll first create a variable that checks if the props.action.actionName is of type of APPROVED or SUBMITTED in ReportActionItemMessage right below the other variables.

import CONST from '../../../CONST'; // <- Imported since it's currently not there

// ...

const isApprovedOrSubmittedReportActionType = _.contains(
    [CONST.REPORT.ACTIONS.TYPE.APPROVED, CONST.REPORT.ACTIONS.TYPE.SUBMITTED], 
    props.action.actionName
);

Next, we'll create a helper function in ReportActionItemMessage that returns a ReportActionItemFragment.

This is done because ReportActionItemFragment will be rendered regardless of whether the actionName is of type SUBMITTED/APPROVED or not. Also, should changes ever need to be made, they're made in one place and apply on both sides of the if check.

This function will accept two parameters - fragment and index - that it will receive from the map() function in ReportActionItemMessage's return statement.

// previous variables including 'isApprovedOrSubmittedReportActionType'...

const getReportActionItemFragment = (fragment, index) => {
    return(
        <ReportActionItemFragment
            key={`actionFragment-${props.action.reportActionID}-${index}`}
            fragment={fragment}
            iouMessage={iouMessage}
            isThreadParentMessage={ReportActionsUtils.isThreadParentMessage(props.action, props.reportID)}
            attachmentInfo={props.action.attachmentInfo}
            pendingAction={props.action.pendingAction}
            source={lodashGet(props.action, 'originalMessage.source')}
            accountID={props.action.actorAccountID}
            style={props.style}
            displayAsGroup={props.displayAsGroup}
            actionName={props.action.actionName}
        />
    );
};

We'll also need to store the Text component currently rendered if !props.isHidden is false in a variable because that's also going to be shown regardless of the check for submitted/approved.

// previous variables...

const flaggedContentText = (
    <Text style={[styles.textLabelSupporting, styles.lh20]}>{props.translate('moderation.flaggedContent')}</Text>
);

This part will also be repeated, so it can be stored in a variable.

const content = !props.isHidden ? _.map(messages, (fragment, index) => getReportActionItemFragment(fragment, index)) : flaggedContentText;

Finally, we'll replace what's inside the View in ReportActionItemMessage's return statement with the following:

<View style={[styles.chatItemMessage, !props.displayAsGroup && isAttachment ? styles.mt2 : {}, ...props.style]}>
    {isApprovedOrSubmittedReportActionType ? (
        // Wrapping 'ReportActionItemFragment' inside '<Text>' so that text isn't broken up into separate lines when
        // there are multiple messages of type 'TEXT', as seen when a report is submitted/approved from a
        // policy on Old Dot and then viewed on New Dot.

       <Text>{content}</Text>
   ) : (
       <>{content}</>
   )}
</View>

Things look good so far after some light testing.

cc: @lakchote for a second opinion

[Victor-Nyagudi]

Made some good progress on the PR, but it's late for me, so I'll pick things back up first thing tomorrow morning.

I'm also still trying to figure out how to navigate to the Old Dot report on native devices and desktop (web works fine for mobile and Mac) given the provided method of doing so (see step 9 above) involves copying the URL from Old Dot and pasting it the New Dot URL.

I'm open to suggestions.

[jjcoffee]

If at all any of you, @jjcoffee or @lakchote, feel this change isn't needed, I can just revert it.

@Victor-Nyagudi I think you can leave this out/revert as that has been there for over 2 years (just got updated to prepend with props). Correct me if I'm wrong, but as I understand it this also won't affect anything in the test steps for this issue, so would be out of scope.

I'm also still trying to figure out how to navigate to the Old Dot report on native devices and desktop

If you haven't figured this out yet, I usually use npx uri-scheme open, e.g. npx uri-scheme open new-expensify://r/12345678 --android. For the desktop app, you can just use DevTools and window.location in the console to open the relevant URL.

[Victor-Nyagudi]

@jjcoffee I moved those deleted comments to the draft PR. Thought it would be better to continue the conversation there, but here works too if you prefer it.

I think you can leave this out/revert as that has been there for over 2 years (just got updated to prepend with props)

Got it. I'll revert.

If you haven't figured this out yet, I usually use npx uri-scheme open, e.g. npx uri-scheme open new-expensify://r/12345678 --android. For the desktop app, you can just use DevTools and window.location in the console to open the relevant URL.

I also figured out how to get to the report on mobile devices (see step 38 and native screenshots). There are a lot of steps because of the extra set up occurring on Old Dot.

This issue occurs on all platforms.

[Victor-Nyagudi]

I'm terribly sorry for the delay, gentlemen.

I grossly underestimated the time it would take to test the solution on different platforms and complete the PR author checklist.

The PR is, however, now ready for review.

Please take a look and let me know what you think.

cc: @jjcoffee @lakchote

[jjcoffee]

No worries at all, I really appreciate how thorough you've been! Will be reviewing today.

[Victor-Nyagudi]

I'm glad to hear that.

Thank you.

[jjcoffee]

Just a quick update here - the PR is still in review but we're nearing the finish line! Hopefully ready to merge by EOD.

[Victor-Nyagudi]

@OSBotify says the PR was deployed to production 3 days ago, but @MelvinBot hasn't said anything about this.

Could something have gone wrong somewhere with the automation?

cc: @jjcoffee

[jjcoffee]

Oh good catch, yeah there's been some issues with good 'ole Melv lately I think. @MitchExpensify Can you update the title and label to reflect a HOLD for payment 2023-11-17? Thanks!

[MitchExpensify]

Payment summary:

9+ days to merge after assignment penalty (50%)

• $1000 @Victor-Nyagudi C: Upwork
• $1000 @jjcoffee C+: Upwork

[Victor-Nyagudi]

@MitchExpensify With regard to the penalty, please take a look at this.

I was assigned this issue on October 23rd.

Exact time I got assigned the issue

There was a delay of 2 business days at some point because the C+ was swamped with worked.

I made the following commits on Sunday, October 29th.

October 29th commits

This wasn't a business day, so it doesn't count. However, I didn't get a response until the following Wednesday and only after I bumped the C+.

Exact time I bumped the C+

Exact time C+ responded after bump

I didn't want to be a nuisance, so I wasn't bumping the C+ every day for reviews, but there wasn't much I could to prevent the 2 business days lost waiting for a response.

I was very much aware we were approaching the 9-days penalty, so I doubled down my efforts, and thankfully, the C+ was able to prioritize the PR too.

After everything was ironed out, the PR was ready to be merged on November 3rd.

Exact time C+ approved PR changes

The amount of time in business days between October 23rd (6:45pm) when I was assigned and November 3rd (4:51pm) when the PR was ready to be merged is 8d 22h 6min.

November 3rd is a Friday, so the PR could only be merged the following week which happened.

Again, not enough time had elapsed to warrant bumping the internal engineer, so there wasn't much I could about the extra hours incurred between PR approval and the internal engineer merging the PR.

Exact time PR got merged

As you can see, @MitchExpensify, even without the 2-business-day delay, the PR was ready to merge in less than 9 business days.

There's nothing I could do about the amount of work the C+ was dealing with.

In addition, if you take a look at the comments in the PR, the C+ and I were very proactive in finding a solution without regression, so the longer duration wasn't as a result of poor effort on our part.

I beg you to take all this into account and please reconsider your stance on the 50% penalty.

[jjcoffee]

Hmm yeah I agree the penalty wouldn't usually be applied here as the PR was ready to merge within 9 WD, then was merged without changes on the next working day.

• Assigned 23/10 5:45 PM
• C+ approved 3/11 2:51 PM
• Merged by @lakchote without changes 6/11 11:43 AM

As @Victor-Nyagudi points out, this required a lot of work as we needed quite a lot of back and forth on the PR to handle a few edge cases so that's why we ended up going right up to the limit. Thanks for your consideration @MitchExpensify 🙇

[MitchExpensify]

Ok, that's fair! Updating the payment summary accordingly. I appreciate the timelines!

[Victor-Nyagudi]

Bump @MitchExpensify for payment.

This was supposed to be paid out last Friday.

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - these messages were never handled
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug. *Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. As user A: On oldDot, create a policy (Settings > Policies > New Policy > Collect or Control)
2. In the policy settings go to People (Settings > Policies > "Policy name" > People tab) and invite another user B
3. Ensure the admin is set as the "Submit reports to:" and the Approval Mode is "Submit & Approve"
4. Create and submit a report as user B to the newly created policy in step 1
5. Approve user B's report as user A
6. Log in to newDot as user A
7. Open the report using the pageReportID in the oldDot URL for the report
8. Verify that the approval comment displays all on one line, greyed out like other system messages and with a consistent font weight.

Do we agree 👍 or 👎

[jjcoffee]

@MitchExpensify Checklist complete!

[MitchExpensify]

Paid and contracts ended