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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CP Stg] Move report fetch to ReportScreen to populate props.report #10545

Merged
merged 7 commits into from
Aug 25, 2022
Merged

[CP Stg] Move report fetch to ReportScreen to populate props.report #10545

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74a0748
report.reportID only exists until after the report has loaded
marcaaron Aug 24, 2022
0164f59
Clean up a bit so that ReportActionsView still only relies on the rep…
marcaaron Aug 24, 2022
50cb41a
improve comments
marcaaron Aug 24, 2022
492a144
rm whitespace
marcaaron Aug 24, 2022
61f3f3f
remove unneeded changes
marcaaron Aug 24, 2022
444fb2f
Update src/pages/home/ReportScreen.js
roryabraham Aug 25, 2022
5f5c063
Fix lint
roryabraham Aug 25, 2022
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
20 changes: 16 additions & 4 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ const propTypes = {

/** Flag to check if the report actions data are loading */
isLoadingReportActions: PropTypes.bool,

/** ID for the report */
reportID: PropTypes.string,
}),

/** Array of report actions for this report */
Expand Down Expand Up @@ -165,22 +168,31 @@ class ReportScreen extends React.Component {
shouldShowLoader() {
// This means there are no reportActions at all to display, but it is still in the process of loading the next set of actions.
const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;
return !getReportID(this.props.route) || isLoadingInitialReportActions;
return !getReportID(this.props.route) || isLoadingInitialReportActions || !this.props.report.reportID;
}

/**
* Persists the currently viewed report id
*/
storeCurrentlyViewedReport() {
const reportID = getReportID(this.props.route);
if (_.isNaN(reportID)) {
const reportIDFromPath = getReportID(this.props.route);
if (_.isNaN(reportIDFromPath)) {
Report.handleInaccessibleReport();
return;
}

// Always reset the state of the composer view when the current reportID changes
toggleReportActionComposeView(true);
Report.updateCurrentlyViewedReportID(reportID);
Report.updateCurrentlyViewedReportID(reportIDFromPath);

// It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that
// is not stored locally yet. In this case, we fetch the report and render the view once it has loaded or redirect if the report
// is no longer accessible.
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
if (this.props.report.reportID) {
return;
}

Report.fetchChatReportsByIDs([reportIDFromPath], true);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either that, or separating the logic into three different pieces to handle each side-effect.

I like this idea. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

We can create a GH for this, maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with this, but also agree with not blocking on it for now

}

/**
Expand Down
5 changes: 0 additions & 5 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ class ReportActionsView extends React.Component {
Report.openReport(this.props.report.reportID);
});

// If the reportID is not found then we have either not loaded this chat or the user is unable to access it.
// We will attempt to fetch it and redirect if still not accessible.
if (!this.props.report.reportID) {
Report.fetchChatReportsByIDs([this.props.report.reportID], true);
}
Report.subscribeToReportTypingEvents(this.props.report.reportID);
this.keyboardEvent = Keyboard.addListener('keyboardDidShow', () => {
if (!ReportActionComposeFocusManager.isFocused()) {
Expand Down