-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix blank page on archived room with messages #13124
Fix blank page on archived room with messages #13124
Conversation
@dangrous @thesahindia 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] |
@robertjchen @dangrous, what's your opinion? Should we reuse Line 1045 in 5f89a98
|
Hm, interesting. Are we sure using |
Yes, I think. But we have some if statements at |
Let us know what you think is best @dangrous. |
Yeah I think I agree, @thesahindia - best to keep it simple. @hellohublot , if you wouldn't mind swapping back to the original solution - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If we're going to go with this solution we should do the tests in both priority modes
- Also, since we're modifying
shouldReportBeInOptionList
, should we add automated tests that cover the new cases? - I think the QA section should be updated since you can't modify the running code when doing QA on staging
- In the future please write good titles for PRs explaining what its doing and capitalize the first word since it's a title
I agree |
58607ee
to
f6ca014
Compare
@thesahindia Hi, I have submitted my original solution and tests in this PR. Thank you very much for your suggestions. |
Thanks! I will test it in a few minutes. |
src/pages/home/ReportScreen.js
Outdated
@@ -195,10 +195,12 @@ class ReportScreen extends React.Component { | |||
// We create policy rooms for all policies, however we don't show them unless | |||
// - It's a free plan workspace | |||
// - The report includes guides participants (@team.expensify.com) for 1:1 Assigned | |||
// - It has removed all guide participants before archiving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need a comment for this. It's making it more confusing
// - It has removed all guide participants before archiving |
Or maybe add this
// - It has removed all guide participants before archiving | |
// - It's an archived room |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it has been updated
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2022-11-30.at.3.53.37.PM.movMobile Web - ChromeScreen.Recording.2022-11-30.at.4.07.20.PM.movMobile Web - SafariScreen.Recording.2022-11-30.at.4.23.45.PM.movDesktopScreen.Recording.2022-11-30.at.4.39.54.PM.moviOSScreen.Recording.2022-11-30.at.4.28.54.PM.movAndroidScreen.Recording.2022-11-30.at.4.11.40.PM.mov |
@hellohublot, please remove the first step from QA steps. |
@thesahindia Sure, it has been removed |
@hellohublot, please also add these steps to offline steps -
|
@thesahindia Yes, it has been added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All yours @dangrous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@cead22 re-requested review from you to clear out the changes you requested (for the old version of code), so I can merge. |
Bump @cead22 |
Changes requested on old version of code
I went ahead and dismissed the requested changes (since no longer relevant) so we can merge this |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @luacmartins in version: 1.2.35-0 🚀
|
Details
Use
ReportUtils.isArchivedRoom
fix blank page on archived roomFixed Issues
$ #12045
PROPOSAL: #12045 (comment)
Tests
Permissions.canUseDefaultRooms
methodfunction canUseDefaultRooms(betas) { + return false return _.contains(betas, CONST.BETAS.DEFAULT_ROOMS) || canUseAllBetas(betas); }
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots/Videos
Web
_web.mov
Mobile Web - Chrome
_mobile_chrome.mp4
Mobile Web - Safari
_mobile_safari.mp4
Desktop
_desktop.mov
iOS
_ios.mp4
Android
_android.mp4