[HOLD for payment 2024-05-07] [$250] Android - Room-Tapping create room with specific description directs to concierge page

[lanitochka17]

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: 1.4.52
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4426548
Issue reported by: Applause - Internal Team

Action Performed:

1. Launch app
2. Tap fab-- start chat --- Room
3. Enter room name
4. Paste the below text in room description

we can close this to focus on more urgent issue this is just a minor viewing discrepancy

can close this to focus on more urgent issue this is just a minor viewing discrepancy

can close this to focus on more urgent issue this is just a minor viewing discrepancy

can close this to focus on more urgent issue this is just a minor viewing discrepancy

5. Tap create room

Expected Result:

When user taps create room, he must be directed to room page and must not be redirected to concierge page

Actual Result:

When user taps create room with specific text entered in room description. , he is directed to room page but within seconds redirected to concierge page

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6413077_1710398840280.Usethis.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0171e588018217e1f4
• Upwork Job ID: 1772221395416739840
• Last Price Increase: 2024-04-01
• Automatic offers:
• shubham1206agra | Reviewer | 0
• rmm-fl | Contributor | 0

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

@greg-schroeder 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

[rmm-fl]

Proposal

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

create room with specific description directs to concierge page

What is the root cause of that problem?

Server returns description too long error

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

Because the server checks the final html generated length, instead of markdown, we should use the same final html generated string length for validation.
Validation step doesn't check for description length, we should add length validation here:

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Lines 163 to 181 in 0a512c9

	(values: FormOnyxValues<typeof ONYXKEYS.FORMS.NEW_ROOM_FORM>): OnyxCommon.Errors => {
	const errors: {policyID?: string; roomName?: string} = {};
	if (!values.roomName || values.roomName === CONST.POLICY.ROOM_PREFIX) {
	// We error if the user doesn't enter a room name or left blank
	ErrorUtils.addErrorMessage(errors, 'roomName', 'newRoomPage.pleaseEnterRoomName');
	} else if (values.roomName !== CONST.POLICY.ROOM_PREFIX && !ValidationUtils.isValidRoomName(values.roomName)) {
	// We error if the room name has invalid characters
	ErrorUtils.addErrorMessage(errors, 'roomName', 'newRoomPage.roomNameInvalidError');
	} else if (ValidationUtils.isReservedRoomName(values.roomName)) {
	// Certain names are reserved for default rooms and should not be used for policy rooms.
	ErrorUtils.addErrorMessage(errors, 'roomName', ['newRoomPage.roomNameReservedError', {reservedName: values.roomName}]);
	} else if (ValidationUtils.isExistingRoomName(values.roomName, reports, values.policyID ?? '')) {
	// Certain names are reserved for default rooms and should not be used for policy rooms.
	ErrorUtils.addErrorMessage(errors, 'roomName', 'newRoomPage.roomAlreadyExistsError');
	} else if (values.roomName.length > CONST.TITLE_CHARACTER_LIMIT) {
	ErrorUtils.addErrorMessage(errors, 'roomName', ['common.error.characterLimitExceedCounter', {length: values.roomName.length, limit: CONST.TITLE_CHARACTER_LIMIT}]);
	}

Solution

const descriptionLength = ReportUtils.getCommentLength(values.reportDescription);
if (descriptionLength > CONST.DESCRIPTION_LIMIT) {
    ErrorUtils.addErrorMessage(errors, 'reportDescription', ['common.error.characterLimitExceedCounter', {length: descriptionLength, limit: CONST.DESCRIPTION_LIMIT}]);
}

What alternative solutions did you explore? (Optional)

[melvin-bot]

@greg-schroeder Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0171e588018217e1f4

[melvin-bot]

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

[greg-schroeder]

Applying External

[allgandalf]

Proposal

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

Tapping create room with specific description directs to concierge page
(Note: This issue is not specific to android, it happens on all platforms)

What is the root cause of that problem?

We parse the description to html from markdown before making buildOptimisticChatReport and pass the html parsed description to the report:

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Lines 104 to 118 in 4eb0705

	const parsedDescription = ReportUtils.getParsedComment(values.reportDescription ?? '');
	const policyReport = ReportUtils.buildOptimisticChatReport(
	participants,
	values.roomName,
	CONST.REPORT.CHAT_TYPE.POLICY_ROOM,
	policyID,
	CONST.REPORT.OWNER_ACCOUNT_ID_FAKE,
	false,
	'',
	visibility,
	writeCapability || CONST.REPORT.WRITE_CAPABILITIES.ALL,
	CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS,
	'',
	'',
	parsedDescription,

Now the problem here is that when we convert to html, we get extra text of html tags which are counted by the backend and we get description too long error, but this error is technically wrong as the user is typing in markdown so we must allow this behavior.

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

1. So to allow this behavior first of all, we must send the description without parsing to the BE.
2. Then we need to update other places where we show this description, there we need to parse the report description before display.

Note

We all need to create one htmltotext parser util function in reportUtils to parse as normal text heading in ReportHeader

We need to update the description query at the following places:

App/src/pages/home/HeaderView.tsx

Line 101 in 4eb0705

const reportDescription = ReportUtils.getReportDescriptionText(report);

App/src/components/ReportWelcomeText.tsx

Line 114 in 4eb0705

<RenderHTML html={report.description} />

App/src/pages/ReportDetailsPage.tsx

Line 265 in 4eb0705

title={report.description}

Test Branch: https://github.com/GandalfGwaihir/App/tree/issue38293

Result Video

simplescreenrecorder-2024-03-27_02.41.58.mp4

[allgandalf]

Also, @greg-schroeder , If my proposal gets selected, can you please make the bounty to $500, this is a lot of extra work but it is to be future proof and allow the user to enter upto 500 character in markdown and not limited to 350-390 characters like we have right now.

[mkhutornyi]

@greg-schroeder please reassign C+. I am currently sick and not able to review this sooner.

[allgandalf]

gentle bump to @greg-schroeder for reassignment :)

[melvin-bot]

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

[melvin-bot]

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

[greg-schroeder]

Re-assigning C+ - @shubham1206agra do you mind taking a look at the proposals above? Thanks!

[shubham1206agra]

@greg-schroeder Can you ask QA to retest this?
I am unable to repro this at all.

[allgandalf]

@shubham1206agra , you can reproduce the issue by pasting the below content:

# we can close this to focus on more urgent issue this is just a minor viewing discrepancy

~we can close this to focus on more urgent issue this is just a minor viewing discrepancy~


we can close this to focus on more urgent issue this is just a minor viewing discrepancy


> *we can close this to focus on more urgent issue this is just a minor viewing discrepancy*

>  *we can close this to focus on more urgent issue this is just a minor viewing discrepancy*

Also now we get error on the room page rather than redirection to concierge chat, so this needs to be fixed at the creation level as mention in my proposal:

[shubham1206agra]

@GandalfGwaihir Your proposal is good but will have issues with backward compatibility, and security wise too.

So, I am going to go ahead with @rmm-fl's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[allgandalf]

@shubham1206agra @jasperhuangg , but if we limit that text we won't be able to utilize all 500 characters, for room description the limit is of '500' characters, and for the markdown mentioned in the OP, the total length is close '370' characters, this would not allow us to use the characters fully and will lead to another regression, my approach allows the user to enter upto 500 characters, this is the desired behavior i guess :) can you please re-review

[greg-schroeder]

Bump @jasperhuangg on confirming contributor selection above :)

[allgandalf]

bumping my comments again just to not miss the eye thanks, please refer to them before assignment @jasperhuangg

Also the expected results from the GH are to successfully redirect the user to the room and not to show error, so the selected proposal by the C+ will not satisfy the expected results at all:

Expected Result:

When user taps create room, he must be directed to room page and must not be redirected to concierge page

[shubham1206agra]

@jasperhuangg Can you do the assignment here?

[jasperhuangg]

Agree that @rmm-fl's proposal is simpler and less prone to bugs, let's move forward with their proposal.

[melvin-bot]

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

📣 @rmm-fl 🎉 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 📖

[rmm-fl]

The PR for fix was created here - #39624
@shubham1206agra Friendly ping :)

[greg-schroeder]

PR is merged. Awaiting deploy to staging -> prod

[rmm-fl]

@greg-schroeder @jasperhuangg The PR has been deployed to Prod. Seems like the payment automation didn't trigger.

[rmm-fl]

@greg-schroeder @jasperhuangg friendly bump for payment :)

[rmm-fl]

bumping @mountiny, because seems like the deploy was handled by them: #39624 (comment)

[greg-schroeder]

I will process this today

[greg-schroeder]

Payments made.