Refactor OpenReport API call for Report GetHistory by sketchydroide · Pull Request #10164 · Expensify/App

sketchydroide · 2022-08-01T15:53:19Z

CC @marcaaron @Beamanator

Fixed Issues

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

Tests

This needs to be tested along with the Web PR https://github.com/Expensify/Web-Expensify/pull/34429 (now merged)

Make sure to have a chat with a few messages, I would say around the 100, that you already have opened before.
Open the app and head to the chat in question
Verify that it loads the report and shows the messages. Verify as well that no loader is shown

Make sure to have a third chat with a less than 50 messages, I would say around the 30, that you already have opened before.
Open the app and head to the chat in question
Verify that it loads the report and shows the messages. Verify as well that no loader is shown

Make sure to have a second chat with around the same amount of messages, that you already have NOT opened before.
Open the app and head to the chat in question
Verify a loader is shown
Verify that after the report is loaded the report appears and shows the messages.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron · 2022-08-01T17:03:23Z

+            unreadActionCount: 0,
+        }
+    };
+    // only show loading state if the report has not been open yet


This seems like something the view should handle. We can still set the loading state. The view will decide to use it or not based on whether it has initial report actions to display.

tgolen

@sketchydroide Would you mind adding a better title and description to this PR? I am looking at it with very little context from the original issue and I'm confused about what exactly this PR is meant to be doing. The issue is long, it mentions several different pieces, so I'm missing the context of the changes. It also needs QA tests to be added.

sketchydroide · 2022-08-01T19:06:48Z

Sorry @tgolen this is still WIP, I always forget that this Repo adds a reviewer as soon as I create it

I will add all the things, also I'll improve the title

tgolen · 2022-08-01T19:21:48Z

OK, thanks! Creating the PR in draft mode is a good practice where you can avoid that issue. 👍

# Conflicts: # src/pages/home/ReportScreen.js # src/pages/home/report/ReportActionsView.js

…en will

