[CP Stg] Move report fetch to ReportScreen to populate props.report
#10545
Conversation
There was a problem hiding this comment.
At some point I'd love to completely remove CURRENTLY_VIEWED_REPORTID from Onyx. I was digging into this with the sidebar links. As I see it, this was originally added to Onyx because:
- There is trouble passing the
routeto SideBarLinks due to it being in acontextDrawerwhich doesn't have direct access to theroute - The user action needs to know it
But, I don't think it should be in Onyx, since I don't think it needs persisted at all. It could just be an in-memory variable stored in the report action (and then we add a getCurrentlyViewingReportID() method.
I'd also love to find a way to pass the route directly down to the contextDrawer, but got stuck when I attempted it.
| return; | ||
| } | ||
|
|
||
| Report.fetchChatReportsByIDs([reportIDFromPath], true); |
There was a problem hiding this comment.
NAB: This is fine for now, but it occurred to me that this method has a bunch of side effect if all it's supposed to do is save the currently viewing report ID (three side-effects). It's probably something that could be cleaned up by renaming the method to something like... (I can't think of a name right now). Either that, or separating the logic into three different pieces to handle each side-effect.
There was a problem hiding this comment.
Either that, or separating the logic into three different pieces to handle each side-effect.
I like this idea. 👍
There was a problem hiding this comment.
We can create a GH for this, maybe
There was a problem hiding this comment.
Agree with this, but also agree with not blocking on it for now
Co-authored-by: Tim Golen <tgolen@gmail.com>
|
One thing I'm noticing is that we are not optimistically creating a new chat if it doesn't exist yet. Need to confirm if that's new or if it's happening on staging/production already |
|
Okay, verified that same problem exists on both staging and production |
|
|
…NotReportObject Move report fetch to `ReportScreen` to populate `props.report` (cherry picked from commit 168f56e)
…-10545 🍒 Cherry pick PR #10545 to staging 🍒
sketchydroide
changed the title
ReportScreen to populate props.reportReportScreen to populate props.report
sketchydroide
changed the title
ReportScreen to populate props.reportReportScreen to populate props.report
|
Just QA on desktop and Web, this is working |
|
Tested on Android (v1.1.89-3) and I'm still seeing the #10539 bug :( |
|
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.89-3 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
@Beamanator @roryabraham @marcaaron Part of the CP is not fixed #10539 Screen_Recording_20220825-084559_New_Expensify.mp4 |
|
@kbecciv I think that is the last blocker that is already open. |
|
+1 not related to these changes. |
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
Details
cc @tgolen
When we recently refactored the
ReportActionsViewwe accidentally broke fetching the report object when navigating to chats that might not have the report yet. This is an interesting case and seems like it can happen when:The report won't be able to load because
ReportActionsViewnow references theprops.report.reportID(which doesn't exist yet)Solution for now - move the logic that was fetching the report to the ReportScreen and always make sure a
reportis passed toReportActionsViewSolution to do later - Refactor the
fetchChatReportsByIDs()call to a new command called something likeSwitchReportthat will load the report if it doesn't exist locally yet.Fixed Issues
$ #10527 #10539
Tests / QA Steps
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
Mobile Web
Desktop
iOS
Android