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.

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

Refactor OpenReport API call for Report GetHistory #10164

Merged
merged 33 commits into from
Aug 18, 2022
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
713e1e0
removing fetch data
sketchydroide Jul 28, 2022
de85a84
sequence number is no longer used
sketchydroide Jul 28, 2022
1c7bce4
reorganizing this a bit
sketchydroide Aug 1, 2022
8408423
removing updateNewMarkerAndMarkReadOnce
sketchydroide Aug 2, 2022
7f11c94
the view will decide on this
sketchydroide Aug 2, 2022
b301b28
using the isLoadingReportData only if we have no reportActions
sketchydroide Aug 2, 2022
8260661
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 2, 2022
b654d97
the actions does not know if it's the inital loading or not, the scre…
sketchydroide Aug 2, 2022
5e37db3
removing isLoadingReportData
sketchydroide Aug 2, 2022
2705f6e
adding the fetchData back for now until we do the reconnect action
sketchydroide Aug 2, 2022
2763fc4
linter
sketchydroide Aug 3, 2022
9b1b0b6
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 8, 2022
3dc5dc6
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 9, 2022
c87a3e6
remove IS_LOADING_INITIAL_REPORT_ACTIONS
sketchydroide Aug 10, 2022
18d11f3
the OpenReport action is write
sketchydroide Aug 10, 2022
11cddbc
adding why to the doc
sketchydroide Aug 10, 2022
a298893
missing semi colon
sketchydroide Aug 10, 2022
7468ac5
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 11, 2022
630f75c
removing the collection
sketchydroide Aug 11, 2022
d9909a2
adding the comment
sketchydroide Aug 11, 2022
df12dbd
adding for isLoadingMoreReportActions
sketchydroide Aug 11, 2022
a689a90
missed the route
sketchydroide Aug 11, 2022
10b86ee
correcting the compose
sketchydroide Aug 12, 2022
bc124fc
linter
sketchydroide Aug 12, 2022
bca0f8b
styling suggestions
sketchydroide Aug 15, 2022
39fd1ec
forgot indenting
sketchydroide Aug 15, 2022
0be9f97
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 16, 2022
852e636
wrong method changed
sketchydroide Aug 16, 2022
599ca8b
remove fetchActions
sketchydroide Aug 16, 2022
2d7de46
setting the report as required
sketchydroide Aug 17, 2022
8954f03
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 17, 2022
8ebc4f7
Update src/pages/home/report/ReportActionsView.js
marcaaron Aug 17, 2022
d2695b2
Merge branch 'main' into afonseca_get_history_open_report
sketchydroide Aug 18, 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
2 changes: 0 additions & 2 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ export default {
POLICY: 'policy_',
REPORTS_WITH_DRAFT: 'reportWithDraft_',
REPORT_IS_COMPOSER_FULL_SIZE: 'reportIsComposerFullSize_',
IS_LOADING_INITIAL_REPORT_ACTIONS: 'isLoadingInitialReportActions_',
IS_LOADING_MORE_REPORT_ACTIONS: 'isLoadingMoreReportActions_',
POLICY_MEMBER_LIST: 'policyMemberList_',
},

Expand Down
54 changes: 30 additions & 24 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -625,15 +625,14 @@ function fetchOrCreateChatReport(participants, shouldNavigate = true) {
}

/**
* Get the actions of a report
* Get the initial actions of a report
*
* @param {Number} reportID
* @returns {Promise}
*/
function fetchActions(reportID) {
function fetchInitialActions(reportID) {
const reportActionsOffset = -1;

return DeprecatedAPI.Report_GetHistory({
DeprecatedAPI.Report_GetHistory({
reportID,
reportActionsOffset,
reportActionsLimit: CONST.REPORT.ACTIONS.LIMIT,
Expand All @@ -644,17 +643,6 @@ function fetchActions(reportID) {
});
}

/**
* Get the initial actions of a report
*
* @param {Number} reportID
*/
function fetchInitialActions(reportID) {
Onyx.set(`${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${reportID}`, true);
fetchActions(reportID)
.finally(() => Onyx.set(`${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${reportID}`, false));
}

/**
* Get all of our reports
*
Expand Down Expand Up @@ -989,22 +977,34 @@ function deleteReportComment(reportID, reportAction) {
* @param {Number} reportID
*/
function openReport(reportID) {
const sequenceNumber = getMaxSequenceNumber(reportID);
API.write('OpenReport',
{
reportID,
sequenceNumber,
},
{
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
lastReadSequenceNumber: sequenceNumber,
isLoadingReportActions: true,
lastVisitedTimestamp: Date.now(),
unreadActionCount: 0,
},
}],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: false,
},
}],
failureData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: false,
},
}],
});
}

Expand All @@ -1024,18 +1024,24 @@ function readOldestAction(reportID, oldestActionSequenceNumber) {
{
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_MORE_REPORT_ACTIONS}${reportID}`,
value: true,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingMoreReportActions: true,
},
}],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_MORE_REPORT_ACTIONS}${reportID}`,
value: false,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingMoreReportActions: false,
},
}],
failureData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_MORE_REPORT_ACTIONS}${reportID}`,
value: false,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingMoreReportActions: false,
},
}],
});
}
Expand Down
18 changes: 8 additions & 10 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ const propTypes = {

/** Whether there is an outstanding amount in IOU */
hasOutstandingIOU: PropTypes.bool,

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

/** Array of report actions for this report */
Expand All @@ -71,9 +74,6 @@ const propTypes = {
/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),

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

/** The policies which the user has access to */
policies: PropTypes.objectOf(PropTypes.shape({
/** The policy name */
Expand All @@ -99,10 +99,10 @@ const defaultProps = {
unreadActionCount: 0,
maxSequenceNumber: 0,
hasOutstandingIOU: false,
isLoadingReportActions: false,
},
isComposerFullSize: false,
betas: [],
isLoadingInitialReportActions: false,
policies: {},
};

Expand Down Expand Up @@ -146,7 +146,6 @@ class ReportScreen extends React.Component {
}

componentWillUnmount() {
clearTimeout(this.loadingTimerId);
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
this.removeViewportResizeListener();
}
marcaaron marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -163,11 +162,14 @@ class ReportScreen extends React.Component {

/**
* When reports change there's a brief time content is not ready to be displayed
* It Should show the loader if it's the first time we are opening the report
*
* @returns {Boolean}
*/
shouldShowLoader() {
return !getReportID(this.props.route) || (_.isEmpty(this.props.reportActions) && this.props.isLoadingInitialReportActions);
// 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;
}

/**
Expand Down Expand Up @@ -303,10 +305,6 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
isLoadingInitialReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${getReportID(route)}`,
initWithStoredValues: false,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
Expand Down
61 changes: 10 additions & 51 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
Keyboard,
AppState,
} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import _ from 'underscore';
import lodashGet from 'lodash/get';
Expand All @@ -22,7 +21,6 @@ import ReportActionComposeFocusManager from '../../../libs/ReportActionComposeFo
import * as ReportActionContextMenu from './ContextMenu/ReportActionContextMenu';
import PopoverReportActionContextMenu from './ContextMenu/PopoverReportActionContextMenu';
import Performance from '../../../libs/Performance';
import ONYXKEYS from '../../../ONYXKEYS';
import {withNetwork} from '../../../components/OnyxProvider';
import * as EmojiPickerAction from '../../../libs/actions/EmojiPickerAction';
import FloatingMessageCounter from './FloatingMessageCounter';
Expand Down Expand Up @@ -51,6 +49,9 @@ const propTypes = {

/** Whether there is an outstanding amount in IOU */
hasOutstandingIOU: PropTypes.bool,

/** Are we loading more report actions? */
isLoadingMoreReportActions: PropTypes.bool,
}),

/** Array of report actions for this report */
Expand All @@ -65,12 +66,6 @@ const propTypes = {
/** Whether the composer is full size */
isComposerFullSize: PropTypes.bool.isRequired,

/** Are we loading more report actions? */
isLoadingMoreReportActions: PropTypes.bool,

/** Are we waiting for more report data? */
isLoadingReportData: PropTypes.bool,

/** Information about the network */
network: networkPropTypes.isRequired,

Expand All @@ -84,11 +79,10 @@ const defaultProps = {
unreadActionCount: 0,
maxSequenceNumber: 0,
hasOutstandingIOU: false,
isLoadingMoreReportActions: false,
},
reportActions: {},
session: {},
isLoadingMoreReportActions: false,
isLoadingReportData: false,
};

class ReportActionsView extends React.Component {
Expand Down Expand Up @@ -116,7 +110,6 @@ class ReportActionsView extends React.Component {
this.loadMoreChats = this.loadMoreChats.bind(this);
this.recordTimeToMeasureItemLayout = this.recordTimeToMeasureItemLayout.bind(this);
this.scrollToBottomAndMarkReportAsRead = this.scrollToBottomAndMarkReportAsRead.bind(this);
this.updateNewMarkerAndMarkReadOnce = _.once(this.updateNewMarkerAndMarkRead.bind(this));
}

componentDidMount() {
Expand All @@ -141,11 +134,7 @@ class ReportActionsView extends React.Component {
ReportScrollManager.scrollToBottom();
});

if (!this.props.isLoadingReportData) {
this.updateNewMarkerAndMarkReadOnce();
}

this.fetchData();
Report.openReport(this.props.reportID);
sketchydroide marked this conversation as resolved.
Show resolved Hide resolved
}

shouldComponentUpdate(nextProps, nextState) {
Expand All @@ -164,11 +153,7 @@ class ReportActionsView extends React.Component {
return true;
}

if (nextProps.isLoadingMoreReportActions !== this.props.isLoadingMoreReportActions) {
return true;
}

if (!nextProps.isLoadingReportData && this.props.isLoadingReportData) {
if (nextProps.report.isLoadingMoreReportActions !== this.props.report.isLoadingMoreReportActions) {
return true;
}

Expand Down Expand Up @@ -203,14 +188,9 @@ class ReportActionsView extends React.Component {
if (lodashGet(prevProps.network, 'isOffline') && !lodashGet(this.props.network, 'isOffline')) {
if (this.getIsReportFullyVisible()) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
Report.openReport(this.props.reportID);
} else {
this.fetchData();
}
this.fetchData();
}

// Update the last read action for the report currently in view when report data finishes loading.
// This report should now be up-to-date and since it is in view we mark it as read.
if (!this.props.isLoadingReportData && prevProps.isLoadingReportData) {
this.updateNewMarkerAndMarkReadOnce();
}

// The last sequenceNumber of the same report has changed.
Expand Down Expand Up @@ -284,7 +264,7 @@ class ReportActionsView extends React.Component {
*/
loadMoreChats() {
// Only fetch more if we are not already fetching so that we don't initiate duplicate requests.
if (this.props.isLoadingMoreReportActions) {
if (this.props.report.isLoadingMoreReportActions) {
return;
}

Expand Down Expand Up @@ -350,18 +330,6 @@ class ReportActionsView extends React.Component {
});
}

/**
* Update NEW marker and mark report as read
*/
updateNewMarkerAndMarkRead() {
this.updateNewMarkerPosition(this.props.report.unreadActionCount);

// Only mark as read if the report is fully visible
if (this.getIsReportFullyVisible()) {
Report.openReport(this.props.reportID);
}
}

/**
* Show the new floating message counter
*/
Expand Down Expand Up @@ -431,7 +399,7 @@ class ReportActionsView extends React.Component {
onLayout={this.recordTimeToMeasureItemLayout}
sortedReportActions={this.sortedReportActions}
mostRecentIOUReportSequenceNumber={this.mostRecentIOUReportSequenceNumber}
isLoadingMoreReportActions={this.props.isLoadingMoreReportActions}
isLoadingMoreReportActions={this.props.report.isLoadingMoreReportActions}
loadMoreChats={this.loadMoreChats}
/>
<PopoverReportActionContextMenu ref={ReportActionContextMenu.contextMenuRef} />
Expand All @@ -453,13 +421,4 @@ export default compose(
withDrawerState,
withLocalize,
withNetwork(),
withOnyx({
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the component get this.props.report now without using withOnyx()?

Copy link
Contributor

Choose a reason for hiding this comment

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

from the parent component here:

<ReportActionsView
reportID={reportID}
reportActions={this.props.reportActions}
report={this.props.report}

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, Thanks! I don't think we should pass both the reportID and report. Those are redundant. Let's just pass report and get the reportID off that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, but reportID at the moment is required not sure if we should make the report required as well, not sure if we should have a default value for reportID

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like an easy change and a good thing to look into. I also think it would be OK to do in a follow up and not add additional refactoring scope to these changes. I don't immediately see a clear reason for passing the reportID and report as separate props, but I haven't looked into it deeply enough to say there isn't one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make report the required prop then, but as always, make sure you take a look at the code and understand it first before making a change like that.

I won't fight tooth-and-nail about it being done in this PR, but I also think this is a refactor PR and I am not too concerned about scope for something small like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just made the change and tested it after, seems to work great, let me know if you are happy with that @tgolen

isLoadingReportData: {
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
},
isLoadingMoreReportActions: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.IS_LOADING_MORE_REPORT_ACTIONS}${reportID}`,
initWithStoredValues: false,
},
}),
)(ReportActionsView);