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

check if reports data is up to date before update last read action id #6095

Merged
merged 12 commits into from
Nov 17, 2021
Merged

check if reports data is up to date before update last read action id #6095

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5dd71c1
check if reports data is up to date before update last read action id
jolivervidal Oct 28, 2021
bed6ebc
Variable renamed to be more clear and grammatical correction done
jolivervidal Oct 28, 2021
78f3cb4
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 3, 2021
ccf2684
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 4, 2021
8c90fab
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 5, 2021
74bb201
IS_LOADING_REPORT_DATA Onyx key added. updateLastReadActionID functio…
jolivervidal Nov 5, 2021
462418d
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 9, 2021
800160d
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 9, 2021
e887eaa
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 10, 2021
ad63b8c
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 11, 2021
eac3653
Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread
jolivervidal Nov 15, 2021
411428b
Merge branch 'Expensify:main' into jolivervidal-ownMessagesMarkedAsUn…
jolivervidal Nov 16, 2021
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
13 changes: 13 additions & 0 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ const lastReadSequenceNumbers = {};
// since we will then be up to date and any optimistic actions that are still waiting to be replaced can be removed.
const optimisticReportActionIDs = {};

// Boolean to indicate if report data comes from the API request or from local cache.
let isReportDataFresh = false;

/**
* Checks the report to see if there are any unread action items
*
Expand Down Expand Up @@ -952,6 +955,7 @@ function fetchAllReports(
})
.then((returnedReports) => {
Onyx.set(ONYXKEYS.INITIAL_REPORT_DATA_LOADED, true);
isReportDataFresh = true;
jolivervidal marked this conversation as resolved.
Show resolved Hide resolved

// If at this point the user still doesn't have a Concierge report, create it for them.
// This means they were a participant in reports before their account was created (e.g. default rooms)
Expand Down Expand Up @@ -1413,6 +1417,14 @@ function handleInaccessibleReport() {
navigateToConciergeChat();
}

/**
* Check if report data is up to date or it comes from local cache
* @returns {Boolean}
*/
function isReportDataUptoDate() {
return isReportDataFresh;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really telling us whether report data is up to date, but whether initial report fetching is over.
The return value will stay true ever since

Without a server check we can't reliably know whether we're up to date, we can only guess that we're not
To know whether we're synced we should at least check

  • we're online
  • we're connected to pusher
  • we have no pending request regarding reports (or anything)

Then, yes we're probably up to date

IMO the variable should be renamed to isReportDataLoading or isReportDataLoaded and should be updated at the start of the load and at then end, similarly to how fetchActionsWithLoadingState works

If that's the case we might also put in in ONYXKEYS e.g. isReportDataLoading and then subscribe the ReportActionsView to that key

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 agree. It's a better solution to use a more generic approach like this. I added a new ONYX KEY called IS_LOADING_REPORT_DATA. I chose that name to keep it the same way the existing loading keys.
This key is set in the fetchAllReports function and it updates the variable called ´isReportDataLoading´ in the Report.js. It is not necessary to subscribe the view to that key, because I moved the if statement inside the updateLastReadID like you suggested


export {
fetchAllReports,
fetchActions,
Expand All @@ -1439,6 +1451,7 @@ export {
syncChatAndIOUReports,
navigateToConciergeChat,
handleInaccessibleReport,
isReportDataUptoDate,
setReportWithDraft,
fetchActionsWithLoadingState,
};
4 changes: 3 additions & 1 deletion src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ class ReportActionsView extends React.Component {

// Only mark as read if the report is open
if (!this.props.isDrawerOpen) {
Report.updateLastReadActionID(this.props.reportID);
if (Report.isReportDataUptoDate()) {
Report.updateLastReadActionID(this.props.reportID);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to move this check inside updateLastReadActionID
There are other places that could call updateLastReadActionID while data is still being fetch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount);
Expand Down