Load parent report action from withOnyx in ReportScreen #33970
Conversation
melvin-bot
bot
requested review from
roryabraham
and removed request for
a team
|
@roryabraham Please 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] |
| /** Array of report actions for this report */ | ||
| reportActions: PropTypes.arrayOf(PropTypes.shape(reportActionPropTypes)), | ||
| /** All the report actions for this report */ | ||
| reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)), |
There was a problem hiding this comment.
I noticed this was set to an array, but I checked the data in Onyx (it's an object) and the place that uses this data also has it typed in TS as a Record (which would be an object and not an array).
| { | ||
| // This is added down here so that it can access the full report object fetched from Onyx in the previous withOnyx. | ||
| parentReportActions: { | ||
| key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`, |
There was a problem hiding this comment.
Is there a case when parentReportID is empty string?
If not, that's fine but if possible, recommend report && report.parentReportID || 0}
There was a problem hiding this comment.
It shouldn't be possible, no. I checked all of the other places where the parent report actions are fetched using withOnyx and they all use the ternary. It's a little inconsistent whether the ternary returns 0 or the string '0', but since it's a template string that will always cast it all to a string anyway, I guess it doesn't really matter.
| @@ -239,7 +238,6 @@ function ReportScreen({ | |||
| policy={policy} | |||
| personalDetails={personalDetails} | |||
| isSingleTransactionView={isSingleTransactionView} | |||
| parentReportAction={parentReportAction} | |||
There was a problem hiding this comment.
Turns out this component no longer references a parentReportAction prop, so I just removed it. The parentReportAction is only used in the MoneyRequestHeader.
There was a problem hiding this comment.
Good catch! Just in case you were curious, I checked and this type of error would not be possible with TypeScript, so that's good 🙂
|
@aimane-chnaif bump for review/testing please. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
Maybe unrelated but noticed very buggy behavior on iOS ios.mov
|
|
Strange... I don't know what to say except... I can't reproduce any of that. Could you try debugging |
|
Also reproduced on main, not caused by this PR so out of scope |
|
@youssef-lr Please 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] |
|
🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #34071. |
There was a problem hiding this comment.
"Delete request" option doesn't show in 3 dots menu of header
@tgolen Upon testing against main vs this branch, this is clearly reproducible and caused by this PR. Confirmed not happening on main
In this video, former part is from this branch, latter part is from main
Screen.Recording.2024-01-06.at.2.08.04.AM.mov
There was a problem hiding this comment.
Sorry, I'm having trouble with my dev environment and it's making it difficult to validate this solution, but I made some tweaks that I think would improve this in two ways:
- No need to double-wrap this component in
withOnyxand make a deeper component tree - Don't re-render
ReportScreenwhen any reportAction on the parent report is added or updated, only the one we care about – the one withparentReportActionID
diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js
index 416a8b5f13..f9a1f1c746 100644
--- a/src/pages/home/ReportScreen.js
+++ b/src/pages/home/ReportScreen.js
@@ -69,8 +69,8 @@ const propTypes = {
/** All the report actions for this report */
reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
- /** All the report action belonging the report's parent */
- parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
+ /** The report's parentReportAction */
+ parentReportAction: reportActionPropTypes,
/** Whether the composer is full size */
isComposerFullSize: PropTypes.bool,
@@ -108,7 +108,7 @@ const propTypes = {
const defaultProps = {
isSidebarLoaded: false,
reportActions: {},
- parentReportActions: {},
+ parentReportAction: {},
report: {},
reportMetadata: {
isLoadingInitialReportActions: true,
@@ -148,7 +148,7 @@ function ReportScreen({
report,
reportMetadata,
reportActions,
- parentReportActions,
+ parentReportAction,
accountManagerReportID,
personalDetails,
markReadyForHydration,
@@ -193,7 +193,6 @@ function ReportScreen({
const isOptimisticDelete = lodashGet(report, 'statusNum') === CONST.REPORT.STATUS.CLOSED;
const shouldHideReport = !ReportUtils.canAccessReport(report, policies, betas);
const isLoading = !reportID || !isSidebarLoaded || _.isEmpty(personalDetails);
- const parentReportAction = parentReportActions[report.parentReportActionID];
const isSingleTransactionView = ReportUtils.isMoneyRequest(report);
const policy = policies[`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`] || {};
const isTopMostReportId = currentReportID === getReportID(route);
@@ -578,14 +577,16 @@ export default compose(
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`,
initialValue: false,
},
- },
- true,
- ),
- withOnyx(
- {
- // This is added down here so that it can access the full report object fetched from Onyx in the previous withOnyx.
- parentReportActions: {
+ parentReportAction: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
+ selector: (parentReportActions, props) => {
+ const parentReportActionID = lodashGet(props, 'report.parentReportActionID');
+ if (!parentReportActionID) {
+ return {};
+ }
+ return parentReportActions[parentReportActionID];
+ },
canEvict: false,
},
},I think this should work as expected and be better for performance. @tgolen what do you think?
|
+1 @roryabraham, your changes are working for me. I like that we'll only grab the report action we care about. |
|
@roryabraham Every time I've tried to get this to work with a selector, I've found that the |
|
@aimane-chnaif Thanks for further videos and I understand that you're able to reproduce it. I was hoping that since you could reproduce it, you could help debug it to figure out why it's not able to find the parent report action. It doesn't look like you did that debugging. I'll take another look at this today in hopes that I might be able to reproduce it and complete the debugging. |
|
@roryabraham @youssef-lr you're right, it did work! That's great that this is working with the selector. I'll push those changes up now. @aimane-chnaif I was still never able to reproduce the missing "Delete" option from the three-dot menu. Could you please try again with the latest change? |
| @@ -239,7 +238,6 @@ function ReportScreen({ | |||
| policy={policy} | |||
| personalDetails={personalDetails} | |||
| isSingleTransactionView={isSingleTransactionView} | |||
| parentReportAction={parentReportAction} | |||
There was a problem hiding this comment.
Good catch! Just in case you were curious, I checked and this type of error would not be possible with TypeScript, so that's good 🙂
|
holing off on merging to let @aimane-chnaif answer @tgolen's questions and re-test after the selector change. |
|
retesting |
|
@roryabraham I tried using the selector in another PR and it didn't work again. I think I may have stumbled upon the bug... The There could maybe be a solution where the selector has a third argument that includes the props that were originally passed to the component and were not loaded from Onyx in that instance of the HOC. |
|
That makes sense to me, good sleuthing. I'd be happy to review a PR for that feature in Onyx. Also, thinking more long-term, using passed-in props (or values from Onyx, for that matter) in a selector would be trivial if we had a hook-based Onyx API, because those values would already be in-scope: function MyComponent({reportID, parentReportActionID}) {
const report = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}reportID`);
const parentReportAction = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {
// note: at this point we could use any property from `report` or from passed-in props
selector: (reportActions) => reportActions[parentReportActionID]
});
...
...
} |
|
Actually @tgolen, maybe instead of using a 3rd param for the |
|
Unfortunately, the selectors are a bit deeper down here: https://github.com/Expensify/react-native-onyx/blob/34e293766dc7f7fa7e5bf23a2aab5c95c563610f/lib/Onyx.js#L113 and it would take quite a bit of work to get the props down at that level, possibly. I haven't really tried. |
|
Heads up @tgolen, there are conflicts now. |
|
Conflicts are solved. Looks like this is waiting on @aimane-chnaif performing some final tests and @youssef-lr or @roryabraham approving/merging. |
|
will give 🟢 in an hour |
|
@thienlnam Please 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] |
|
✋ 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 staging by https://github.com/roryabraham in version: 1.4.24-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.24-8 🚀
|
Details
This PR removes the deprecated method
ReportActionsUtils.getParentReportAction(report);and changes the ReportScreen component to properly load the parent report action using withOnyx.Fixed Issues
Part of #27262
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
Unable to test due to emulator not working locally
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop