Refactor the componentDidUpdate method for ReportActionCompose #4768

mountiny · 2021-08-20T17:51:09Z

Details

To resolve a StagingDeployBlocker, we have introduced some changes which need some refactoring. How to refactor has been discussed and decided in this thread here. Look there for more details.

Fixed Issues

❌

Tests

Just repeat the action performed steps from this issue #4756 and make sure it all works as expected.

QA Steps

Navigate to a conversation
Write something in compose box
Use ctrl+k command
Select another conversation
Verify that nothing is displayed in compose box if the selected conversation didn't have a draft
Use ctrl+k command to switch back to the first conversation.
Verify that the draft message in the compose box is shown.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny · 2021-08-20T18:10:46Z

@marcaaron My theory based on what I have observed when testing it is that the reportID is fetched from route here and passed down to the ReportActionCompose. However, the report which holds the comment we need is not updated at that same time as the route changes. In console I could see ~2-3 calls of componentDidUpdate in ReportActionCompose with the previous report while the reportID has already been updated (as a consequence the draft comment from previous message is populated into the input).

Isn't it that the reportID fetched from route is from the Navigation whereas report is from Onyx which can take slightly longer to switch?

marcaaron · 2021-08-23T18:53:28Z

Isn't it that the reportID fetched from route is from the Navigation whereas report is from Onyx which can take slightly longer to switch?

Yes that makes sense to me. It should be like 1 tick behind because Onyx is asynchronous. Is that what you're asking?

marcaaron · 2021-08-23T18:54:23Z

src/pages/home/report/ReportActionCompose.js

-            this.textInput.setNativeProps({text: this.props.comment});
+        // If we switch the reports, make sure to update the composer comment
+        const reportChanged = this.props.report.reportID !== prevProps.report.reportID;
+        if (reportChanged) {


This should use an early return like

if (!reportChanged) { return; } this.updateComment(this.props.comment);

Also reportChanged does not need a variable just put the comparison into the if case?

Updated the componentDidUpdate accordingly. Since the same logic is also used in ReportScreen, I have updated it there too to follow the same pattern or early return for consistency :)

mountiny · 2021-08-25T17:44:00Z

Sorry for the ping, Bondy, that was some bug, I have not assigned PullerBear again :/

It should be like 1 tick behind because Onyx is asynchronous. Is that what you're asking?

@marcaaron Yes, that is what I meant. The Onyx report change is always a bit behind the route change.

bondydaa · 2021-08-25T17:46:48Z

src/pages/home/report/ReportActionCompose.js

