Refactor Get_ReportHistory #9613

sketchydroide · 2022-06-28T21:25:06Z

CC @marcaaron - let me know if I'm going in the right direction

Details

Fixed Issues

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

Tests

For this test you need a chat with over 100 actions, if more than 150 even better.
Also before starting make sure you have the latest changes from Web-Expensify pulled

Open the app, and go to the chat with more than 100 actions.
Verify that is opens and you can see the first most recent actions.
Scroll up until you see the loading spinner
Verify that after a while you get the next set of actions
Repeat until you see the start of the chat.
Verify that it work every time.

[] Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

For this test you need a chat with over 100 actions, if more than 150 even better.

Open the app, and go to the chat with more than 100 actions.
Verify that is opens and you can see the first most recent actions.
Scroll up until you see the loading spinner
Verify that after a while you get the next set of actions
Repeat until you see the start of the chat.
Verify that it work every time.

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

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

sketchydroide · 2022-07-12T16:20:59Z

@marcaaron if you have some time I would appreciate a first glance at this one as well, I think I got the gist of it, but still not sure I did it right.

marcaaron

Good start, but a lot of changes left to make. Please let me know if there are specific questions!

marcaaron · 2022-07-12T20:42:19Z

src/libs/actions/Report.js

+            _.each(reportIDsToFetchActions, (reportID) => {
+                const offset = ReportActions.dangerouslyGetReportActionsMaxSequenceNumber(reportID, false);
+                readOldestAction(reportID, offset);
+            });


Most of these changes need to be removed. I'll try to break down everything and explain why it's incorrect...

Actions need to be tied to user actions

We should not be calling readOldestAction() as a direct replacement for fetchActions()

readOldestAction() would be called when we... read the oldest action and no other time

Reading the oldest action means that we are scrolling up on the report and new chat messages are loading

So what "action" should we use for this existing code?

None. IIRC we decided to remove this.

Why does it exist?

It's an optimization (possibly not needed) used by the front end to preload a bunch of chat messages for various recent reports that we know are "missing actions" based on the locally stored actions we already have.

Why are we using a timer? If we keep the code should it be removed?

We run this code at a significant delay to avoid performance issues from loading too many actions at once. If we are not removing the code right now we should keep the timer.

What should we do for now?

I think just remove the code entirely is fine. If we want to "preload" actions we can think of a better design and have it hook into the big "App Start" event. Chatted about this for a bit with @luacmartins in this doc section.

I agree with this, feel like once we finish the refactor, we can figure if this is still needed and if that is the case come up with a better way of doing this.

Is this one of the reasons we sometimes have a white loading page?

Cool. Let's remove it and create a follow up issue to investigate.

I'm not sure about the white loading page and haven't experienced that myself - could be related or not.

marcaaron · 2022-07-12T20:43:37Z

src/libs/actions/Report.js

