Refactor Update Last Read #9529

madmax330 · 2022-06-22T10:47:33Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/212871

Tests

Verify that no errors appear in the JS console
In one chrome log into newDot
In another incognito window log into another account on newDot
Send messages to the first user
If this is a new chat make sure when you click on the chat the messages are marked as read
If this is an existing chat:
- If the report is already open, make sure messages are marked as read as they come in
- If the report is not already open, make sure messages are marked as read when you click on the report

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

See "Tests"

Screenshots

This is not a UI change, so I tested on web to show the right network requests being made to the new API commands

Web

update.read.mov

Mobile Web

Screen.Recording.2022-07-01.at.7.10.54.PM.mov

Desktop

Screen.Recording.2022-07-01.at.7.07.51.PM.mov

iOS

Android

update branch

madmax330 · 2022-06-29T13:47:10Z

Updated and tested, this is ready for review

marcaaron · 2022-06-29T19:46:54Z

Adding @arosiclair as well since he is tagged on the Web-E PR

arosiclair · 2022-06-30T14:31:05Z

I'm seeing an issue where occasionally the report is marked as read briefly and then goes back to unread after switching to it. Seems to be caused by Report_GetHistory getting chained.

Will the functionality for Report_GetHistory get refactored into OpenReport with another issue or should it be this one?

Screen.Recording.2022-06-30.at.10.22.43.AM.mov

arosiclair · 2022-06-30T14:34:12Z

@madmax330 also you have to flesh out your QA steps (even if its just "same as tests") and screenshots sections

madmax330 · 2022-06-30T14:39:56Z

Will the functionality for Report_GetHistory get refactored into OpenReport with another issue or should it be this one?

Good observation, Report_GetHistory is a separate refactor that will update this behavior. See: https://github.com/Expensify/Expensify/issues/212871#issuecomment-1168063361

also you have to flesh out your QA steps (even if its just "same as tests")

Good call.

and screenshots sections

Not sure what to include for this still since it's not a UI change really, but I'll include a video or the indicator updating and the network tab to show that we're making the requests to the new API

madmax330 · 2022-06-30T14:59:13Z

Updated and added a description to the screenshots section

arosiclair · 2022-07-01T14:04:03Z

Not sure what to include for this still since it's not a UI change really, but I'll include a video or the indicator updating and the network tab to show that we're making the requests to the new API

Thanks but you'll have to include recordings for each platform unfortunately since this is UI behavior we're implementing. I'll be testing this on each platform too (minus Android since the build is broken on M1 right now).

madmax330 · 2022-07-01T15:46:44Z

Cool, I've added videos for mweb, and desktop. I'll add ios, and android sometime next week b/c it's not working rn.

since this is UI behavior we're implementing

I don't agree with this. These changes don't modify or add any UI components so none of the components will look different than they did before this PR.
All it's modifying is the api calls.

arosiclair · 2022-07-01T18:50:26Z

I don't agree with this. These changes don't modify or add any UI components so none of the components will look different than they did before this PR. All it's modifying is the api calls.

I don't like the cross-platform testing requirement for functional changes either, but the guidelines don't really give us an exception. I'll ask for clarification in eng-chat.

EDIT: sounds like we should

src/libs/actions/Report.js

marcaaron · 2022-07-01T00:37:45Z

src/pages/home/report/ReportActionsView.js

@@ -201,6 +202,7 @@ class ReportActionsView extends React.Component {

    componentDidUpdate(prevProps) {
        if (prevProps.network.isOffline && !this.props.network.isOffline) {
+            Report.openReport(this.props.reportID);
            this.fetchData();


Just noting: Once the report actions fetching stuff is refactored this call should not be here anymore. cc @sketchydroide

src/pages/home/report/ContextMenu/ContextMenuActions.js

src/pages/home/report/ReportActionsView.js

udpate branch

madmax330 · 2022-07-06T21:00:29Z

I was retesting this and it seems nothing is ever unread anymore.
When I open two chats, one normal one incognito, the latest chats always show as read already:

Screen.Recording.2022-07-06.at.10.58.44.PM.mov

I'm on latest web and app, I think it's the AddComment PR that broke this (b/c even without my latest changes it's not working). Do I need to be on the latest Auth as well @marcaaron ?

marcaaron · 2022-07-07T02:11:39Z

I'm on latest web and app, I think it's the AddComment PR that broke this (b/c even without my latest changes it's not working). Do I need to be on the latest Auth as well @marcaaron ?

Shouldn't need any Auth changes. Is it broken if you switch to main?

madmax330 · 2022-07-07T14:53:32Z

Is it broken if you switch to main?

Yeah it's broken on main for me.

marcaaron · 2022-07-07T17:31:14Z

Ok, it does seem like something is broken 😬

On a fresh chat with a new user it will show as unread, but leaving additional comments and they are not showing as unread. Will take a look (unless you have an idea about what is broken).

madmax330 · 2022-07-07T17:37:49Z

unless you have an idea about what is broken

I don't, but I can look as well 🙃

marcaaron · 2022-07-07T17:45:42Z

Just documenting here what I am seeing as well... first comment from a user seems OK later comments not so much.

2022-07-07_07-44-23.mp4

marcaaron · 2022-07-07T18:10:38Z

Ok so doing some debugging and I think it has to do with the timestamp check. For some reason, the timestamp of the actions getting created on dev is off by a solid 30 seconds.

That means the logic here won't run...

App/src/libs/actions/Report.js

Lines 1495 to 1498 in eb54f23

    
           // If we are past the deadline to notify for this comment don't do it 
        
           if (moment.utc(action.timestamp * 1000).isBefore(moment.utc().subtract(10, 'seconds'))) { 
        
               return; 
        
           }

Seems to work ok on staging though.

marcaaron · 2022-07-07T18:46:06Z

Ok, so pretty weird stuff, but I think we are both affected by the VM clock drifting based on this thread -> https://expensify.slack.com/archives/C03TQ48KC/p1657218624480699

After a vagrant reload the unreads started to work normally again for me.

Which also explains why nobody noticed this and why it worked fine for me before but suddenly stopped working 😓

madmax330 · 2022-07-07T20:12:14Z

Oh wow, you're right, restarting the vm fixed it. I wonder what happened between yesterday and the day before for my clock to skew by 30s 😕

madmax330 · 2022-07-07T20:17:01Z

Aright, I addressed the comments and re-tested.
The rest will either be removed in the PR that will implement OpenReport or in your PR to clean up the unread count stuff.
lmk what you think

marcaaron

Sorry @madmax330 meant to approve this but changes LGTM. Gonna wrap up a few other things in my open PR so you don't get clobbered with conflicts 😅

src/libs/actions/Report.js

marcaaron · 2022-07-09T01:17:24Z

src/libs/actions/Report.js

+                onyxMethod: 'merge',
+                key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
+                value: {
+                    lastVisitedTimestamp: Math.round(Date.now() / 1000),


NAB and sorry, as mentioned in that other comment I was wrong about this and Date.now() is totally fine to use as it returns milliseconds and the lastVisitedTimestamp should be in ms.

melvin-bot · 2022-07-09T01:29:05Z

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-07-09T01:34:48Z

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

marcaaron · 2022-07-09T01:52:44Z

GH actions did pass.

OSBotify · 2022-07-12T21:34:02Z

🚀 Deployed to staging by @marcaaron in version: 1.1.83-1 🚀

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

OSBotify · 2022-07-20T22:27:38Z

🚀 Deployed to production by @chiragsalian in version: 1.1.84-13 🚀

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

Refactor markasunread

fa49894

madmax330 requested review from marcaaron and a team as code owners June 22, 2022 10:47

madmax330 self-assigned this Jun 22, 2022

melvin-bot bot requested review from johnmlee101 and removed request for a team June 22, 2022 10:47

madmax330 added 5 commits June 23, 2022 11:23

Read newest action

52cb17c

add openreport

f16573a

Merge branch 'main' into maxence-rrulr

148da7b

update branch

polish + style

dda655e

revert package lock

df64eae

madmax330 changed the title ~~[WIP] Refactor Update Last Read~~ Refactor Update Last Read Jun 29, 2022

madmax330 changed the title ~~Refactor Update Last Read~~ [HOLD Web 34097] Refactor Update Last Read Jun 29, 2022

marcaaron requested a review from arosiclair June 29, 2022 19:46

madmax330 changed the title ~~[HOLD Web 34097] Refactor Update Last Read~~ Refactor Update Last Read Jul 1, 2022

marcaaron requested changes Jul 5, 2022

View reviewed changes

madmax330 added 2 commits July 6, 2022 19:33

Merge branch 'main' into maxence-rrulr

bccb6a8

udpate branch

Address comments

d4d41d3

marcaaron mentioned this pull request Jul 7, 2022

Clean up lastReadSequenceNumbers and reportMaxSequenceNumbers #9755

Merged

88 tasks

Address comments again

112c986

marcaaron approved these changes Jul 9, 2022

View reviewed changes

marcaaron merged commit a0c4e83 into main Jul 9, 2022

marcaaron deleted the maxence-rrulr branch July 9, 2022 01:29

melvin-bot bot added the Emergency label Jul 9, 2022

marcaaron removed the Emergency label Jul 9, 2022

OSBotify mentioned this pull request Jul 12, 2022

Deploy Checklist: New Expensify 2022-07-12 #9847

Closed

95 tasks

hoangzinh mentioned this pull request Oct 4, 2023

[HOLD for payment 2023-11-09] [$1000] Web - API OpenReport shows after switch away from the app and back #27887

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Update Last Read #9529

Refactor Update Last Read #9529

madmax330 commented Jun 22, 2022 •

edited

Loading

madmax330 commented Jun 29, 2022

marcaaron commented Jun 29, 2022

arosiclair commented Jun 30, 2022 •

edited

Loading

arosiclair commented Jun 30, 2022

madmax330 commented Jun 30, 2022 •

edited

Loading

madmax330 commented Jun 30, 2022

arosiclair commented Jul 1, 2022

madmax330 commented Jul 1, 2022

arosiclair commented Jul 1, 2022 •

edited

Loading

marcaaron Jul 1, 2022

madmax330 commented Jul 6, 2022

marcaaron commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron commented Jul 7, 2022

marcaaron commented Jul 7, 2022

marcaaron commented Jul 7, 2022 •

edited

Loading

madmax330 commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron left a comment

marcaaron Jul 9, 2022

melvin-bot bot commented Jul 9, 2022

OSBotify commented Jul 9, 2022

marcaaron commented Jul 9, 2022

OSBotify commented Jul 12, 2022

OSBotify commented Jul 20, 2022

Refactor Update Last Read #9529

Refactor Update Last Read #9529

Conversation

madmax330 commented Jun 22, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

madmax330 commented Jun 29, 2022

marcaaron commented Jun 29, 2022

arosiclair commented Jun 30, 2022 • edited Loading

arosiclair commented Jun 30, 2022

madmax330 commented Jun 30, 2022 • edited Loading

madmax330 commented Jun 30, 2022

arosiclair commented Jul 1, 2022

madmax330 commented Jul 1, 2022

arosiclair commented Jul 1, 2022 • edited Loading

marcaaron Jul 1, 2022

Choose a reason for hiding this comment

madmax330 commented Jul 6, 2022

marcaaron commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron commented Jul 7, 2022

marcaaron commented Jul 7, 2022

marcaaron commented Jul 7, 2022 • edited Loading

madmax330 commented Jul 7, 2022

madmax330 commented Jul 7, 2022

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Jul 9, 2022

Choose a reason for hiding this comment

melvin-bot bot commented Jul 9, 2022

OSBotify commented Jul 9, 2022

marcaaron commented Jul 9, 2022

OSBotify commented Jul 12, 2022

OSBotify commented Jul 20, 2022

madmax330 commented Jun 22, 2022 •

edited

Loading

arosiclair commented Jun 30, 2022 •

edited

Loading

madmax330 commented Jun 30, 2022 •

edited

Loading

arosiclair commented Jul 1, 2022 •

edited

Loading

marcaaron commented Jul 7, 2022 •

edited

Loading