-        // so we need to update the comment manually.
-        if (prevProps.comment !== this.props.comment) {
-            this.textInput.setNativeProps({text: this.props.comment});
+        // If we switch the reports, make sure to update the composer comment; otherwise return early


NAB comments should explain why not what we are doing in the code. so I would suggest removing the otherwise return early and instead a because... explaining why we need to update the composer comment:

Suggested change

// If we switch the reports, make sure to update the composer comment; otherwise return early

// If we switch the reports, make sure to update the composer comment because [we don't want to leave old data around] [we shouldn't copy what a user had types in one place to another]

Gotcha, I was not sure about that comment actually, thank you for bringing it up! I will update it just now.

marcaaron · 2021-08-25T18:12:35Z

src/pages/home/ReportScreen.js

-            this.storeCurrentlyViewedReport();
+        // If we do not switch between reports, return early
+        if (this.props.route.params.reportID === prevProps.route.params.reportID) {
+            return;


NAB, I know this was probably added since you noticed that the style doesn't match what I asked for in another comment. But since this change doesn't have anything to do with the current change I wonder if a better way to handle this would be to:

Create a new issue calling out there there are places where early returns are not used

Follow up to audit existing if blocks to see which are not using early returns and fix them

I'm not asking you to do it since we don't really have any rules or conventions about this. Some feel like it's fine to fix things that are wrong when you see them and others (like me) prefer to keep only material changes in PRs so that context is preserved and no one needs to ask why something was done. I don't think there is a "right way", so mostly just sharing this perspective.

I really appreciate you sharing this with me. This helps me a lot 🙇 I do agree it is not ideal to add changes and fixes to PR where it might be unrelated and uncalled for. I haven't realized it now, but I will definitely try to keep this on mind for future! It will make for clearer and easier to follow issues and PR for sure 🙌

I agree with you on the proposed process how to handle such a change, although as you stated, I think before creating such an investigation issue, we should make sure we update the Styling guidelines accordingly. That way if we fix this bad smell throughout the codebase we will have lesser chances of introducing it again.

Do you think this is work of being added to the styling guide, or it is too nitty-gritty 👀 ?

FWIW how I view "adjacent style changes" (completely just made that up) like this are to weigh the "pros vs cons" of how bad it might be to have to revert or quickly fix if this breaks something unexpectedly.

I agree this seems like a pretty low risk change so probably fine to leave. But this mean you should be on the look out for any messages in #api-errors, #qa, or new GHs from Success that mention anything about weirdness about the report pages.

If you're not willing to be on "high alert" and fix something like that then I'd say only make the change you intended to make and leave the "adjacent style change" alone and spin up a new GH to fix it so that it's properly tested in it's own PR.

I don't think there is a need for a style guide rule or anything since there will be nuances to every PR where sometimes it makes sense to change all "adjacent style changes" and other times it might not make the most sense.

@bondydaa Thank you very much for your 2 cents here! Glad to have another perspective on this 💯

I will keep an eye on those channels! Thanks for the heads up. I think in this case there should not be high risk 🤞 since the logic stays the same, but I totally see what you mean. Even with minor styling changes, we should make sure all the flows are working and no edge case has been omitted 👍

Regarding the styling guide, I see your point. Eventually, informing more and more engineers about this style approach, we might get more of them using it in their code and PR reviews consequently making it standard the organic way.

marcaaron · 2021-08-25T18:14:27Z

src/pages/home/ReportScreen.js

-        if (reportChanged) {
-            this.prepareTransition();
-            this.storeCurrentlyViewedReport();
+        // If we do not switch between reports, return early


Similar comment to @bondydaa but if a comment ends up just describing what the code is doing (or is very obvious) then we probably don't need the comment at all.

Good point, in this case it is actually bit clearer why we check if the reports have changed. I will omit the comment here 👍

mountiny · 2021-08-25T21:53:58Z

@bondydaa @marcaaron Thank you very much for your tips, advice and best practices on this matter. I have pushed some updates, I tried to give a better yet short description of why we update the comment if reportIDs change and I have removed the comment in ReportScreen since it is self-explanatory what the code does there.

Thank you for your reviews 🙌

OSBotify · 2021-08-30T19:44:33Z

✋ 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 · 2021-08-31T00:26:39Z

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

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

isagoico · 2021-08-31T15:30:08Z

@mountiny @marcaaron Are we good to check this PR off the list? (I'm guessing it's NoQA but wanted to double check)

roryabraham · 2021-09-01T00:24:17Z

@isagoico I filled in the QA steps here. Can you please perform QA on this?

isagoico · 2021-09-01T00:36:56Z

Just tested it and it was a pass 🎉 checking it off.

OSBotify · 2021-09-01T01:42:23Z

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

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

Refactor the componentDidUpdate for ReportActionCompose

01d5c3d

mountiny requested a review from a team August 20, 2021 17:51

mountiny self-assigned this Aug 20, 2021

MelvinBot requested review from joelbettner and removed request for a team August 20, 2021 17:51

mountiny mentioned this pull request Aug 20, 2021

Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats #4761

Merged

5 tasks

marcaaron requested review from marcaaron and removed request for joelbettner August 20, 2021 18:12

marcaaron requested changes Aug 23, 2021

View reviewed changes

mountiny added 2 commits August 25, 2021 18:25

Refactor and return early

56dfcf0

Refactor componentDidUpdate in ReportScreen as well and update comments

57ec194

mountiny requested a review from a team as a code owner August 25, 2021 17:36

MelvinBot requested review from bondydaa and removed request for a team August 25, 2021 17:37

mountiny removed the request for review from bondydaa August 25, 2021 17:39

bondydaa reviewed Aug 25, 2021

View reviewed changes

marcaaron reviewed Aug 25, 2021

View reviewed changes

mountiny added 2 commits August 25, 2021 22:42

Remove redundant comment

9eb84c0

Update the comment to give better description and reason for the change

a27e869

mountiny requested review from bondydaa and marcaaron August 25, 2021 21:54

bondydaa approved these changes Aug 25, 2021

View reviewed changes

marcaaron approved these changes Aug 30, 2021

View reviewed changes

marcaaron merged commit 36dc811 into main Aug 30, 2021

marcaaron deleted the vit-refactorTheReportActionCompose branch August 30, 2021 19:43

OSBotify mentioned this pull request Aug 31, 2021

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

eVoloshchak mentioned this pull request Mar 15, 2023

[HOLD for payment 2023-03-14] [$2000] Editing status doesn't sync correctly between tabs. #15284

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the componentDidUpdate method for ReportActionCompose #4768

Refactor the componentDidUpdate method for ReportActionCompose #4768

mountiny commented Aug 20, 2021 •

edited by roryabraham

Loading

mountiny commented Aug 20, 2021 •

edited

Loading

marcaaron commented Aug 23, 2021

marcaaron Aug 23, 2021

marcaaron Aug 23, 2021

mountiny Aug 25, 2021

mountiny commented Aug 25, 2021

bondydaa Aug 25, 2021

mountiny Aug 25, 2021

marcaaron Aug 25, 2021

mountiny Aug 25, 2021

bondydaa Aug 25, 2021

mountiny Aug 25, 2021

marcaaron Aug 25, 2021

mountiny Aug 25, 2021

mountiny commented Aug 25, 2021

OSBotify commented Aug 30, 2021

OSBotify commented Aug 31, 2021

isagoico commented Aug 31, 2021

roryabraham commented Sep 1, 2021

isagoico commented Sep 1, 2021

OSBotify commented Sep 1, 2021

	// If we switch the reports, make sure to update the composer comment; otherwise return early
	// If we switch the reports, make sure to update the composer comment because [we don't want to leave old data around] [we shouldn't copy what a user had types in one place to another]

Refactor the componentDidUpdate method for ReportActionCompose #4768

Refactor the componentDidUpdate method for ReportActionCompose #4768

Conversation

mountiny commented Aug 20, 2021 • edited by roryabraham Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny commented Aug 20, 2021 • edited Loading

marcaaron commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny commented Aug 25, 2021

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

mountiny commented Aug 25, 2021

OSBotify commented Aug 30, 2021

OSBotify commented Aug 31, 2021

isagoico commented Aug 31, 2021

roryabraham commented Sep 1, 2021

isagoico commented Sep 1, 2021

OSBotify commented Sep 1, 2021

mountiny commented Aug 20, 2021 •

edited by roryabraham

Loading

mountiny commented Aug 20, 2021 •

edited

Loading