[HOLD for payment 2023-08-01] [HOLD for payment 2023-07-28] Web - Workspace avatar is not updating after renaming

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open a new account
2. Click on FAB > new workspace > Rename the workspace
3. Do step 2 again and notice the announce room vs the admin room avatars

Expected Result:

Both announce room and admin room avatar should change at the same time to reflect the renamed workspace

Actual Result:

Only admin changes and announce room avatar doesn't update

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.43-0
Reproducible in staging?: y
Reproducible in production?: n
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

Recording.3762.mp4

2023-07-19.22.27.03.mp4

Expensify/Expensify Issue URL:
Issue reported by: GH @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689794502432149

View all open jobs on GitHub

[kbecciv]

For mWeb both admin and announce rooms avatars don't update

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[s-alves10]

Proposal

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

Admin and announce room avatar are not updated after renaming workspace

What is the root cause of that problem?

This issue is from this PR
We don't get option data when policy changes as you can see below(no dependency)

App/src/components/LHNOptionsList/OptionRowLHNData.js

Lines 58 to 66 in fa473e9

	const optionItem = useMemo(() => {
	// Note: ideally we'd have this as a dependent selector in onyx!
	const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale);
	if (deepEqual(item, optionItemRef.current)) {
	return optionItemRef.current;
	}
	optionItemRef.current = item;
	return item;
	}, [fullReport, preferredLocale, personalDetails]);

For admin report, the lastMessageText of the report is changed and so the useMemo is calculated again, but this is not the case for announce report.

This is the root cause

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

We need to add policy as dependency for policy reports.
The code would look like

    const policy = lodashGet(policies, [`${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`], '');

    const optionItemRef = useRef();
    const optionItem = useMemo(() => {
        // Note: ideally we'd have this as a dependent selector in onyx!
        const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale);
        if (deepEqual(item, optionItemRef.current)) {
            return optionItemRef.current;
        }
        optionItemRef.current = item;
        return item;
    }, [fullReport, preferredLocale, personalDetails, policy]);

Add the following keys to withOnyx

            policies: {
                key: ONYXKEYS.COLLECTION.POLICY
            },

This works perfect

What alternative solutions did you explore? (Optional)

[jasperhuangg]

It looks like a regression from #21406. I think we should have the PR's original author @hannojg fix it

[francoisl]

Sounds good, hopefully @hannojg can chime in, otherwise @s-alves10 's proposal above makes sense and works well for me.

[fedirjh]

I am looking at this.

[fedirjh]

I think @s-alves10 proposal makes sense to me as well. Let's just wait for @hannojg feedbacks. I think the solution will make unnecessary re-renders when any other prop changes, maybe we should limit the dependency to policy.name ?

[fedirjh]

I think another solution is adding an optimistic data to the announce report when the policy name is updated.

[jasperhuangg]

I think another solution is adding an optimistic data to the announce report when the policy name is updated.

I'm not sure if that would work for other clients that receive an Onyx update with the new policy name.

I think @s-alves10 proposal makes sense to me as well. Let's just wait for @hannojg feedbacks. I think the solution will make unnecessary re-renders when any other prop changes, maybe we should limit the dependency to policy.name ?

Sounds good with me! let's wait

[hannojg]

Hey gang, I am here to look into it now!

[hannojg]

Just adding to the proposal of @s-alves10, while this may work we explicitly want to express all the dependencies we need to calculate the option item data. By that I mean that we preferably want to forward the policy to the getOptionData function.

[s-alves10]

@hannojg
Yes, you're right. I'm doing that work now

[hannojg]

@s-alves10 there are a lot of places where this change needs to be made. Just to be sure that we catch all of them, I am also looking into it (you can do the final PR with your proposal if you want, right now I am just concerned to solve this properly).

You can find my (yet unfinished) draft PR here:

• [CP Staging] Fix regression #23189 #23240

I gotta leave for an hour and then will come back to it!
Appointment got canceled, continue looking into it

[mountiny]

Taking this one over from Francois as I have been also involved in the offending PR. Thanks for a quick fix.

@s-alves10 you will also get compensated for your help while Hanno implements it

[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

[mountiny]

@slafortune This was a regression from another PR so Hanno and @fedirjh dont need any payment but @s-alves10 helped with finding the solution so we should pay them $250 for their help

Additionalyt @Nathan-Mulugeta to the issue reporter

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.43-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Fix regression #23189 #23240

If no regressions arise, payment will be issued on 2023-07-28. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @hannojg does not require payment (Contractor)
• @fedirjh requires payment
• @s-alves10 requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] 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:
• [@fedirjh] 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:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] 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.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.44-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Fix regression #23189 #23240

If no regressions arise, payment will be issued on 2023-08-01. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @hannojg does not require payment (Contractor)
• @fedirjh requires payment
• @s-alves10 requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] 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:
• [@fedirjh] 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:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] 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.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@slafortune, @hannojg, @mountiny, @fedirjh, @s-alves10 Huh... This is 4 days overdue. Who can take care of this?

[fedirjh]

This is awaiting payment based on #23189 (comment)

[slafortune]

@s-alves10 and @Nathan-Mulugeta - offers sent!

[Nathan-Mulugeta]

Offer accepted

[slafortune]

All Paid