sketchydroide · 2022-08-03T11:44:49Z

     */
    shouldShowLoader() {
-        return !getReportID(this.props.route) || (_.isEmpty(this.props.reportActions) && this.props.isLoadingInitialReportActions);
+        const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.isLoadingReportActions;


@marc suggested this and I agreed, this makes it easier to understand the purpose of the check

Can you please update the method description in the JSDocs to be more clear? It is answering "what", but not "why"

Shouldn't this really be more like isLoadingReportActions? It doesn't look like it has anything to do with "initial" actions or not. I'm not even sure what "initial" implies.

"initial" means you have no reportActions at all to display vs. having some reportActions to display, but still in the process of loading the next set of actions ones. There are two different animations displayed based on which situation we have.

are you happy with that @tgolen or do you want me to rename the var?

I really like Marc's explanation. Could you add that as a code comment to help explain it to others who will see this? If the comment is added, then I think the name is OK then.

I added a shorter version of Marcs answer, that I feel makes more sense to read, let me know if that works.

marcaaron · 2022-08-03T16:09:01Z

Code is looking good. So, we just need QA tests added now?

# Conflicts: # src/pages/home/ReportScreen.js

tgolen · 2022-08-15T15:49:19Z

-    Onyx.set(`${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${reportID}`, true);
-    fetchActions(reportID)
-        .finally(() => Onyx.set(`${ONYXKEYS.COLLECTION.IS_LOADING_INITIAL_REPORT_ACTIONS}${reportID}`, false));
+    fetchActions(reportID);


Now that this is all this method does, could you please just remove the method fetchInitialActions() and call fetchActions() directly?

Just a note that it's going to be removed entirely in the next round of PRs.

I can still replace it though, it's not that much work I think

tgolen · 2022-08-15T15:50:48Z

 */
 function readOldestAction(reportID, oldestActionSequenceNumber) {
-    API.read('ReadOldestAction',
+    API.write('ReadOldestAction',


NAB hahaha... we're writeing an action called Read? That doesn't make a whole lot of sense. The API command should probably be something more like MarkOldestActionAsRead

If this is a NAB - I prefer the current name and think it's OK. The alternative suggestion confuses me a bit because I am imagining the user manually marking something (and this is something that happens as a side-effect of the user reading something with their eyes and not "reading data from the server").

(and this is something that happens as a side-effect of the user reading something with their eyes and not "reading data from the server").

+1 - Since the action is the "reading" I think it's fine, but maybe we could go with ViewOldestAction so we don't use the term "read"

😅 I think this is still a read, and I just changed the wrong method, I meant to change OpenReport to Write

tgolen · 2022-08-15T15:53:13Z

    withDrawerState,
    withLocalize,
    withNetwork(),
-    withOnyx({


How does the component get this.props.report now without using withOnyx()?

from the parent component here:

App/src/pages/home/ReportScreen.js

Lines 242 to 245 in 972b835

<ReportActionsView

reportID={reportID}

reportActions={this.props.reportActions}

report={this.props.report}

Oh, Thanks! I don't think we should pass both the reportID and report. Those are redundant. Let's just pass report and get the reportID off that.

I can do that, but reportID at the moment is required not sure if we should make the report required as well, not sure if we should have a default value for reportID

This sounds like an easy change and a good thing to look into. I also think it would be OK to do in a follow up and not add additional refactoring scope to these changes. I don't immediately see a clear reason for passing the reportID and report as separate props, but I haven't looked into it deeply enough to say there isn't one.

I think we should make report the required prop then, but as always, make sure you take a look at the code and understand it first before making a change like that.

I won't fight tooth-and-nail about it being done in this PR, but I also think this is a refactor PR and I am not too concerned about scope for something small like this.

just made the change and tested it after, seems to work great, let me know if you are happy with that @tgolen

marcaaron · 2022-08-15T17:30:30Z

@sketchydroide I think this still needs to be on HOLD as the Web-E PR has been merged but not yet deployed.

marcaaron · 2022-08-15T17:32:03Z

 function openReport(reportID) {
-    const sequenceNumber = getMaxSequenceNumber(reportID);
-    API.write('OpenReport',
+    API.read('OpenReport',


Please make this a write()

marcaaron

LGTM! Thanks for the changes. We still need to wait for this Web-Expensify PR to go to production before merging.

marcaaron · 2022-08-16T20:12:58Z

Web-E PR is now on production.

tgolen

Looks great now! Just this one last small change I noticed

Co-authored-by: Tim Golen <tgolen@gmail.com>

sketchydroide · 2022-08-18T09:38:47Z

the tests seems to be failing but it does not seem related to the last commit

sketchydroide · 2022-08-18T09:52:55Z

merging main seems o have fixed it

sketchydroide · 2022-08-18T11:35:42Z

@Beamanator is OOO, but his comments were addresed, so I think we are good

OSBotify · 2022-08-18T15:35:18Z

✋ 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-08-23T19:11:56Z

🚀 Deployed to staging by @tgolen in version: 1.1.89-0 🚀

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

sketchydroide added 3 commits July 28, 2022 17:08

removing fetch data

713e1e0

sequence number is no longer used

de85a84

reorganizing this a bit

1c7bce4

sketchydroide requested a review from a team as a code owner August 1, 2022 15:53

sketchydroide self-assigned this Aug 1, 2022

melvin-bot Bot requested review from tgolen and removed request for a team August 1, 2022 15:53

marcaaron requested changes Aug 1, 2022

View reviewed changes

tgolen requested changes Aug 1, 2022

View reviewed changes

sketchydroide changed the title ~~Report GetHistory OpenReport~~ [WIP] Report GetHistory OpenReport Aug 1, 2022

sketchydroide changed the title ~~[WIP] Report GetHistory OpenReport~~ [WIP] Refactor OpenReport API call for Report GetHistory Aug 1, 2022

sketchydroide marked this pull request as draft August 2, 2022 12:43

sketchydroide added 8 commits August 2, 2022 14:45

removing updateNewMarkerAndMarkReadOnce

8408423

the view will decide on this

7f11c94

using the isLoadingReportData only if we have no reportActions

b301b28

Merge branch 'main' into afonseca_get_history_open_report

8260661

# Conflicts: # src/pages/home/ReportScreen.js # src/pages/home/report/ReportActionsView.js

the actions does not know if it's the inital loading or not, the scre…

b654d97

…en will

removing isLoadingReportData

5e37db3

adding the fetchData back for now until we do the reconnect action

2705f6e

linter

2763fc4

sketchydroide commented Aug 3, 2022

View reviewed changes

Comment thread src/pages/home/ReportScreen.js

sketchydroide commented Aug 3, 2022

View reviewed changes

Merge branch 'main' into afonseca_get_history_open_report

9b1b0b6

# Conflicts: # src/pages/home/ReportScreen.js

sketchydroide marked this pull request as ready for review August 9, 2022 11:42

Merge branch 'main' into afonseca_get_history_open_report

3dc5dc6

# Conflicts: # src/pages/home/ReportScreen.js

sketchydroide changed the title ~~[WIP] Refactor OpenReport API call for Report GetHistory~~ Refactor OpenReport API call for Report GetHistory Aug 9, 2022

sketchydroide requested a review from marcaaron August 9, 2022 11:45

tgolen requested changes Aug 15, 2022

View reviewed changes

marcaaron changed the title ~~Refactor OpenReport API call for Report GetHistory~~ [HOLD Web-E Production Deploy 34429] Refactor OpenReport API call for Report GetHistory Aug 15, 2022

marcaaron requested changes Aug 15, 2022

View reviewed changes

sketchydroide added 3 commits August 16, 2022 15:57

Merge branch 'main' into afonseca_get_history_open_report

0be9f97

wrong method changed

852e636

remove fetchActions

599ca8b

marcaaron previously approved these changes Aug 16, 2022

View reviewed changes

marcaaron changed the title ~~[HOLD Web-E Production Deploy 34429] Refactor OpenReport API call for Report GetHistory~~ Refactor OpenReport API call for Report GetHistory Aug 16, 2022

marcaaron requested a review from tgolen August 16, 2022 20:12

setting the report as required

2d7de46

sketchydroide dismissed marcaaron’s stale review via 2d7de46 August 17, 2022 13:47

Merge branch 'main' into afonseca_get_history_open_report

8954f03

sketchydroide requested a review from marcaaron August 17, 2022 15:31

tgolen requested changes Aug 17, 2022

View reviewed changes

Comment thread src/pages/home/report/ReportActionsView.js Outdated

Update src/pages/home/report/ReportActionsView.js

8ebc4f7

Co-authored-by: Tim Golen <tgolen@gmail.com>

marcaaron requested a review from tgolen August 17, 2022 17:28

marcaaron approved these changes Aug 17, 2022

View reviewed changes

tgolen approved these changes Aug 17, 2022

View reviewed changes

Merge branch 'main' into afonseca_get_history_open_report

d2695b2

tgolen merged commit a4c70a6 into main Aug 18, 2022

tgolen deleted the afonseca_get_history_open_report branch August 18, 2022 15:29

OSBotify mentioned this pull request Aug 23, 2022

Deploy Checklist: New Expensify 2022-08-23 #10498

Closed

48 tasks

roryabraham mentioned this pull request Jan 11, 2024

HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269

Merged

50 tasks

	<ReportActionsView
	reportID={reportID}
	reportActions={this.props.reportActions}
	report={this.props.report}

Conversation

sketchydroide commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

sketchydroide commented Aug 1, 2022

Uh oh!

tgolen commented Aug 1, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

sketchydroide commented Aug 1, 2022 •

edited

Loading