fix workspace avatar it not updated #19750

hungvu193 · 2023-05-29T17:18:44Z

Details

fix workspace avatar and workspace name are not updated

Fixed Issues

$ #19550
PROPOSAL: #19550 (comment)

Tests

Go to Settings > Workspaces
Create new workspace
Close the modal
Click on workspace chat in LHN
Go to Settings > Workspaces
Change the workspace picture
Verify that the workspace picture in report screen is updated
Change the workspace's name
Verify that the workspace's name in report screen is updated.

Verify that no errors appear in the JS console

Offline tests

Same as Tests.

QA Steps

Go to Settings > Workspaces
Create new workspace
Close the modal
Click on workspace chat in LHN
Go to Settings > Workspaces
Change the workspace picture
Verify that the workspace picture in report screen is updated
Change the workspace's name
Verify that the workspace's name in report screen is updated.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots

Web

Screen.Recording.2023-05-30.at.00.20.21.mov

Mobile Web - Chrome

Screen.Recording.2023-05-30.at.00.31.09.mov

Mobile Web - Safari

Screen.Recording.2023-05-30.at.00.27.52.mov

Desktop

Screen.Recording.2023-05-30.at.00.26.02.mov

iOS

Screen.Recording.2023-05-30.at.00.23.00.mov

Android

Screen.Recording.2023-05-30.at.00.24.46.mov

melvin-bot · 2023-05-29T17:33:20Z

@iwiznia @abdulrahuman5196 One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

hungvu193 · 2023-05-29T17:34:37Z

@iwiznia @abdulrahuman5196 hey, I wanna discuss about this.
Actually when we changed the icon, we change the avatar inside policy, so I think it'd better to add the condition with the policy.

abdulrahuman5196 · 2023-05-29T18:00:50Z

@iwiznia @hungvu193
One other reason i was aligned with ReportUtils.getIcons is that it provides icons on various other types of reports like threads, rooms, types of rooms, etc not just the workspace icon. So even in future if some other icons comes with same issue we won't have to fix it again in the same way. But as you know it might include slight latency as we get the icons and check on equality.

So @iwiznia wanted to check since you had approved, is the policy.avatar more reliable? Since its basically the original proposal instead of passing icons we are using policy.avatar and introducing a prop just to fix this issue. That's why we went with the alternate proposal.
#19550 (comment)

@hungvu193 Let me know your thoughts as well.

iwiznia · 2023-05-29T18:05:26Z

Ah damn, sorry, yes, I agree ReportUtils.getIcons is better

abdulrahuman5196 · 2023-05-29T19:50:24Z

Thank you.

@hungvu193 could you kindly update the PR with the original approved proposal?

hungvu193 · 2023-05-30T03:39:51Z

Yeah, but actually if we want to compare icons inside ReportActionView we still need to use personalDetails, so we still need to pass another props to our ReportActionsView.
Also if we use comparing our icons props inside, it also won't make the component re-rendering because both of them depending on the allPolicies which was already updated before that. So that why I recommended to add a new props icons and pass it into ReportActionsView or we can check for the policy updated like in my PR. Let me know your thoughts on this @abdulrahuman5196 @iwiznia

const icons = ReportUtils.getIcons(this.props.report, this.props.personalDetails);
const nextIcons = ReportUtils.getIcons(nextProps.report, nextProps.personalDetails);

abdulrahuman5196 · 2023-05-30T08:16:22Z

@hungvu193

but actually if we want to compare icons inside ReportActionView we still need to use personalDetails, so we still need to pass another props to our ReportActionsView

you don't need to pass it as props from parent view. You can directly access it from onyx like in the ReportActionItem

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

Line 443 in d06d032

withPersonalDetails(),

hungvu193 · 2023-05-30T08:25:36Z

@hungvu193

but actually if we want to compare icons inside ReportActionView we still need to use personalDetails, so we still need to pass another props to our ReportActionsView

you don't need to pass it as props from parent view. You can directly access it from onyx like in the ReportActionItem

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

Line 443 in d06d032

withPersonalDetails(),

How about my second concern? I realized if we calculated icons inside shouldcomponentUpdate it will always had same values between nextIcons and icons because, allPolicies had been updated before we get the information from it.

abdulrahuman5196 · 2023-05-30T10:05:58Z

@hungvu193 .

Ok got it. So the main concern is if the policy information gets changed we won't reload since we don't get the information as its not a prop of ReportActionsView.

So anyways regardless of ReportUtils.getIcons usage we would need policy as a prop. So for the current usecase policy.avatar is fixing and checking ReportUtils.getIcons would be redudent check.

I am aligned with the current approach based on the above. So I think if in future anyone wants more function they can use always use ReportUtils.getIcons and it would be working fine since we already have policy as props.

abdulrahuman5196 · 2023-05-30T10:06:12Z

Will start testing this out then.

abdulrahuman5196 · 2023-05-30T14:41:28Z

src/pages/home/ReportScreen.js

@@ -340,6 +342,7 @@ class ReportScreen extends React.Component {
                                    isComposerFullSize={this.props.isComposerFullSize}
                                    isDrawerOpen={this.props.isDrawerOpen}
                                    parentViewHeight={this.state.skeletonViewContainerHeight}
+                                    policy={policy}


Can we pass the policies directly and have the filter in ReportActionsView similar to how MoneyRequestHeader is doing?

App/src/pages/home/ReportScreen.js

Line 297 in 619a529

policies={this.props.policies}

The reason I saying is, for normal chats and group chats the policy would be undefined and we would be passing a undefined value, which won't be a good code practice. With the proposed way it will be in sync with existing code as well.

Sure @abdulrahuman5196

Updated @abdulrahuman5196 !

abdulrahuman5196 · 2023-05-30T15:57:05Z

src/pages/home/report/ReportActionsView.js

+        const policy = this.props.policies[`${ONYXKEYS.COLLECTION.POLICY}${this.props.report.policyID}`];
+        const nextPolicy = nextProps.policies[`${ONYXKEYS.COLLECTION.POLICY}${nextProps.report.policyID}`];
+
+        if (!_.isEqual(policy, nextPolicy)) {


Shouldn't avatar equality be enough? Policy will have other values for which we might not want to reload.

yeah, you're right, we can avoid unnecessary comparing. @abdulrahuman5196

I updated and tested it as well.

abdulrahuman5196 · 2023-05-30T18:03:55Z

src/pages/home/report/ReportActionsView.js

@@ -120,6 +127,13 @@ class ReportActionsView extends React.Component {
    }

    shouldComponentUpdate(nextProps, nextState) {
+        const policy = this.props.policies[`${ONYXKEYS.COLLECTION.POLICY}${this.props.report.policyID}`];


@hungvu193 One change. This should be checked only before the icons equality check.
https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsView.js#L171

I don't think this is of high priority to be checked for equality first. Can we move this check just before the icons equality check?

Nice catch. Thank you @abdulrahuman5196 . I've updated the PR!

I don't think there is use in fetching the policy before and checking its equality on lower part. This policy is not used anywhere else.
can we move the policy and nextPolicy lines before the equality check?

// other condition const policy = this.props.policies[`${ONYXKEYS.COLLECTION.POLICY}${this.props.report.policyID}`]; const nextPolicy = nextProps.policies[`${ONYXKEYS.COLLECTION.POLICY}${nextProps.report.policyID}`]; if (lodashGet(policy, 'avatar') !== lodashGet(nextPolicy, 'avatar')) { return true; }

Somethings like this right? So we can avoid to calculate policy every time.
@abdulrahuman5196

updated! @abdulrahuman5196

src/pages/home/report/ReportActionsView.js

abdulrahuman5196 · 2023-06-01T16:01:56Z

@iwiznia
Could you kindly let us know on your thoughts on this as well? This is blocking PR now

#19750 (comment)
#19750 (comment)
#19750 (comment)

iwiznia · 2023-06-01T16:58:10Z

Could you kindly let us know on your thoughts on this as well? This is blocking PR now

🤔 so the question is if we fix the name problems here too or just the avatar?

abdulrahuman5196 · 2023-06-01T17:31:19Z

so the question is if we fix the name problems here too or just the avatar?

Yes. @iwiznia

iwiznia · 2023-06-01T17:42:01Z

Ah, ok. If we are touching the same places and makes sense to group the changes, then lets?

abdulrahuman5196 · 2023-06-01T17:46:57Z

Ok then @hungvu193 could you fix this? #19750 (comment)

hungvu193 · 2023-06-02T05:29:51Z

Ok then @hungvu193 could you fix this? #19750 (comment)

@abdulrahuman5196 this is actually another issue. Problem came from here:

App/src/components/ReportActionItem/ReportPreview.js

Lines 118 to 121 in d8b1d84

    
           {props.iouReport.hasOutstandingIOU ? ( 
        
               <Text style={[styles.chatItemMessage, styles.cursorPointer, styles.colorMuted]}> 
        
                   {lodashGet(message, 'html', props.translate('iou.payerOwesAmount', {payer: managerName, amount: reportAmount}))} 
        
               </Text>

The payer name is already updated, but we are displaying it only when we couldn't get the html from message, that's why even you reloaded the page, you still saw that the workspace name is not updated.
I'm happy to open another PR to fix this issue.
cc @iwiznia

abdulrahuman5196 · 2023-06-02T15:00:10Z

@hungvu193 The issue #19847 which we added here required the fix for both the workspace name in created message header and IOU message.

In a new workspace chat, the changed workspace name should be updated on the welcome workspace chat message as well as on the request money report dynamically

I wouldn't say it as different issue. I don't think we should only solve half the other issue we agreed to fix it as part of this pr

abdulrahuman5196 · 2023-06-02T16:49:49Z

@iwiznia I think we might be bugging you. 😶‍🌫️ . But wanted to close the loop and avoid future churns.

What do you think of this #19750 (comment)?
The other issue we included in this PR to solve has 2 sub issues.

Workspace name not updated in the creation header message
Workspace name not updated in IOU report

In a new workspace chat, the changed workspace name should be updated on the welcome workspace chat message as well as on the request money report dynamically

The 2nd sub-issue isn't in the code place where our current PR originally focused. - #19750 (comment). It could even need a separate proposal review. That was the original reason i thought to not merge the issues since it would require more changes at different place - #19750 (comment)

So the options here is

original PR + full fix of the name issue(with sub-issue 1 and 2)
original PR + name sub issue 1(change path same as our PR) and have a separate issue and proposal to solve name sub issue 2
original PR only and have a separate issue and proposal for name issue.

My alignment was either to fix the name issue totally or totally not(1 and 3). But I am also fine with option 2, to solve as much as possible here and have a separate review on IOU workspace name not updated - #19847 (I think @hungvu193's alignment is on this option #19750 (comment)).

Kindly let us know of the path forward.

iwiznia · 2023-06-02T17:03:10Z

2 sounds best to me. We have a proposal here to solve this issue and that other subissue and they are fixed in the same place.
Then, let's update the other issue saying that one part of it has already been solved and that proposal should be for that part only.

Sounds good?

abdulrahuman5196 · 2023-06-02T18:01:09Z

Sure sounds good. Will start reviewing on this basis

hungvu193 · 2023-06-03T01:57:21Z

Sounds good to me too. I'll update my proposal at related issue.

abdulrahuman5196 · 2023-06-04T13:49:16Z

@hungvu193 Could you kindly include the tests to reflect the workspace name change as well?

And for offline test,

Shouldn't it be same as Tests? Not Bug is related to offline mode itself. No extra steps needed. The bug is both offline and online right?

abdulrahuman5196 · 2023-06-04T13:54:53Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Untitled.49.mp4

Mobile Web - Chrome

Untitled.47.mp4

Mobile Web - Safari

Untitled.51.mp4

Desktop

Untitled.48.mp4

iOS

Untitled.50.mp4

Android

Untitled.mp4

hungvu193 · 2023-06-04T14:04:02Z

Thanks for pointing that out @abdulrahuman5196 . I've just updated the steps.

abdulrahuman5196 · 2023-06-04T15:31:25Z

src/pages/home/report/ReportActionsView.js

@@ -41,6 +42,7 @@ const propTypes = {
    ...windowDimensionsPropTypes,
    ...withDrawerPropTypes,
    ...withLocalizePropTypes,
+    ...policyPropTypes,


I don't think we can use policyPropTypes because it has policyMemberList which we don't pass now.

Can we use the way you previously had? @hungvu193

That's ok. I've just updated it @abdulrahuman5196

abdulrahuman5196

Changes looks good and works well. Reviewers Checklist is also complete.

@iwiznia All yours.

OSBotify · 2023-06-05T17:19:44Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-06-05T19:24:35Z

🚀 Deployed to staging by https://github.com/iwiznia in version: 1.3.24-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-06-07T03:28:25Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-06-07T13:20:04Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	success ✅

hungvu193 force-pushed the fix-19550 branch from 4ecfee4 to 3e1824b Compare May 29, 2023 17:19

hungvu193 marked this pull request as ready for review May 29, 2023 17:33

hungvu193 requested a review from a team as a code owner May 29, 2023 17:33

melvin-bot bot requested review from abdulrahuman5196 and removed request for a team May 29, 2023 17:33

melvin-bot bot requested a review from iwiznia May 29, 2023 17:33

hungvu193 mentioned this pull request May 29, 2023

[HOLD for payment 2023-06-13] [$1000] workspace picture not update in report screen #19550

Closed

6 tasks

fix workspace avatar it not updated

c6f3b20

hungvu193 force-pushed the fix-19550 branch from 3e1824b to c6f3b20 Compare May 29, 2023 17:35

iwiznia previously approved these changes May 29, 2023

View reviewed changes

abdulrahuman5196 reviewed May 30, 2023

View reviewed changes

update the policies props

e12175c

hungvu193 dismissed iwiznia’s stale review via e12175c May 30, 2023 15:49

abdulrahuman5196 reviewed May 30, 2023

View reviewed changes

only compare avatar for update policy

31d54d3

abdulrahuman5196 reviewed May 30, 2023

View reviewed changes

hungvu193 added 2 commits May 31, 2023 07:31

change the priority of equality check

cf911e2

update the position of avatar check

fa62c00

hungvu193 force-pushed the fix-19550 branch from 129cb5a to fa62c00 Compare May 31, 2023 06:27

hungvu193 mentioned this pull request May 31, 2023

[$1000] Request Money: The name doesn’t update dynamically on a new workspace chat #19847

Closed

6 tasks

iwiznia requested changes Jun 1, 2023

View reviewed changes

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

strepanier03 mentioned this pull request Jun 1, 2023

[HOLD for payment 2023-07-14] [$1000] The workspace logo disappears if changed the workspace picture on combined user/workspace chat #19905

Closed

6 tasks

hungvu193 added 3 commits June 2, 2023 12:13

update policy props

391120e

remove unused policy

3cacb86

fix lint complaining

68fba32

abdulrahuman5196 reviewed Jun 4, 2023

View reviewed changes

update policy proptypes

b68a6d9

abdulrahuman5196 reviewed Jun 4, 2023

View reviewed changes

abdulrahuman5196 approved these changes Jun 4, 2023

View reviewed changes

iwiznia approved these changes Jun 5, 2023

View reviewed changes

iwiznia merged commit 71017e4 into Expensify:main Jun 5, 2023

OSBotify mentioned this pull request Jun 5, 2023

Deploy Checklist: New Expensify 2023-06-05 #20195

Closed

51 tasks

fix workspace avatar it not updated #19750

fix workspace avatar it not updated #19750

Conversation

hungvu193 commented May 29, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot bot commented May 29, 2023

hungvu193 commented May 29, 2023

abdulrahuman5196 commented May 29, 2023

iwiznia commented May 29, 2023

abdulrahuman5196 commented May 29, 2023

hungvu193 commented May 30, 2023 • edited Loading

abdulrahuman5196 commented May 30, 2023

hungvu193 commented May 30, 2023

abdulrahuman5196 commented May 30, 2023

abdulrahuman5196 commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungvu193 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulrahuman5196 commented Jun 1, 2023

iwiznia commented Jun 1, 2023

abdulrahuman5196 commented Jun 1, 2023

iwiznia commented Jun 1, 2023

abdulrahuman5196 commented Jun 1, 2023 • edited Loading

hungvu193 commented Jun 2, 2023 • edited Loading

abdulrahuman5196 commented Jun 2, 2023 • edited Loading

abdulrahuman5196 commented Jun 2, 2023

iwiznia commented Jun 2, 2023

abdulrahuman5196 commented Jun 2, 2023

hungvu193 commented Jun 3, 2023

abdulrahuman5196 commented Jun 4, 2023 • edited Loading

abdulrahuman5196 commented Jun 4, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

hungvu193 commented Jun 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulrahuman5196 left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 5, 2023

OSBotify commented Jun 5, 2023

OSBotify commented Jun 7, 2023

OSBotify commented Jun 7, 2023

hungvu193 commented May 29, 2023 •

edited

Loading

hungvu193 commented May 30, 2023 •

edited

Loading

hungvu193 May 31, 2023 •

edited

Loading

abdulrahuman5196 commented Jun 1, 2023 •

edited

Loading

hungvu193 commented Jun 2, 2023 •

edited

Loading

abdulrahuman5196 commented Jun 2, 2023 •

edited

Loading

abdulrahuman5196 commented Jun 4, 2023 •

edited

Loading

abdulrahuman5196 commented Jun 4, 2023 •

edited

Loading