Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fallback on report #19127

Merged
merged 3 commits into from
May 17, 2023
Merged

Fallback on report #19127

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da5acb0
fallback on report
luacmartins May 17, 2023
942b4fc
fix conditional
luacmartins May 17, 2023
4fbb188
update onyx key
luacmartins May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const HeaderView = (props) => {
const isChatRoom = ReportUtils.isChatRoom(props.report);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(props.report);
const isTaskReport = ReportUtils.isTaskReport(props.report);
const reportHeaderData = (isTaskReport || !isThread) && !_.isEmpty(props.parentReport) ? props.parentReport : props.report;
const reportHeaderData = (isTaskReport || !isThread) && props.report.parentReportID ? props.parentReport : props.report;
const title = ReportUtils.getReportName(reportHeaderData);
const subtitle = ReportUtils.getChatRoomSubtitle(reportHeaderData, props.parentReport);
const isConcierge = participants.length === 1 && _.contains(participants, CONST.EMAIL.CONCIERGE);
Expand Down Expand Up @@ -239,7 +239,7 @@ export default compose(
canEvict: false,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`,
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID || report.reportID}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this change makes sense - the parentReport is never the report.reportID right? Like can we just return nothing here if it doesn't exist?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was wondering the same, we should not retrun a wrong report as parent. I think we should return 0 in this case and make sure we check for that -> its root does not have a parent

Copy link
Contributor Author

@luacmartins luacmartins May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, Onyx will return all the reports in the report collection if report.parentReportID is undefined because the key will be report_. I spoke to @tgolen about this and we agreed that falling back to the report was a good solution in this case. As I mentioned in the OP, we could probably consolidate these into a single Onyx key report so the component always gets the right data and doesn't need to have any logic to decide which report it should use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, Onyx will return all the reports in the report collection if report.parentReportID is undefined because the key will be report_.

Does that also happen with report_0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but lately I've started seeing a report_0 key in Onyx (which seems to be coming from PHP somewhere) so I assume that'd be returned instead.

Screenshot 2023-05-17 at 11 58 43 AM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that also happen with report_0?

I don't think it should, no. It's expected that if you subscribe to a key like report_ that it gives you the entire collection and that is normal behavior. The undefined reportID is a nasty gotcha that results in subscribing to the entire collection.

},
}),
)(HeaderView);
Loading