search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.87k forks source link

[HOLD for regression test] Web - The whole message with the (edited) text disappears when the updated message contains quotes (" ") @adeel0202 #11377

Closed kbecciv closed 1 year ago

kbecciv commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to any chat
  3. Type any message containing quotes e.g. "test"
  4. Send the message
  5. Click on the edit icon
  6. Edit the message e.g. "test updated"
  7. Click on save changes to update the message
  8. Click on the edit icon again
  9. Click on save changes without making any change

Expected Result:

The whole message with the (edited) text does not disappear

Actual Result:

The whole message with the (edited) text disappears

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.8.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/192831193-942a1726-b2bd-48a3-ba99-bd9d755f0ca2.mov

Expensify/Expensify Issue URL:

Issue reported by: @adeel0202

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663071879002369

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

adeel0202 commented 2 years ago

There has been a change in this issue. I have also mentioned that on the slack thread. Now, not just the (edited) text but the whole message disappears and then reappears after a moment. So we might have to update the bug title/description accordingly.

kbecciv commented 2 years ago

@adeel0202 Updated the bug title

Christinadobrzyn commented 2 years ago

Ah interesting, I don't see any other issues related to this so going to send it to eng

melvin-bot[bot] commented 2 years ago

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

Luke9389 commented 2 years ago

Hmm, this smells like the refactor for Report_EditComment @PauloGasparSv, you did that one right? (I think this is the correct PR)

Does anything leap out to you about this issue? If not I can take a look but I figured you might know what's going on here off-hand.

PauloGasparSv commented 2 years ago

Hey @Luke9389! Yes I did : D

I debugged it a bit and (added the lines so you can add some breakpoints and follow): 1) The message is sent correctly from NewDot to Web-Expensify 2) Web-Expensify knows we are editing the message 3) Web-Expensify is sending the message content to Auth 4) Auth is returning the html field as an empty string to Web-Expensify 5) Web-Expensify returns that string through Onyx

And that's as far as I went. I think the problem may be on the Auth command that updates the comment or in the way we sanitize the message in Web-Expensify before sending it to Auth.

But It needs more investigation 😔

Luke9389 commented 2 years ago

I'll check this out tomorrow

Luke9389 commented 2 years ago

When testing this locally, I got this error:

?api? [info] transactionList is empty, skipping post processing
?api? [info] Finish initing report ~~ 160
?api? [info] Not updating fields, cache hasn't changed ~~ reportID: '160'
?api? [dbug] _send( expensidev2004.web.report.postProcessPartial:0|ms )
?api? [info] Found a deleted comment without any edit history ~~ sequenceNumber: '20'
?api? [info] Expensify\Onyx - Sending command response via Pusher ~~ 0: '[onyxMethod: 'merge' key: 'reportActions_160']' 1: '[onyxMethod: 'merge' key: 'report_160']'
?api? [info] Push_API - Sending to alternate endpoint to process Pusher notification asynchronously ~~ 0: '{"channels":[{"name":"user","accessModifier":"private-encrypted","params":{"accountID":"2"}},{"name":"user","accessModifier":"private-encrypted","params":{"accountID":"87"}}],"eventType":"onyxApiUpdate","canChunk":true,"eventLogID":"yCW79"}'

?api? [info] [trace] StackTrace start
?api? [info] [trace] 0/5. file '/vagrant/Web-Expensify/lib/Push/API.php' function 'traceFunctionCall' line '254'
?api? [info] [trace] 1/5. file '/vagrant/Web-Expensify/lib/Push/API.php' function 'send' line '174'
?api? [info] [trace] 2/5. file '/vagrant/Web-Expensify/lib/Onyx.php' function 'sendToUserChannelsByAccountIDs' line '120'
?api? [info] [trace] 3/5. file '/vagrant/Web-Expensify/lib/ReportAPI.php' function 'pushUpdatesToClients' line '560'
?api? [info] [trace] 4/5. file '/vagrant/Web-Expensify/lib/ReportAPI.php' function 'editComment' line '567'
?api? [info] [trace] 5/5. file '/vagrant/Web-Expensify/api.php' function 'updateCommentNewDot' line '745'
?api? [info] [trace] StackTrace end

I then decided to check out our logs and it looks like Report::updateCommentNewDot is hitting this a lot.

Luke9389 commented 2 years ago

I'll be doing further investigation on this one this week.

Luke9389 commented 2 years ago

Still need to roll around to this

Luke9389 commented 1 year ago

Still need to roll around to this

Luke9389 commented 1 year ago

OK I think we should actually approach this from the front end. The problem is only reproducable if we edit the message and then hit submit without making changes. If the front end detects this, it should just not send an API request at all (in other words, the submit button shouldn't even be clickable if the edited message is the same as the original).

Luke9389 commented 1 year ago

Poppin around to this tomorrow after re-brand stuff gets done.

Luke9389 commented 1 year ago

tackling this one this week.

JmillsExpensify commented 1 year ago

@Luke9389 just came across this issue as I was auditing all the older bugs in the repo. Will you be able to pick this up this week? If not, I think we should open this up to volunteers. We seem to have lots of engineers willing to jump in and help out, and if you're already working on other issues, let's get some helping hands to jump into this one. Cool?

JmillsExpensify commented 1 year ago

Actually, I just noticed that Luke is OOO, so I'm going to go ahead and unassign this one and highlight in Slack.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

JmillsExpensify commented 1 year ago

@sakluger This issue lacked a BZ member so I just removed/re-applied the Bug label.

JmillsExpensify commented 1 year ago

Also asked for volunteers in Slack.

stitesExpensify commented 1 year ago

Started looking into this Friday night, diving in fully today

stitesExpensify commented 1 year ago

Fix is up

Luke9389 commented 1 year ago

Thanks for moving the ball on my behalf @JmillsExpensify!

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2022-12-27. :confetti_ball:

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

JmillsExpensify commented 1 year ago

@sakluger Are you going to be around next week? If not, happy to jump in and help close this out.

mollfpr commented 1 year ago

@sakluger Could you assign me to the issue and create the Upwork job for the PR review? Thank you!

melvin-bot[bot] commented 1 year ago

Current assignees @JmillsExpensify and @sakluger are eligible for the Bug assigner, not assigning anyone new.

JmillsExpensify commented 1 year ago

Well that didn't work as planned. Upwork job is here: https://www.upwork.com/jobs/~01b913417c96073cc8. Payments will be as follows:

Can you both please apply and we can get this paid out today?

adeel0202 commented 1 year ago

Thanks. I have applied.

JmillsExpensify commented 1 year ago

Thanks, all paid out. @mollfpr This is ready for you when you're back online.

mollfpr commented 1 year ago

@JmillsExpensify It should be $500, the PR causing regression.

JmillsExpensify commented 1 year ago

Thanks for your honesty! I'm away from my computer but I'll circle back this evening.

sakluger commented 1 year ago

@mollfpr did you receive an offer already through Upwork?

mollfpr commented 1 year ago

@sakluger I haven't.

sakluger commented 1 year ago

Thanks - I've sent the offer now through Upwork, once you accept we'll finish paying out.

mollfpr commented 1 year ago

@sakluger Accepted, thank you!

JmillsExpensify commented 1 year ago

All paid out. @sakluger Given that we have a regression in play, we should definitely add something to TestRail. Updating the title and leaving that to you.

sakluger commented 1 year ago

I posted in #bug-zero in Slack with a regression test proposal (Slack link).

sakluger commented 1 year ago

Created the GH issue to add the regression test. I think we're all done here.