Conversation
| if (isReportFullyVisible) { | ||
| openReportIfNecessary(); | ||
| } else { | ||
| Report.reconnect(reportID); | ||
| } | ||
| openReportIfNecessary(); |
There was a problem hiding this comment.
This is the only case we still need to fetch report with OpenReport because this is not related to loosing/regaining connection, so ReconnectApp is not called.
In the past, we were call Report.reconnect(reportID); instead of openReportIfNecessary(); if the App is in the background/not focused, but I don't think it is important to have this consideration here because this case only happens when we are login in from an anonymous account, and we can't log in with the app in the background.
| useEffect(() => { | ||
| const prevNetwork = prevNetworkRef.current; | ||
| // When returning from offline to online state we want to trigger a request to OpenReport which | ||
| // will fetch the reportActions data and mark the report as read. If the report is not fully visible | ||
| // then we call ReconnectToReport which only loads the reportActions data without marking the report as read. | ||
| const wasNetworkChangeDetected = lodashGet(prevNetwork, 'isOffline') && !lodashGet(props.network, 'isOffline'); | ||
| if (wasNetworkChangeDetected) { | ||
| if (isReportFullyVisible) { | ||
| openReportIfNecessary(); | ||
| } else { | ||
| Report.reconnect(reportID); | ||
| } | ||
| } | ||
| // update ref with current network state | ||
| prevNetworkRef.current = props.network; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [props.network, props.report, isReportFullyVisible]); |
There was a problem hiding this comment.
Not needed anymore because ReconnectApp will fetch the missing updates
| if (report.reportID && report.reportName === undefined) { | ||
| reconnect(report.reportID); | ||
| } |
There was a problem hiding this comment.
TODO: Need to make sure this isn't needed. If reliable updates works properly, I don't think we should miss a report, but 🤷
There was a problem hiding this comment.
This code is very very very very Old (~2020), and it is very likely that things were much more unreliable back then. I think this came from this PR:
https://github.com/Expensify/App/pull/339/files
And have been modified a few times after, for example here: https://github.com/Expensify/App/pull/1075/files#r548053786
I think this can be tested by:
accountAis offline and has never chatted withaccountBbeforeaccountBcreates a DM withaccountAand sends a message.accountAmisses the push eventaccountAgoes onlineaccountBsends another message
That way, when we push the last message sent by accountB in step 4, accountA could have been missing the report, but that is not the case anymore because accountA got the report in step 3 because of "reliable updates".
There was a problem hiding this comment.
Agree with you @aldo-expensify that the app is more reliable. I think this code is basically here for the case you have laid out i.e. it was possible to get a pusher update for a report that you didn't have locally yet. And in that case, we would fetch it.
cc @tgolen to get more eyes on this one just in case he can think of something we are not seeing.
There was a problem hiding this comment.
Huh... Good to see this code is no longer necessary! I agree with both of you and the assumptions.
|
Looks good! seems like testing steps will cover everything |
|
Testing this today. |
|
Something I've noticed (which is happening on staging too) is that a new chat recieved while offline will not show up in the LHN. This somewhat impedes the given testing steps: Screen.Recording.2024-01-24.at.12.09.41.mov |
hmm, this didn't happen when I tested in dev, but I never tested with staging backend. Do you see the missing report and reportActions in the |
|
@aldo-expensify No Onyx data is returned by ReconnectApp, just: |
This is unexpected for me and different from what I was getting while testing in dev. You can HOLD on reviewing for now, I have to fix conflicts anyway and then I'll retest. Old screenshot from my dev testing,
|
|
What's the next step for this PR? I see it has a few conflicts. |
I should test again and see why ReconnectApp is not getting the missing report anymore. |
|
I'm going to make this a DRAFT because I don't think we can merge this until the update ordering problem is solved. |
|
Thanks for that update. Sorry I wasn't following the updates in the issue.
I'll subscribe over there so I follow what's happening!
…On Mon, Mar 25, 2024 at 2:47 PM Aldo Canepa Garay ***@***.***> wrote:
@aldo-expensify <https://github.com/aldo-expensify> what's the status of
this? I think we need to find a way to increase the #urgency of finishing
this. What is blocking you or what else are you focused on right now? Can
you hand off some of that work to others so you can focus on this?
I've been putting the status in the issue here: Expensify/Expensify#325231
(comment)
<Expensify/Expensify#325231 (comment)>
This is being handled with #urgency, I have spent a lot of hours the last
week on it and it was my main focus.
What is blocking you or what else are you focused on right now?
Gave an update here: Expensify/Expensify#325231 (comment)
<Expensify/Expensify#325231 (comment)>
Other stuff that last week was taking time from me and is supposed to be
high priority is:
- Implement the backend changes for QBO in NewDot (spring release)
(done)
- Fix auth/command/AdminUpdateInvoicedCustomer.cpp so it doesn't
timeout for clients with lots of big policies. We need this done before the
end of the month so they can run it before billing (almost done)
Can you hand off some of that work to others so you can focus on this?
I have been helping with deploy blockers / PR reviews in the morning, it
would be great if I don't have to deal with this one: #38839
<#38839>
—
Reply to this email directly, view it on GitHub
<#34902 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB35QPJOFI26MQ2REJ3Y2CENXAVCNFSM6AAAAABCFQUIYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHA4DKNBRHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@Ollyws this is ready for review, the backend changes have been deployed. Can you retest this from scratch please 🙏 |
|
Backend sends a 'deprecated version' error. Clicking on update or refreshing the page does not fix it. 🤔 Installed node-modules afresh and tried again still does not work. deprecatedVersion.mp4 |
|
Please merge I am testing after merging |
Reviewer Checklist
Screenshots/VideosAndroid: NativereconnectAndroid.mp4Android: mWeb ChromereconnectAndroidChrome.mp4iOS: mWeb SafarireconnectiOSSafari.mp4MacOS: Chrome / SafariTest 1 reportCreatedWhileOffline.mp4Test 2 switchingFromAnonAccount.mp4Test 3 toggleNetworkWithReportScreenOpen.mp4MacOS: Desktoptest1And3Desktop.mp4 |
|
@c3024 I updated with main now |
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Thanks @c3024! Merging since Marc approved in the past and Tim already approved too. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|






Details
In the context of optimizing
ReconnectToReport, we are proposing to just get rid of it since it became redundant afterreliable updateswas implemented.ReconnectToReportcommand was introduced to get the missingreportActionsafter coming back online. This was necessary becauseReconnectAppdidn't getreportActions, but now that we have "reliable updates", we are able to fetch and retrieve the missed Onyx updates which contain the missing report Actions (The missed updates are returned byReconnectApp).Fixed Issues
$ #39464
$ https://github.com/Expensify/Expensify/issues/325231
PROPOSAL:
Tests
1. Report created while offline
accountAandaccountB, and log them in on different sessionsaccountAgo offlineaccountB, create a DM chat withaccountAand send a messageaccountAgo onlineaccountAgets the DM chat withaccountBand gets all messagesaccountBaccountAreceived the last message2. Switching from being an
anonymousAccountto a normal one while looking at a public room3. Going offline/online with the report screen open
Almost the same as test 1. Report created while offline, but the
accountAgoes offline after loading the report.accountAandaccountB, and log them in on different sessionsaccountB, create a DM chat withaccountAand send a messageaccountAreceives the DM chat withaccountBand the messagesaccountA, leave the DM chat open and go offlineaccountBaccountAgo onlineaccountAreceives the last messageOffline tests
QA Steps
1. Report created while offline
accountAandaccountB, and log them in on different sessionsaccountAgo offlineaccountB, create a DM chat withaccountAand send a messageaccountAgo onlineaccountAgets the DM chat withaccountBand gets all messagesaccountBaccountAreceived the last message2. Switching from being an
anonymousAccountto a normal one while looking at a public room3. Going offline/online with the report screen open
Almost the same as test 1. Report created while offline, but the
accountAgoes offline after loading the report.accountAandaccountB, and log them in on different sessionsaccountB, create a DM chat withaccountAand send a messageaccountAreceives the DM chat withaccountBand the messagesaccountA, leave the DM chat open and go offlineaccountBaccountAgo onlineaccountAreceives the last messagePR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
1. Report created while offline
Screen.Recording.2024-01-22.at.12.21.45.PM.mov
2. Switching from being an
anonymousAccountto a normal one while looking at a public roomScreen.Recording.2024-01-22.at.4.56.34.PM.mov
3. Going offline/online with the report screen open
Screen.Recording.2024-01-22.at.5.35.03.PM.mov
MacOS: Desktop