+ */
+function readOldestAction(reportID, offset = 0) {
+    const hasFinishedLoadingData = {
+        onyxMethod: 'merge',


Suggested change

onyxMethod: 'merge',

onyxMethod: CONST.ONYX.METHOD.MERGE,

marcaaron · 2022-07-12T20:46:07Z

src/libs/actions/Report.js

+ * @param {Number} offset
+ */
+function readOldestAction(reportID, offset = 0) {
+    const hasFinishedLoadingData = {


This name sounds a bit like a boolean (has, should, is, etc) and we should update to defaultReportActionsLoadingState or something.

for some reason at some point this became a boolean in my head, really don't know why, when is so obviously not.

marcaaron · 2022-07-12T20:46:20Z

src/libs/actions/Report.js

+        },
+    };
+
+    Onyx.set(ONYXKEYS.IS_LOADING_REPORT_ACTIONS, true);


Suggested change

Onyx.set(ONYXKEYS.IS_LOADING_REPORT_ACTIONS, true);

marcaaron · 2022-07-12T20:47:25Z

src/libs/actions/Report.js

+            offset,
+        },
+        {
+            optimisticData: [],


Suggested change

optimisticData: [],

optimisticData: [{

onyxMethod: CONST.ONYX.METHOD.MERGE,

key: `${ONYXKEYS.IS_LOADING_REPORT_ACTIONS}${reportID}`,

value: {isLoading: true},

}],

marcaaron · 2022-07-12T20:51:05Z

src/libs/actions/Report.js

+function readOldestAction(reportID, offset = 0) {
+    const hasFinishedLoadingData = {
+        onyxMethod: 'merge',
+        key: `${ONYXKEYS.IS_LOADING_REPORT_ACTIONS}${reportID}`,


Looks kind of right, but there's a more correct way to do this.

If we want to start tracking the loading status for each report in this way we would want to create a "collection" key. Explained in the features of Onyx here, but let me know if you have any questions about it.

So I would need to create a key in collections, and use that one instead? is that correct?

Yep you got it.

marcaaron · 2022-07-12T20:54:01Z

src/pages/home/report/ReportActionsView.js

@@ -300,7 +300,7 @@ class ReportActionsView extends React.Component {
        // Retrieve the next REPORT.ACTIONS.LIMIT sized page of comments, unless we're near the beginning, in which
        // case just get everything starting from 0.
        const offset = Math.max(minSequenceNumber - CONST.REPORT.ACTIONS.LIMIT, 0);
-        Report.fetchActionsWithLoadingState(this.props.reportID, offset);
+        Report.readOldestAction(this.props.reportID, offset);


This is correct because we are passing the "oldest action" to the API.

marcaaron · 2022-07-12T20:54:03Z

src/pages/home/report/ReportActionsView.js

@@ -275,7 +275,7 @@ class ReportActionsView extends React.Component {
    }

    fetchData() {
-        Report.fetchActions(this.props.reportID);
+        Report.readOldestAction(this.props.reportID);


There are a few more changes that need to happen in this file, but this one is incorrect as we are not reading the oldest action here.

I think I got it now, we need to call ReadOldestAction in the places we are calling fetchActions and passing an oldestAction

Almost... we need to call ReadOldestAction when the user "reads the oldest action". Which is just another way of saying "pagination".

yes, that makes sense.
I think we only have one place at the moment though

marcaaron · 2022-07-12T20:55:10Z

src/pages/home/report/ReportActionsView.js

@@ -300,7 +300,7 @@ class ReportActionsView extends React.Component {
        // Retrieve the next REPORT.ACTIONS.LIMIT sized page of comments, unless we're near the beginning, in which
        // case just get everything starting from 0.
        const offset = Math.max(minSequenceNumber - CONST.REPORT.ACTIONS.LIMIT, 0);


maybe it would help to rename this variable oldestActionSequenceNumber

sketchydroide · 2022-07-13T14:53:03Z

src/libs/actions/Report.js

+
+    Onyx.set(ONYXKEYS.IS_LOADING_REPORT_ACTIONS, true);
+
+    API.write('ReadOldestAction',


I set this as Write, but I think it should be read right?

Oh hmm yes since we are not writing anything in the server I think that's correct.

src/libs/actions/Report.js

marcaaron · 2022-07-20T16:43:04Z

src/libs/actions/Report.js

+                {
+                    onyxMethod: CONST.ONYX.METHOD.MERGE,
+                    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
+                }


Suggested change

}

},

src/pages/home/report/ReportActionsView.js

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

src/libs/actions/Report.js

remove successData for actions

marcaaron · 2022-07-21T01:52:53Z

Changes look good, but we are still missing...

Test steps
package-lock.json changes that got merged in need to be removed as they should not be committed

Also we'll need a new Web-E PR with these changes -> https://github.com/Expensify/Web-Expensify/compare/marcaaron-stdClassSolution?expand=1 to fix the weird issue described in this Slack thread

melvin-bot · 2022-07-21T13:14:10Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

melvin-bot · 2022-07-21T13:14:24Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

sketchydroide · 2022-07-21T13:15:12Z

this needs to hold until https://github.com/Expensify/Web-Expensify/pull/34374 is CP/merged, but it's ready otherwise

melvin-bot · 2022-07-21T13:16:42Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

Beamanator

Overall looking real good! Just leaving a few tiny recommended changes

Beamanator · 2022-07-21T13:20:46Z

package-lock.json

-      "integrity": "sha1-BqZgTWpV1L9BqaR9mHLXp42jHnM="
+      "integrity": "sha512-xpkr6sCDIYTPqzvjG8M3ncw1YOTaloWZOyrUmicoEifBEKzQzt+ooUpRpQ/AbOoJfO/p2ZKiyp79qHThzJDulQ=="


Just wondering, are we ok with committing these types of changes?

Mainly asking b/c it was recommended I don't commit these changes here: #9635 (comment)

And I think it makes sense to all agree or disagree what the best plan is 😅

cc @Julesssss

As @sketchydroide is on an M1 Mac, I think it's okay right? But yeah we should clarify this rule as the changes do look similar to yours @Beamanator.

I think the idea is that anyone on an M1 Mac should not see a diff when running npm i

I think this was commited by mistake, and @marcaaron already asked for the commit of this file to be reverted

I have reset this

Beamanator · 2022-07-21T13:26:25Z

src/libs/actions/Report.js

@@ -1160,6 +1095,37 @@ function openReport(reportID) {
        });
 }

+/**
+ * Gets the actions from the oldest unread action.


Can we clarify: Does this get actions from the oldest unread action and older? or and newer?

Not sure if my proposed addition is 100% clear even, but just from reading this I think ReadOldestAction is singular so we're only getting 1 action. If that's true, great - let's update this comment so it is clear we're only getting 1 action. Otherwise can we think about making the command ReadOldestActions and clarifying if we're getting oldest unread action & newer or & older?

so this reads in to the older actions/messages, basically when you scroll up, so older and older

not sure about the plural part though. @marcaaron do you have thoughts on that?
We would need to change Web as well.

We (the user) are reading the "oldest action". This is synonymous with scrolling up to the oldest action stored locally - a.k.a. triggering a call to paginate the actions and load the next set. It should not be plural and we are not "only getting 1 action" we are getting the next set of historical actions.

marcaaron

LGTM!

Both comments left have been resolved.

OSBotify · 2022-07-21T17:00:37Z

✋ 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 · 2022-07-25T18:03:39Z

🚀 Deployed to staging by @marcaaron in version: 1.1.86-0 🚀

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

OSBotify · 2022-08-01T22:11:27Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

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

start

f2f4740

sketchydroide requested a review from marcaaron as a code owner June 28, 2022 21:25

sketchydroide self-assigned this Jun 28, 2022

sketchydroide added 3 commits June 29, 2022 15:53

remove unneeded import

963fcc9

updating OpenReport

9777a88

Merge branch 'maxence-rrulr' into afonseca_rrgh

9bcfda0

sketchydroide requested a review from a team as a code owner June 30, 2022 14:30

melvin-bot bot requested review from Beamanator and removed request for a team June 30, 2022 14:31

sketchydroide added 14 commits June 30, 2022 15:32

duplicated OpenReport

ff8bb0f

updating

bdc16db

adding is loading

926d388

improving on is loading

1f6fbe4

I don't think there is optimistic data in this case, it just loads

0a0cf9a

I was editing the wrong method

3fffea6

remove offset, I'm not sure it is needed

01269a3

Merge branch 'main' into afonseca_rrgh

72e896e

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

no message

55835a6

export method

ef81dbb

replace fetchData

7ba25f1

loadMoreChats

c6b0ccc

linter requests

4e01c35

on the timer part as well

4a3bcd3

marcaaron requested changes Jul 12, 2022

View reviewed changes

sketchydroide added 2 commits July 13, 2022 15:46

removing the old timer section as it should no longer be needed

38e73a3

also removing the timers import

ff7cd6b

sketchydroide commented Jul 13, 2022

View reviewed changes

sketchydroide added 2 commits July 13, 2022 16:27

renaming offset to oldestActionSequenceNumber

0d66a1f

was the same but forgot about it

dd5d4b6

changing the ONYX key for the loading

92ddb4a

marcaaron reviewed Jul 20, 2022

View reviewed changes

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

marcaaron changed the title ~~[WIP] Refactor Get_ReportHistory~~ Refactor Get_ReportHistory Jul 20, 2022

sketchydroide added 5 commits July 20, 2022 22:36

offset is always a number

17769b9

correcting the onyx key

a3592e2

style

048751d

Merge branch 'main' into afonseca_rrgh

00e0dba

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

commit new package

700792a

marcaaron reviewed Jul 20, 2022

View reviewed changes

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

remove object for is loading,

92c619e

remove successData for actions

marcaaron requested review from marcaaron and removed request for Beamanator July 21, 2022 02:06

Beamanator previously requested changes Jul 21, 2022

View reviewed changes

sketchydroide added 3 commits July 21, 2022 15:54

reseting the file

b9e84f6

version is wrong?

ec6c141

Updated the description a bit

a016a61

marcaaron approved these changes Jul 21, 2022

View reviewed changes

marcaaron merged commit d48e147 into main Jul 21, 2022

marcaaron deleted the afonseca_rrgh branch July 21, 2022 16:54

OSBotify mentioned this pull request Jul 25, 2022

Deploy Checklist: New Expensify 2022-07-25 #10077

Closed

92 tasks


		Onyx.set(ONYXKEYS.IS_LOADING_REPORT_ACTIONS, true);

		API.write('ReadOldestAction',

		"integrity": "sha1-BqZgTWpV1L9BqaR9mHLXp42jHnM="
		"integrity": "sha512-xpkr6sCDIYTPqzvjG8M3ncw1YOTaloWZOyrUmicoEifBEKzQzt+ooUpRpQ/AbOoJfO/p2ZKiyp79qHThzJDulQ=="

Refactor Get_ReportHistory #9613

Refactor Get_ReportHistory #9613

Conversation

sketchydroide commented Jun 28, 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

sketchydroide commented Jul 12, 2022

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jul 21, 2022

melvin-bot bot commented Jul 21, 2022

melvin-bot bot commented Jul 21, 2022

sketchydroide commented Jul 21, 2022

melvin-bot bot commented Jul 21, 2022

Beamanator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 21, 2022

OSBotify commented Jul 25, 2022

OSBotify commented Aug 1, 2022

sketchydroide commented Jun 28, 2022 •

edited

Loading