Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2021-12-15] $500 Chat Switcher - Draft message from previous conv is displayed after using chat switcher #4756

Closed
isagoico opened this issue Aug 19, 2021 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Aug 19, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! https://www.upwork.com/jobs/~01e5dccfcff059ed0f


Action Performed:

Please, see this comment for further investigation and reproduction steps. Some of the hints might helps us find what is exactly going wrong.

  1. Navigate to a conversation
  2. Write something in compose box
  3. Use ctrl+k command
  4. Select another conversation

Expected Result:

Nothing should be displayed in compose box if the selected conversation didn't have a draft

Actual Result:

The draft from the previous conversation is displayed on the current conversation.

Workaround:

For chats with people which they have already chatted with, they can use the LHN chat switcher and the drafts are not transferred. Also when user sends a message to someone or composes a draft which is then deleted, they wont experience this bug for the given chat.

When this will prevail is in case user creates chat with new person using the Ctrl + k chat switcher.

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.0.86-2

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Recording.206.mp4

Expensify/Expensify Issue URL:

View all open jobs on GitHub


From @iwiznia https://expensify.slack.com/archives/C01GTK53T8Q/p1629393716037000

BUG: (tested on web, staging, probably deploy blocker): draft is not working properly. Steps:
Write something in the chat but don't send it
Switch to another chat using CMD+K shortcut
See that draft message is carried over to the chat you switched, when it should not

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Aug 19, 2021
@MelvinBot
Copy link

Triggered auto assignment to @mountiny (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Aug 19, 2021
@isagoico isagoico changed the title Chat Switcher - Draft message form previous conv is displayed after using chat switcher Chat Switcher - Draft message from previous conv is displayed after using chat switcher Aug 19, 2021
@mountiny
Copy link
Contributor

The only PR which might be related to this and could introduced it is #4538 @marcaaron but I am not completely sure. What do you think? I can't really dig into this one right now. I would in the morning if noone identifies what is causing it 🙇 Thank you in any case for having a look.

@luacmartins
Copy link
Contributor

It seems that with #4538 ReportActionCompose no longer mounts and the input ref is lost in the update.

@marcaaron
Copy link
Contributor

Why is the input not mounting? Which ref are you referring to?

@luacmartins
Copy link
Contributor

When the chat is switch using the search bar didComponentMount is not triggered for ReportActionCompose and this inputRef does not seem to update.

@marcaaron
Copy link
Contributor

The ReportActionCompose component should be mounting only once now. Is there some reason it needs to be unmounted and remounted? I'm not seeing it yet...

@luacmartins
Copy link
Contributor

I agree that it should be mounted only once and we should keep it that way. What seems to be causing this issue is that this.setInputRef is not providing the correct ref to TextInputFocusable. Both this.props.comment and defaultValue are correct when switching chats with the Search Page, so I'm not sure why the value is not updated.

@mountiny
Copy link
Contributor

Thank you both for looking into this! Following from what you have found @luacmartins, I have identified that when using the SearchBar, the ReportActionCompose does need to update couple of time (after selecting different chat from the search) before it gets the report data of the new report.

In comparison, when using sidebar chat switcher, the components gets the correct props immediately after switching as it mounts.

I haven't been able to easily revert that PR either because it seems there were some additional ONYX changes made after it, which make the ReportActionsView not render correctly.

Therefore, I have tried to find some possibly temporary workaround, before @marcaaron might be able to look into how to update the refactoring to prevent this problem.

@mountiny
Copy link
Contributor

PR is ready for a review, but it still needs to be tested on Mobile to make sure the workaround does not cause any problems.

@isagoico
Copy link
Author

Retest for this in Web was a pass 🎉

@botify
Copy link

botify commented Aug 30, 2021

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

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

@mallenexpensify
Copy link
Contributor

Regression? Looks like it's happening again https://expensify.slack.com/archives/C01GTK53T8Q/p1634576408421600

@isagoico
Copy link
Author

This is only happening with Chronos conversation at the moment. Other conversations are working as expected

Action performed:

  1. Navigate to Chronos conversation
  2. Draft a message
  3. Hit ctrl + K
  4. Change to another chat
Recording.228.mp4

@mountiny mountiny removed Reviewing Has a PR in review DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 19, 2021
@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 4, 2021
@laurenreidexpensify
Copy link
Contributor

hired 👍🏽

@marcaaron
Copy link
Contributor

Obviously, I will refactor the code in a good way. I have already improved the navigation earlier.

👀

@parasharrajat
Copy link
Member

Those eyes are frightening. Sorry, I forgot to append with help of the team during review.

@mountiny mountiny added the Reviewing Has a PR in review label Nov 16, 2021
@mountiny
Copy link
Contributor

PR is up and in a review so added the label. This might take a while as we would like to make sure the changes are conceptual.

@laurenreidexpensify
Copy link
Contributor

@mountiny assume this is still in review?

@mountiny
Copy link
Contributor

@laurenreidexpensify Correct!

@MonilBhavsar
Copy link
Contributor

I'm coming from this related issue - #6490
Looking at the investigation and proposal, I believe this PR will also fix the linked issue.
Can anyone please confirm, thanks!

@parasharrajat
Copy link
Member

Yeah, I just checked that #6490 is not reproducible on the PR.

@MonilBhavsar
Copy link
Contributor

Thanks! 🙌

@laurenreidexpensify laurenreidexpensify removed the External Added to denote the issue can be worked on by a contributor label Dec 7, 2021
@laurenreidexpensify laurenreidexpensify removed their assignment Dec 7, 2021
@laurenreidexpensify laurenreidexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 7, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Dec 7, 2021
@laurenreidexpensify
Copy link
Contributor

@trjExpensify i'm OOO quite a bit over next week moving house, so handing this one off while I"m away thanks!

@trjExpensify
Copy link
Contributor

Cool, sounds good. Looks like we're waiting for a prod deploy to start the timer. 👍

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 8, 2021
@botify botify changed the title $500 Chat Switcher - Draft message from previous conv is displayed after using chat switcher [HOLD for payment 2021-12-15] $500 Chat Switcher - Draft message from previous conv is displayed after using chat switcher Dec 8, 2021
@botify
Copy link

botify commented Dec 8, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.18-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-12-15. 🎊

@parasharrajat
Copy link
Member

parasharrajat commented Dec 17, 2021

Ping for
image

@trjExpensify

FYI, this qualifies for Company offsite bonus.

@mallenexpensify
Copy link
Contributor

Paid @parasharrajat , wanted to get it issued before the weekend!
Thanks for inc. the 'company offsite hold' bonus info, I forgot to add the label and details to some of issues that were in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests