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 11 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
1 change: 1 addition & 0 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export default {
REPORTS_WITH_DRAFT: 'reportWithDraft_',
REPORT_IS_COMPOSER_FULL_SIZE: 'reportIsComposerFullSize_',
IS_LOADING_INITIAL_REPORT_ACTIONS: 'isLoadingInitialReportActions_',
IS_LOADING_REPORT_ACTIONS: 'isLoadingReportActions_',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between this collection, the one above it, and the one below it? This feels like it's getting a little bit out of hand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using collections for these things at all?

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 can remove the IS_LOADING_INITIAL_REPORT_ACTIONS one. It seems like nothing is subscribing to this key. So that just leaves the others...

But I think we are using collections because:

  • an individual report could be "loading" or "loading more" while not in view and we don't want to incorrectly show a report as loading when it's not or not loading when it is
  • there wasn't any other alternative proposed

Any specific concern with this approach?

Thinking on what we should we do as an alternative... One idea is to possibly use the report object to store a isLoadingActions and isLoadingMoreActions. The report object is a little bloated already with all sorts of random stuff, but maybe it's better than what we have here.

Copy link
Contributor

Choose a reason for hiding this comment

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

One idea is to possibly use the report object to store a isLoadingActions and isLoadingMoreActions.

Yeah, this is basically what I would propose otherwise. It sounds like that's the best practice considering pattern B.

My concern is that I think this leads to an anti-pattern. It becomes a bunch of keys holding a single boolean value, when the boolean value could be a property on the normal report collection. If a new engineer looks at this, and sees it's OK, then what are the next collections with single values? Collections for just report names? You can use this argument to say that instead of one collection with n reports, then it's OK to have n collections for n reports (one collection for each report property).

Unless there is a real advantage to having these kinds of simple value collections (I am just not seeing it), I think we should avoid it and stick to our best practices. (not trying to blame anyone!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wanna move this to Slack to discuss? After, we can put it down in the style guide / fix anything that is not following the best practice.

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 will remove the isLoadingInitialReportActions_

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok seems like we had a good discussion here and are adding this to the style guide.

So, @sketchydroide we just need to update these so that the boolean properties are on the report instead of collection keys.

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 think I have done this for the isLoadingReportActions
Should I do it for the IS_LOADING_MORE_REPORT_ACTIONS as well in this PR as well, it's from the previous PR for ReadOldestAction

Copy link
Contributor Author

@sketchydroide sketchydroide Aug 11, 2022

Choose a reason for hiding this comment

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

I'm not 100% I did that correctly, but I think so, did I miss something? it feels odd that isLoadingReportActions is not always defined...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks correct to me.

it feels odd that isLoadingReportActions is not always defined...

This is JavaScript which means 99% of all things are possibly undefined, but hey at least it's a boolean!

IS_LOADING_MORE_REPORT_ACTIONS: 'isLoadingMoreReportActions_',
POLICY_MEMBER_LIST: 'policyMemberList_',
},
Expand Down
34 changes: 24 additions & 10 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -1049,21 +1049,35 @@ function deleteReportComment(reportID, reportAction) {
* @param {Number} reportID
*/
function openReport(reportID) {
const sequenceNumber = getMaxSequenceNumber(reportID);
API.write('OpenReport',
API.read('OpenReport',
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a write() to me as we are setting the last read sequence number in the command and will want to make sure that happens when we come back from offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right it should be a write

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a write()

{
reportID,
sequenceNumber,
},
{
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
lastReadSequenceNumber: sequenceNumber,
lastVisitedTimestamp: Date.now(),
unreadActionCount: 0,
optimisticData: [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_REPORT_ACTIONS}${reportID}`,
value: true,
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
lastVisitedTimestamp: Date.now(),
unreadActionCount: 0,
},
},
],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_REPORT_ACTIONS}${reportID}`,
value: false,
}],
failureData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.IS_LOADING_REPORT_ACTIONS}${reportID}`,
value: false,
}],
});
}
Expand Down
19 changes: 10 additions & 9 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ const propTypes = {
/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),

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

/** The policies which the user has access to */
policies: PropTypes.objectOf(PropTypes.shape({
Expand All @@ -92,7 +92,7 @@ const defaultProps = {
},
isComposerFullSize: false,
betas: [],
isLoadingInitialReportActions: false,
isLoadingReportActions: false,
};

/**
Expand Down Expand Up @@ -136,10 +136,10 @@ class ReportScreen extends React.Component {
}

componentWillUnmount() {
clearTimeout(this.loadingTimerId);
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
if (window.visualViewport) {
window.visualViewport.removeEventListener('resize', this.viewportOffsetTop);
if (!window.visualViewport) {
return;
}
window.visualViewport.removeEventListener('resize', this.viewportOffsetTop);
}
marcaaron marked this conversation as resolved.
Show resolved Hide resolved

/**
Expand All @@ -163,7 +163,8 @@ class ReportScreen extends React.Component {
* @returns {Boolean}
*/
shouldShowLoader() {
return !getReportID(this.props.route) || (_.isEmpty(this.props.reportActions) && this.props.isLoadingInitialReportActions);
const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.isLoadingReportActions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marc suggested this and I agreed, this makes it easier to understand the purpose of the check

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update the method description in the JSDocs to be more clear? It is answering "what", but not "why"

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this really be more like isLoadingReportActions? It doesn't look like it has anything to do with "initial" actions or not. I'm not even sure what "initial" implies.

Copy link
Contributor

Choose a reason for hiding this comment

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

"initial" means you have no reportActions at all to display vs. having some reportActions to display, but still in the process of loading the next set of actions ones. There are two different animations displayed based on which situation we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you happy with that @tgolen or do you want me to rename the var?

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like Marc's explanation. Could you add that as a code comment to help explain it to others who will see this? If the comment is added, then I think the name is OK then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

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 added a shorter version of Marcs answer, that I feel makes more sense to read, let me know if that works.

return !getReportID(this.props.route) || isLoadingInitialReportActions;
}

/**
Expand Down Expand Up @@ -285,8 +286,8 @@ export default withOnyx({
betas: {
key: ONYXKEYS.BETAS,
},
isLoadingInitialReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${getReportID(route)}`,
isLoadingReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.IS_LOADING_REPORT_ACTIONS}${getReportID(route)}`,
initWithStoredValues: false,
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
},
policies: {
Expand Down
39 changes: 3 additions & 36 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ const propTypes = {
/** 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 @@ -88,7 +85,6 @@ const defaultProps = {
reportActions: {},
session: {},
isLoadingMoreReportActions: false,
isLoadingReportData: false,
};

class ReportActionsView extends React.Component {
Expand Down Expand Up @@ -116,7 +112,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 +136,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 @@ -168,10 +159,6 @@ class ReportActionsView extends React.Component {
return true;
}

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

if (nextState.isFloatingMessageCounterVisible !== this.state.isFloatingMessageCounterVisible) {
return true;
}
Expand Down Expand Up @@ -203,14 +190,9 @@ class ReportActionsView extends React.Component {
if (prevProps.network.isOffline && !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 @@ -350,18 +332,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 @@ -454,9 +424,6 @@ export default compose(
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,
Expand Down