Fix domain all rooms' subtitle #3845
Conversation
| if (policy && policy.id) { | ||
| policies[policy.id] = policy; | ||
| callback: (policy, key) => { | ||
| if (policy && key && policy.name) { |
There was a problem hiding this comment.
Just a note for why I needed to be specific about policy.name here being included because otherwise when this function is called from here:
https://github.com/Expensify/Expensify.cash/blob/8a221022b633404eb84960b23b64ca884dc51fc4/src/libs/Navigation/AppNavigator/AuthScreens.js#L143
we end up calling the Onyx callback here and overwriting the policies with just their employeesList. It'll overwrite all the policy objects in policies with just their employeeList. I believe a race condition is causing this. By checking for policy.name, here we won't overwrite and delete the policy names here.
To see this in effect, you can take out && policy.name and see that the search bar will always show Unknown Policy when you open it:
| return ''; | ||
| } | ||
| if (report.chatType === CONST.REPORT.CHAT_TYPE.DOMAIN_ALL) { | ||
| return report.reportName.substring(1); |
There was a problem hiding this comment.
Why do we need the substring(1) call? Isn't report.reportName sufficient?
There was a problem hiding this comment.
The reportName has the # in it so this just gets rid of that.
There was a problem hiding this comment.
That sounds good, but I think a comment refering that would be nice, just like us whom ever is reading this code might not know and get a bit confused on to the why. Also if at any point we don't have the # before it might make it easier to search for it.
There was a problem hiding this comment.
That's a good point, I'll leave one in!
|
@sketchydroide all yours! |
|
Updated! |
|
Ok happy to aprove once conflicts are taken care of |
…to amal-domain-chatroom-name
|
Lol I have caused my own merge conflicts 😅 . They are resolved now! |
|
@sketchydroide bump! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@TomatoToaster Caught this in the checklist before it's checked off 😄 Bumping as instructed |
|
Oh both of these got merged, yep this one is good too! |
@sketchydroide, please review when you have the chance
CC: @yuwenmemon
Details
Shows domain name instead of
Unknown Policyfor rooms likeexpensify.comFixed Issues
$ https://github.com/Expensify/Expensify/issues/169049
Tests
Did the QA locally to verify this works.
QA Steps
#expensify.com) have subtitles on them that say their domain name instead ofUnknown Policy.Tested On
Screenshots
Web