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

jolivervidal · 2021-10-28T11:13:16Z

Details

Verification added for updating the list of unread messages only if the reports data is up to date

Fixed Issues

$ #5405

Tests

Open a chat in the browser with the purpose to create cached data
Close the tab
Open the same chat on an other device
Send a message to this chat
Open the same chat again on the browser used at the point 1
Verify that the message sent by ourself in the other device does not appear as unread
Repeat the same steps with the other devices

QA Steps

The same steps as above

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-10-28T11:13:36Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

jolivervidal · 2021-10-28T11:15:44Z

I have read the CLA Document and I hereby sign the CLA

johnmlee101

A few minor changes, I'm going to add another reviewer into the mix as well

src/libs/actions/Report.js

johnmlee101 · 2021-10-28T17:42:28Z

@parasharrajat (it says your busy so lmk if you'd rather ignore) or @Beamanator did you want to do a secondary review?

jolivervidal · 2021-10-28T17:58:40Z

Updated. And waiting for secondary review by one of the other engineers

parasharrajat · 2021-11-01T09:57:21Z

@johnmlee101 Thanks for tagging in. Yup, I was busy but now I am available so reviewing it.

src/libs/actions/Report.js

src/pages/home/report/ReportActionsView.js

parasharrajat

The code looks good. ~~I have not tested it with the reproduction steps but I presume that it is working.~~

Just tested it on the Web with the exact reproduction steps and it is working well.

# Conflicts: # src/libs/actions/Report.js # src/pages/home/report/ReportActionsView.js

# Conflicts: # src/libs/actions/Report.js

kidroca

I'm happy that this solution works, but I think we can make it better if we prevent updates during consecutive loads as well

The vast majority of the time the bug would happen during startup, but there's always a chance that on a consecutive fetch something that we don't have locally might come

One of the potential cases is when we suspend the app for a long time then resume the app - see onVisibilityChange. When we focus the app there's a call that would again fetch reports, but we might false set a last read if the call hasn't completed yet.
While the app is suspended we can't guarantee it has received all updates I guess that's why we're re-fetching reports on focus

I think we should go with something like: if reports are loading don't update the last read message

in case something new comes - we have new messages
if nothing new comes - we haven't falsely marked it as unread

So instead of preventing setting last read only during initial report loading, we can prevent it if it happens during report loading in general

kidroca · 2021-11-05T08:59:04Z

src/libs/actions/Report.js

+function isReportDataUptoDate() {
+    return isReportDataFresh;
+}


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

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

kidroca · 2021-11-05T09:32:29Z

src/pages/home/report/ReportActionsView.js

        if (!this.props.isDrawerOpen) {
-            Report.updateLastReadActionID(this.props.reportID);
+            if (Report.isReportDataUptoDate()) {
+                Report.updateLastReadActionID(this.props.reportID);
+            }
        }


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

…n checks its value before to update a sequence number

jolivervidal · 2021-11-05T16:21:59Z

Updated regarding @kidroca suggestions.

mallenexpensify · 2021-11-05T22:36:20Z

Dropping this link in here so we'll hopefully get a ping when this closes, we're holding on it #3981

jolivervidal · 2021-11-09T16:54:37Z

Waiting for @johnmlee101 review

…read

johnmlee101 · 2021-11-16T16:19:28Z

Was busy last week, looking now!

johnmlee101

Looks good to me! @kidroca did you want a final pass of review?

kidroca

thanks @johnmlee101 LGTM!

OSBotify · 2021-11-17T18:35:24Z

@jolivervidal, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

OSBotify · 2021-11-17T18:35:49Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-11-22T20:53:34Z

🚀 Deployed to staging by @johnmlee101 in version: 1.1.15-18 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-11-29T18:49:09Z

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

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

5dd71c1

jolivervidal requested a review from a team as a code owner October 28, 2021 11:13

MelvinBot requested review from johnmlee101 and removed request for a team October 28, 2021 11:13

johnmlee101 assigned jolivervidal Oct 28, 2021

johnmlee101 requested changes Oct 28, 2021

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

Variable renamed to be more clear and grammatical correction done

bed6ebc

parasharrajat suggested changes Nov 1, 2021

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

parasharrajat previously approved these changes Nov 3, 2021

View reviewed changes

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

78f3cb4

# Conflicts: # src/libs/actions/Report.js # src/pages/home/report/ReportActionsView.js

jolivervidal dismissed parasharrajat’s stale review via 78f3cb4 November 3, 2021 13:53

jolivervidal requested a review from johnmlee101 November 3, 2021 14:00

parasharrajat mentioned this pull request Nov 3, 2021

[HOLD for payment 2021-12-06] Own messages are marked as unread when opening the app in another device #5405

Closed

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

ccf2684

# Conflicts: # src/libs/actions/Report.js

kidroca reviewed Nov 5, 2021

View reviewed changes

kidroca mentioned this pull request Nov 5, 2021

[HOLD #8267] Web - Read messages are marked unread after a while #3981

Closed

jolivervidal added 2 commits November 5, 2021 16:18

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

8c90fab

IS_LOADING_REPORT_DATA Onyx key added. updateLastReadActionID functio…

74bb201

…n checks its value before to update a sequence number

jolivervidal added 2 commits November 9, 2021 11:16

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

462418d

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

800160d

jolivervidal added 2 commits November 10, 2021 09:32

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

e887eaa

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

ad63b8c

jolivervidal added 2 commits November 15, 2021 18:06

Merge branch 'main' into jolivervidal-ownMessagesMarkedAsUnread

eac3653

Merge branch 'Expensify:main' into jolivervidal-ownMessagesMarkedAsUn…

411428b

…read

johnmlee101 approved these changes Nov 16, 2021

View reviewed changes

kidroca approved these changes Nov 16, 2021

View reviewed changes

johnmlee101 merged commit e60aa8c into Expensify:main Nov 17, 2021

OSBotify mentioned this pull request Nov 22, 2021

Deploy Checklist: New Expensify 2021-11-22 #6399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jolivervidal commented Oct 28, 2021

github-actions bot commented Oct 28, 2021 •

edited

Loading

jolivervidal commented Oct 28, 2021

johnmlee101 left a comment

johnmlee101 commented Oct 28, 2021

jolivervidal commented Oct 28, 2021

parasharrajat commented Nov 1, 2021

parasharrajat left a comment •

edited

Loading

kidroca left a comment

kidroca Nov 5, 2021

jolivervidal Nov 5, 2021

kidroca Nov 5, 2021

jolivervidal Nov 5, 2021

jolivervidal commented Nov 5, 2021

mallenexpensify commented Nov 5, 2021

jolivervidal commented Nov 9, 2021

johnmlee101 commented Nov 16, 2021

johnmlee101 left a comment

kidroca left a comment

OSBotify commented Nov 17, 2021

OSBotify commented Nov 17, 2021

OSBotify commented Nov 22, 2021

OSBotify commented Nov 29, 2021

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

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

Conversation

jolivervidal commented Oct 28, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Oct 28, 2021 • edited Loading

jolivervidal commented Oct 28, 2021

johnmlee101 left a comment

Choose a reason for hiding this comment

johnmlee101 commented Oct 28, 2021

jolivervidal commented Oct 28, 2021

parasharrajat commented Nov 1, 2021

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

kidroca Nov 5, 2021

Choose a reason for hiding this comment

jolivervidal Nov 5, 2021

Choose a reason for hiding this comment

kidroca Nov 5, 2021

Choose a reason for hiding this comment

jolivervidal Nov 5, 2021

Choose a reason for hiding this comment

jolivervidal commented Nov 5, 2021

mallenexpensify commented Nov 5, 2021

jolivervidal commented Nov 9, 2021

johnmlee101 commented Nov 16, 2021

johnmlee101 left a comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 17, 2021

OSBotify commented Nov 17, 2021

OSBotify commented Nov 22, 2021

OSBotify commented Nov 29, 2021

github-actions bot commented Oct 28, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading