14893: Edit message arrow up by gedu · Pull Request #15207 · Expensify/App

gedu · 2023-02-16T13:26:55Z

Details

Now ReportScreen is in charge to sort the messages and passing them down, this way the children receive the same data.

Fixed Issues

$ #14893
PROPOSAL: #14893 (comment)

Tests

Sign in (if you are already signed in, first sign out)
Open a chat with messages
Click the arrow up and should send you to the last message

Verify that no errors appear in the JS console

Offline tests

Same steps as before

QA Steps

Same steps as before

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

arrowUp_chrome.mp4

Mobile Web - Chrome

arrowUp_androidChrome.mp4

Mobile Web - Safari

arrowUp_iosSafari.mp4

Desktop

arrowUp_desktop.mp4

iOS

arrowUp_ios.mp4

Android

arrowUp_android.mp4

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

MelvinBot · 2023-02-16T13:27:03Z

@mountiny @0xmiroslav One of you needs to 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]

mountiny · 2023-02-16T13:41:47Z

    shouldComponentUpdate(nextProps, nextState) {
        if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
-            this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);
            this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions);
            return true;
        }


Is this required in here as well if we have added the logic to the ReportScreen?

Yes, the mostRecentIOUReportActionID is used just in this component and sends it down, the ReportScreen now just is taking care of the sorting, the logic for mostRecentIOUReportActionID isn't in ReportScreen

@gedu sorry I wan not clear, I meant the big chunk of the shouldComponentUpdate is now duplicated, do we need to check for the changes in these reportActionViews? If ReportScreen re-renders than this rerenders too, no?

ohhhh got it, yes it is true, my bad, I miss it after some merge conflicts, updating PR

mountiny

@0xmiroslav How is it looking with your review here?

mountiny · 2023-02-22T12:01:26Z

+    shouldComponentUpdate(nextProps) {
+        if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
+            this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);
+
+            return true;
+        }


@gedu I am not very happy with moving all of this logic up which mens amore components will have to re-render because of this. Could we keep it in the ReportActionsView and move the getSortedReportActionsForDisplay method to ReportActionsUtils instead since it is itself utils method.

Then we could get the sorted report actions in the ReportScreen, pass it down to the Report screen components and we can call the ReportActionsUtils.getSortedReportActionsForDisplay from the ReportActionsView if needed.

Is there anything I am missing?

@mountiny We needed to move up so the ReportActionsView and ReportFooter receives the same sorted list.

I could move getSortedReportActionsForDisplay into a utils file, but we should keep the sorting at the top.

About the rerender, it will be the same as before, because when ReportScreen needs to rerender when reportActions changes, was rerendering both components ReportActionsView and ReportFooter..., the first one to sort the actions again, and the Footer because of this arrow up feature among others props.

Thanks for context.

mountiny · 2023-02-23T09:16:00Z

@gedu Can you please resolve the merge conflict?

@0xmiroslav any change you could run through the checklist today, would love to move this one on!

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

0xmiroslav · 2023-02-24T11:22:52Z

-        if (this.state.newMarkerReportActionID && _.isEmpty(lodashGet(this.props.reportActions[this.state.newMarkerReportActionID], 'message[0].html'))) {
+        // If the report is unread, we want to check if the number of actions has decreased. If so, then it seems that one of them was deleted. In this case, if the deleted action was the
+        // one marking the unread point, we need to recalculate which action should be the unread marker.
+        if (ReportUtils.isUnread(this.props.report) && ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.props.reportActions.length) {


There's no filterReportActionsForDisplay function defined in ReportActionsUtils

It was removed here c351930

I will look at it thanks

yes, please remove here as well. App crashes right now when other sends message which makes isUnread true

ok, cool, I will update it

0xmiroslav · 2023-02-24T15:39:47Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

desktop.mov

iOS

Android

0xmiroslav · 2023-02-24T15:55:15Z

        Navigation.setIsReportScreenIsReady();
    }

+    shouldComponentUpdate(nextProps) {


can we return true if this.props.report changed?
I found regression (not refreshing) while adding/removing members.
i.e.

Let's say 2 members (including me) in #announce room!
-This should not show Split bill on + menu.

Login same account in another platform

Add member to this workspace on that platform

Now back to original platform and click + and this still doesn't show Split bill
This works fine on production

This happens because report.participants updated but not re-rendered

can we return true if this.props.report changed?

I am asking this because this already does in main

@0xmiroslav so this is not a regression from this PR? I think if not, we can continue here and you can file a bug report for this case

This PR causes that regression.

I am asking this because this already does in main

Here, does means re-renders because ReportScreen doesn't either extend PureComponent or use shouldComponentUpdate

Actually, I don't think we need shouldComponentUpdate in ReportScreen

@gedu For what optimization did you introduce shouldComponentUpdate in ReportScreen?

@0xmiroslav Hey, I've been looking at what you wrote and couldn't reproduce it like that, I could, following other steps, and only on web, mobile is working as expected.

Screen.Recording.2023-02-27.at.09.28.55.mp4

Let's say 2 members (including me) in #announce room!
-This should not show Split bill on + menu.

On the other hand, looking at the code, I see #announce and #admins always will show Split Bill on the + menu (if there are 2 members). This was working (above in the video showing the only case isn't working).

I added the check to the reports as you suggested and is fixing the case I found, doing some round of testing and uploading fix, thanks

@gedu you opened production site on left. please run this PR on both sides.

Discussed it here and we determined that it's ok to filter/sort in render(). I think that we should do that and remove the derived state.

mountiny · 2023-02-27T14:11:05Z

@0xmiroslav Any chance you could give this another go?

0xmiroslav · 2023-02-27T14:13:59Z

I am still not convinced why we should use shouldComponentUpdate in ReportScreen.
Regarding optimization, for which props change, is it not re-rendered?

mountiny · 2023-03-02T16:21:46Z

@luacmartins comments addressed

luacmartins · 2023-03-03T19:32:17Z

@0xmiroslav would you mind testing again since we had changes?

0xmiroslav · 2023-03-03T19:46:25Z

One minor concern is that we were still using unfiltered reportActions here, but after this PR, all reportActions are filtered.

App/src/pages/home/ReportScreen.js

Line 197 in a938832

    
           const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;

So my question is is there any possibility that fetched reportActions are empty?

0xmiroslav · 2023-03-03T19:53:12Z

+        const isArchivedRoom = ReportUtils.isArchivedRoom(this.props.report);
+        let reportClosedAction = null;
+        if (isArchivedRoom) {
+            reportClosedAction = ReportActionsUtils.getLastClosedReportAction(this.props.reportActions);


Here, this.props.reportActions are already filtered, so closed report actions won't exist.
This will cause regression again what @mountiny reported.

Thats correct, I think we should split this into two methods, we need the closed actions in it so I think we should sort it, and then remove the closed actions once we pass that to the ReporActionsView.

0xmiroslav · 2023-03-03T19:55:20Z

We should export both filtered and unfiltered reportActions from selector

mountiny · 2023-03-06T10:11:46Z

@gedu will update this so the footer is using a selector to pull the correct closed message

… the closeAction

gedu · 2023-03-06T11:17:34Z

Now ArchivedReportFooter using the selector

luacmartins

LGTM and tests well. @gedu would you mind adding test steps for the archived footer scenario?

gedu · 2023-03-06T13:02:14Z

LGTM and tests well. @gedu would you mind adding test steps for the archived footer scenario?

I would paste the steps given to me by @mountiny, could you help me with this? if I miss something

you would have to create a workspace
invite two people there
remove one person from the workspace
then merge that removed member to the account who is still a member

mountiny

Tested well

@luacmartins the archived reason will be confirmed/ tested along with this PR #15474

OSBotify · 2023-03-06T13:39:21Z

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

0xmiroslav · 2023-03-06T14:52:17Z

                        {isArchivedRoom && (
                            <ArchivedReportFooter
-                                reportClosedAction={reportClosedAction}
+                                reportClosedAction={this.props.reportClosedAction}


We don't need this anymore

Thanks, created a PR here #15687

OSBotify · 2023-03-06T16:43:20Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.79-0 🚀

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

OSBotify · 2023-03-07T17:20:27Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.79-0 🚀

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

0xmiroslav · 2023-03-24T12:39:51Z

Minor issue we missed:

App/src/pages/home/ReportScreen.js

Line 89 in 10d3d52

reportActions: {},

- reportActions: {}, 
+ reportActions: [],

luacmartins · 2023-03-24T13:16:35Z

Haha I raised this question for the usage in another page. #15207 (comment)

mountiny · 2023-04-12T12:19:19Z

+        reportClosedAction: {
+            key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
+            canEvict: false,
+            selector: ReportActionsUtils.getLastClosedReportAction,


This PR caused this regression #16324 because the getLastClosedReportAction has been returning undefined which in turn did not get swapped for the defaultProps

gedu added 2 commits February 15, 2023 14:51

Moving up the reportActions sorting

4ef5645

Merge branch 'main' into 14893/edit_message_arrow_up

0c821ff

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

gedu requested a review from a team as a code owner February 16, 2023 13:26

melvin-bot Bot requested review from 0xmiroslav and mountiny and removed request for a team February 16, 2023 13:27

mountiny assigned gedu Feb 16, 2023

mountiny reviewed Feb 16, 2023

View reviewed changes

gedu added 2 commits February 16, 2023 17:22

Updated props and removed shouldComponentUpdate

0d1f768

Fixed isPinned

0d91a37

gedu requested review from mountiny and removed request for 0xmiroslav February 16, 2023 21:56

mountiny requested a review from 0xmiroslav February 16, 2023 22:27

mountiny previously approved these changes Feb 22, 2023

View reviewed changes

gedu added 2 commits February 23, 2023 10:33

Merge branch 'main' into 14893/edit_message_arrow_up

17dc211

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

replace local function for utils

f54539a

gedu dismissed mountiny’s stale review via f54539a February 23, 2023 09:57

mountiny previously approved these changes Feb 23, 2023

View reviewed changes

0xmiroslav reviewed Feb 24, 2023

View reviewed changes

gedu added 2 commits February 24, 2023 14:00

Merge branch 'main' into 14893/edit_message_arrow_up

6091178

removed call to unexisting function

e1834f1

gedu dismissed mountiny’s stale review via e1834f1 February 24, 2023 13:23

0xmiroslav reviewed Feb 24, 2023

View reviewed changes

added participants check to rerender the report screen component

5ec7c39

mountiny requested a review from 0xmiroslav February 27, 2023 14:10

Changed the sorting to be in Onyx select

2b1a943

gedu dismissed stale reviews from mountiny and 0xmiroslav via 2b1a943 March 3, 2023 16:43

luacmartins previously approved these changes Mar 3, 2023

View reviewed changes

0xmiroslav reviewed Mar 3, 2023

View reviewed changes

gedu added 2 commits March 6, 2023 12:09

ArchivedReportFooter listening directly the reportsAction and getting…

42ce483

… the closeAction

Merge branch 'main' into 14893/edit_message_arrow_up

6784b28

gedu dismissed luacmartins’s stale review via 6784b28 March 6, 2023 11:11

Updated prop type from reportActions

ab82acd

luacmartins approved these changes Mar 6, 2023

View reviewed changes

mountiny approved these changes Mar 6, 2023

View reviewed changes

mountiny merged commit 593be0f into Expensify:main Mar 6, 2023

0xmiroslav reviewed Mar 6, 2023

View reviewed changes

OSBotify mentioned this pull request Mar 6, 2023

Deploy Checklist: New Expensify 2023-03-06 #15684

Closed

17 tasks

mountiny mentioned this pull request Mar 6, 2023

[NoQA] Remove unnecessary props in ArchivedReportFooter #15687

Merged

55 tasks

MelvinBot mentioned this pull request Mar 7, 2023

[HOLD for payment 2023-03-14] Mac Safari - Chat - clicking arrow up in composer box does not edit the last sent message #14893

Closed

6 tasks

mountiny reviewed Apr 12, 2023

View reviewed changes

sobitneupane mentioned this pull request Apr 13, 2023

[HOLD for payment 2023-04-05] [$1000] The archived Report Footer is missing #16324

Closed

6 tasks

Uh oh!

Conversation

gedu commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MelvinBot commented Feb 16, 2023

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!

mountiny left a comment

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!

mountiny commented Feb 23, 2023

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!

0xmiroslav commented Feb 24, 2023

Reviewer Checklist

Screenshots/Videos

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!

0xmiroslav Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gedu Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mountiny commented Feb 27, 2023

Uh oh!

0xmiroslav commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mountiny commented Mar 2, 2023

Uh oh!

luacmartins commented Mar 3, 2023

Uh oh!

gedu commented Feb 16, 2023 •

edited

Loading

0xmiroslav Feb 24, 2023 •

edited

Loading

gedu Feb 27, 2023 •

edited

Loading

0xmiroslav commented Feb 27, 2023 •

edited

Loading

0xmiroslav Mar 3, 2023 •

edited

Loading