[$250] When we set description of a room for the first time, optimistically it is shown as a comment in the LHN and later updates to system message

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.11-0
Reproducible in staging?: Y
Reproducible in production?: Y
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
Expensify/Expensify Issue URL:
Issue reported by: @allgandalf
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721729123500449

Action Performed:

1. Create a room
2. Click on room heading
3. Go to description and set description
4. Observe in LHN that the exact description entered is shown first then it updates to system message

Expected Result:

Description entered is displayed

Actual Result:

Description entered is shown first then it updates to system message

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

Screen.Recording.2024-07-23.at.3.35.05.PM.mov

Add any screenshot/video evidence

View all open jobs on GitHub

Recording.381.mp4

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d1a09eb334a44e74
• Upwork Job ID: 1816626243288402483
• Last Price Increase: 2024-08-08
• Automatic offers:
• allgandalf | Contributor | 103318023
• FitseTLT | Contributor | 103373101

Issue Owner

Current Issue Owner: @

[melvin-bot]

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

[FitseTLT]

Proposal

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

When we set description of a room for the first time, optimistically it is shown as a comment in the LHN and later updates to system message

What is the root cause of that problem?

Because we do not create an optimistic action (room description updated type of action) in updateDescription the LHN will default to show the description as there will no be any report action until the room update report action is returned from the server.

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

We need to create an optimistic room description updated report action in updateDescription.
We should do similar thing as we did for creating room name updated optimistic report action in here

App/src/libs/actions/Report.ts

Lines 2239 to 2262 in a3df238

	const optimisticRenamedAction = ReportUtils.buildOptimisticRenamedRoomReportAction(policyRoomName, previousName ?? '');
	const optimisticData: OnyxUpdate[] = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
	value: {
	reportName: policyRoomName,
	pendingFields: {
	reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
	},
	errorFields: {
	reportName: null,
	},
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
	value: {
	[optimisticRenamedAction.reportActionID]: optimisticRenamedAction,
	},
	},
	];

We create a similar util function as buildOptimisticRenamedRoomReportAction which takes new description as a param and creates a UPDATEROOMDESCRIPTION report action with the same content as would be returned from the server. And we then update the optimistic, success and failure data accordingly to include the optimistically created report action.

---

Work expected from the BE : the UPDATE_ROOM_DESCRIPTION api should start accepting reportActionID so that it will use the optimistically created reportActionID. Then we will pass the report Action ID as a parameter.

What alternative solutions did you explore? (Optional)

[allgandalf]

Note

I can take this as a C+ as i reported this issue and have more context

[VictoriaExpensify]

Recreated the issue. Agree this is buggy and we should fix it

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d1a09eb334a44e74

[melvin-bot]

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

[allgandalf]

@allroundexperts , mind if i take this over? I have a little extra context as i reported this one :)

[cretadn22]

Proposal

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

When we set description of a room for the first time, optimistically it is shown as a comment in the LHN and later updates to system message

What is the root cause of that problem?

We don't initiate a new optimistic report action when updating the description. Therefore, we need to wait for a response from the backend to show the revised description message

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

We need to implement a function to create a new optimistic report action.

function buildOptimisticChangedDescription(newValue: string): OptimisticEditedTaskReportAction {
    return {
        reportActionID: NumberUtils.rand64(),
        actionName: CONST.REPORT.ACTIONS.TYPE.ROOM_CHANGE_LOG.UPDATE_ROOM_DESCRIPTION,
        pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
        actorAccountID: currentUserAccountID,
        message: [
            {
                type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
                text: `set the room description to: ${newValue}`,
                html: `<muted-text>set the room description to: ${newValue}</muted-text>`,
            },
        ],
        originalMessage: {
            description: newValue,          // IMPORTANT
            lastModified: DateUtils.getDBTime(),
        },
        person: [
            {
                type: CONST.REPORT.MESSAGE.TYPE.TEXT,
                style: 'strong',
                text: newValue,
            },
        ],
        automatic: false,
        avatar: getCurrentUserAvatar(),
        created: DateUtils.getDBTime(),
        shouldShow: false,
    };
}

Next, in the updateDescription function, add this object to optimisticData.

const optimisticRenamedAction = ReportUtils.buildOptimisticChangedDescription(parsedDescription);
........
........
{
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
            value: {
                [optimisticRenamedAction.reportActionID]: optimisticRenamedAction,
            },
},

Additionally, you'll need to update successData and failureData accordingly

Optional: To be safe, we should add these below field to optimistic report

lastActorAccountID: currentUserAccountID,
lastMessageText: `set the room description to: ${parsedDescription}`,
text: `set the room description to: ${parsedDescription}`,
lastVisibleActionCreated: DateUtils.getDBTime(),
        

We also need to include reportActionID as a parameter and ensure that we get the update from the backend.

---

When removing the description, the message display "set the room description to: " but the subtitle on LHN display "cleared the room description"

Since there is no logic in place to determine whether the message is for updating or removing the description

App/src/libs/ReportActionsUtils.ts

Line 1294 in 5e65276

const html = isActionOfType(action, CONST.REPORT.ACTIONS.TYPE.SELECTED_FOR_RANDOM_AUDIT) ? Parser.replace(oldDotMessage) : oldDotMessage;

We should apply the same logic used in the Left Hand Navigation (LHN) for consistency.

