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

mountiny · 2021-08-20T13:55:31Z

Details

Testing on Staging has discovered a bug when using a Search sidebar modal to switch between chats does not update the composer box and draft from previous chat is shown in the new chat.

The issue was most likely introduced in this PR #4538. As a result of the refactoring the ReportActionCompose does not mount when switching the chats using the search bar and hence the text input is to updated either.

As a probably temporary workaround, I have updated the componentDidUpdate function to check if the comment value has changed between this.props and prevProps and update it manually using ref.

Fixed Issues

$ #4756

Tests

QA Steps

Follow the steps described in the linked issue #4756 and make sure that the bug does not prevail.

Tested On

Screenshots

Web

Screen.Recording.2021-08-20.at.14.59.02.mov

Mobile Web

Desktop

iOS

Android

github-actions · 2021-08-20T14:06:28Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

mountiny · 2021-08-20T14:06:49Z

@HorusGoul We still need to test it on mobile, especially Android if the workaround does not cause too much of a trouble.

HorusGoul · 2021-08-20T15:20:34Z

@HorusGoul We still need to test it on mobile, especially Android if the workaround does not cause too much of a trouble.

Tested! Working fine in both iOS and Android.

Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats (cherry picked from commit e9b3a59)

luacmartins

LGTM! Thanks for the fix @vitHoracek

mountiny · 2021-08-20T15:30:52Z

I should have created the test github account with a different name/tag haha 😄

luacmartins · 2021-08-20T15:34:56Z

Lol clearly tagging people is not my forte 🤣

mountiny · 2021-08-20T15:36:38Z

@luacmartins 😂 I admit this is definitely more my fault haha

OSBotify · 2021-08-20T16:18:16Z

🚀 Cherry-picked to staging by @HorusGoul in version: 1.0.86-4 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

marcaaron · 2021-08-20T16:34:28Z

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});
+        }


What about updating this.comment as well in this block or does that not matter? Honestly, I don't totally get why we started using setNativeProps() for this in the first place (I think to fix some bug) - but seeing this updateComment() makes me think it should be the one used here:

App/src/pages/home/report/ReportActionCompose.js

Lines 312 to 320 in 9135cfb

updateComment(newComment) {

this.textInput.setNativeProps({text: newComment});

this.setState({

isCommentEmpty: newComment.length === 0,

});

this.comment = newComment;

this.debouncedSaveReportComment(newComment);

this.debouncedBroadcastUserIsTyping();

}

I see, I have not considered the updateComment(), but I agree now it might be better. I was trying to go with as small change as possible because I have tried updating this.comment too, but it worked the same as without explicitly updating it, so I took it away.

I can submit a PR to use updateComment function and leave it in main and not CP as the functionality is same for now (at least what I could have seen myself).

I thought this would only be a temporary workaround because the componentDidUpdate function is called many times so we should aim to put as little logic in there as possible.

I can submit a PR to use updateComment function and leave it in main and not CP as the functionality is same for now (at least what I could have seen myself).

That sounds good to me. One reason why I would maybe use this is that if we refactor or change the setNativeProps thing in the future there will be only one place where we did it. Or said another way setting the native props is feels like more of a "detail" of the "update comment" code we are running.

I thought this would only be a temporary workaround because the componentDidUpdate function is called many times so we should aim to put as little logic in there as possible.

I'm curious what would the real solution be? It's fine to have logic in this method as long as we check when we should run it e.g. if we only need this to happen when the report switches then maybe it would be better to look at another prop besides the comment. I have one in mind... :)

Created a PR here 🙌

Or said another way setting the native props is feels like more of a "detail" of the "update comment" code we are running.

You are right there could be some hidden problem along the way by using solely the ref to update it. I think the reason it work d is that the component gets to the correct state eventually, but there is the couple of renders, which are still with the old reportID, which are causing the initial bug.

I have one in mind... :)

Thank you for the sneaky hint :) I have used this.props.report.reportID, because the reportID changes before the this.props.report is updated to the correct one and thus the bug would prevail.

but there is the couple of renders, which are still with the old reportID, which are causing the initial bug.

Let's discuss this in the new PR. I'm not sure I am getting why the reportID itself could not be used 🤔

OSBotify · 2021-08-25T17:54:02Z

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

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

OSBotify · 2021-08-30T17:38:33Z

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

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

Update the comment in text input manually in componentUpdate

1e79850

mountiny requested a review from a team as a code owner August 20, 2021 13:55

mountiny self-assigned this Aug 20, 2021

MelvinBot requested review from HorusGoul and removed request for a team August 20, 2021 13:55

mountiny changed the title ~~Update the comment in text input manually in componentUpdate~~ Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats Aug 20, 2021

Remove unnecessary check

9135cfb

mountiny added the CP Staging label Aug 20, 2021

HorusGoul approved these changes Aug 20, 2021

View reviewed changes

HorusGoul merged commit e9b3a59 into main Aug 20, 2021

HorusGoul deleted the vit-testForDeployBlocker branch August 20, 2021 15:22

github-actions bot pushed a commit that referenced this pull request Aug 20, 2021

Merge pull request #4761 from Expensify/vit-testForDeployBlocker

7cdad86

Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats (cherry picked from commit e9b3a59)

This was referenced Aug 20, 2021

🍒 Cherry pick PR #4761 to staging 🍒 #4764

Merged

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

luacmartins approved these changes Aug 20, 2021

View reviewed changes

marcaaron reviewed Aug 20, 2021

View reviewed changes

mountiny mentioned this pull request Aug 20, 2021

Refactor the componentDidUpdate method for ReportActionCompose #4768

Merged

5 tasks

OSBotify mentioned this pull request Aug 25, 2021

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

Closed

This was referenced Sep 1, 2021

Deploy Checklist: New Expensify 2021-09-01 #4976

Closed

Deploy Checklist: New Expensify 2021-09-01 #5004

Closed

Deploy Checklist: New Expensify 2021-09-03 #5046

Closed

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mountiny commented Aug 20, 2021 •

edited by HorusGoul

Loading

github-actions bot commented Aug 20, 2021

mountiny commented Aug 20, 2021

HorusGoul commented Aug 20, 2021

luacmartins left a comment

mountiny commented Aug 20, 2021

luacmartins commented Aug 20, 2021

mountiny commented Aug 20, 2021

OSBotify commented Aug 20, 2021

marcaaron Aug 20, 2021

mountiny Aug 20, 2021

marcaaron Aug 20, 2021

mountiny Aug 20, 2021 •

edited

Loading

marcaaron Aug 20, 2021

OSBotify commented Aug 25, 2021

OSBotify commented Aug 30, 2021

	updateComment(newComment) {
	this.textInput.setNativeProps({text: newComment});
	this.setState({
	isCommentEmpty: newComment.length === 0,
	});
	this.comment = newComment;
	this.debouncedSaveReportComment(newComment);
	this.debouncedBroadcastUserIsTyping();
	}

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

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

Conversation

mountiny commented Aug 20, 2021 • edited by HorusGoul Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Aug 20, 2021

mountiny commented Aug 20, 2021

HorusGoul commented Aug 20, 2021

luacmartins left a comment

Choose a reason for hiding this comment

mountiny commented Aug 20, 2021

luacmartins commented Aug 20, 2021

mountiny commented Aug 20, 2021

OSBotify commented Aug 20, 2021

marcaaron Aug 20, 2021

Choose a reason for hiding this comment

mountiny Aug 20, 2021

Choose a reason for hiding this comment

marcaaron Aug 20, 2021

Choose a reason for hiding this comment

mountiny Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

marcaaron Aug 20, 2021

Choose a reason for hiding this comment

OSBotify commented Aug 25, 2021

OSBotify commented Aug 30, 2021

mountiny commented Aug 20, 2021 •

edited by HorusGoul

Loading

mountiny Aug 20, 2021 •

edited

Loading