const message = getUpdateRoomDescriptionMessage(action)

[allroundexperts]

@allroundexperts , mind if i take this over? I have a little extra context as i reported this one :)

Sure, all yours 😄

[allgandalf]

Thank you so much @allroundexperts, @VictoriaExpensify , can you assign me over here please, thanks :)

[melvin-bot]

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

[VictoriaExpensify]

Over to you @allgandalf

[allgandalf]

Thanks for your proposals @FitseTLT @cretadn22 , A little feedback to both of you:

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

This section is to ensure that you understand what the actual problem is, i would expect that you write this in a way that explains the problem and not copy paste the issue title. This section is to make sure that you understood what the problem is, so please try to write in your own understanding of the bug.

Now coming to the proposals, Both of you have the same and correct RCA!

I am inclined towards @FitseTLT's proposal as they were first to identify the RCA and also provide solution to it.

But before proceeding, @cretadn22 , From my understanding, I feel your solution is just an elaborated version of @FitseTLT's solution. Is that assumption correct or do you have anything different in your proposed solution ?

I will wait for your reply till next overdue label before getting an internal engineer on this, thanks for your proposals both of you!

[cretadn22]

@allgandalf This is some different things

1. @FitseTLT said that:

We should do similar thing as we did for creating room name updated optimistic report action in here

I believe it's incorrect because the updated room name message differs from the set description message. Let's see two below messages

So we can't apply the same logic to both cases

2. @FitseTLT only say that

And we then update the optimistic, success and failure data accordingly to include the optimistically created report action.

They don't specify which field is crucial and effectively used to obtain accurate results. In my proposal, I highlighted that detail.

originalMessage: {
            description: newValue,          // IMPORTANT
            lastModified: DateUtils.getDBTime(),
        }, 

3. Additionally, I recommend updating report.lastMessageText rather than just modifying reportAction.

4. In the final section of my proposal, I also address an edge case that needs to be handled

[FitseTLT]

@allgandalf This is some different things

1. @FitseTLT said that:

We should do similar thing as we did for creating room name updated optimistic report action in here

I believe it's incorrect because the updated room name message differs from the set description message. Let's see two below messages

So we can't apply the same logic to both cases

2. @FitseTLT only say that

And we then update the optimistic, success and failure data accordingly to include the optimistically created report action.

They don't specify which field is crucial and effectively used to obtain accurate results. In my proposal, I highlighted that detail.

originalMessage: {
            description: newValue,          // IMPORTANT
            lastModified: DateUtils.getDBTime(),
        }, 

3. Additionally, I recommend updating report.lastMessageText rather than just modifying reportAction.
4. In the final section of my proposal, I also address an edge case that needs to be handled

@cretadn22 It's not a must for a contributor to provide code changes as long as the give every important steps to be done and in my proposal I have mentioned every major important detail and tasks we need to do, all that is left is the code detail that should be dealt with in the PR phase.
The cases where I gave a similar example in our already existing code base is to give a hint into what is expected in this issue the minor detail changes that apply here are clear and obvious.
cc @allgandalf

[FitseTLT]

@allgandalf For more context you can see a similar example of the same situation as we are facing here and the decision and logic the reviewer use which is the same as your review.

[cretadn22]

For more context you can see a similar #46219 (comment) of the same situation as we are facing here and the decision and logic the reviewer use which is the same as your #46046 (comment).

In this example, two solutions are different. And the reviewer say that your proposal is selected because

"the policy name does not support markdown so there is no need to update the render as a fragment with TextCommentFragment."

[marcochavezf]

@FitseTLT Sure, the changes are in review, I let you know when they are live.

[melvin-bot]

@marcochavezf @FitseTLT @VictoriaExpensify @allgandalf 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!

[allgandalf]

Chill melvin, we are waiting for BE changes

[marcochavezf]

@FitseTLT @allgandalf the BE changes are live, the parameter for the API command UpdateRoomDescription is reportActionID. Let me know if you find any issues

[allgandalf]

Thanks for the quick changes @marcochavezf , and not overdue sir melvin

[allgandalf]

the issue owner is @FitseTLT , (idk how that happened) @FitseTLT can you please comment not overdue on this issue to remove the label, thanks

[FitseTLT]

not overdue

[melvin-bot]

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

[allgandalf]

What is the ETA of the PR @FitseTLT ?

[allgandalf]

PR was raised, today, I will review it soon

[allgandalf]

Approved the PR, all yours @marcochavezf 🙇

[allgandalf]

PR is on staging ♻️

[allgandalf]

• The PR that introduced the bug has been identified. Link to the PR: Migrate welcome message to room description for report #34150

• 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: https://github.com/Expensify/App/pull/34150/files#r1732202908

• 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

• 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. Create a room
2. Click on room heading and go offline
3. Go to description and set description

Verify that 'the room description updated' system message is shown

Verify also that the same system message is shown in LHN

Do we agree 👍 or 👎

[allgandalf]

@VictoriaExpensify can you pay this one please 🙇

[melvin-bot]

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

[VictoriaExpensify]

Created a GH to add this to TestRail - https://github.com/Expensify/Expensify/issues/423524

[VictoriaExpensify]

Payment Summary:
Contributor: @FitseTLT paid $250 via Upwork
Contributor+: @allgandalf paid $250 via Upwork

Thanks for your work on this!