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

[$500] [HOLD for payment 2023-09-04] Cleared Statement Reappears Briefly When Deleting Comment Changes #25690

Closed
1 of 6 tasks
m-natarajan opened this issue Aug 22, 2023 · 44 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 22, 2023

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. Access the staging Expensify website.
  2. Open any chat and write a message.
  3. Hover over the message and click on the edit icon.
  4. Clear the message and click on the save changes icon.
  5. Observe that the delete page popup appears, click on the delete button.
  6. Notice that the cleared statement briefly reappears before being deleted.

Expected Result:

When clearing and deleting a comment, the cleared statement should not reappear even briefly.

Actual Result:

The cleared statement reappears briefly when deleting comment changes.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.56-3
Reproducible in staging?: Yes
Reproducible in production?: No
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-capture.-.2023-08-22T012713.550.mp4
Recording.383.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tewodrosGirmaA
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692693288512319

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0163bb21b363fde22a
  • Upwork Job ID: 1703770694266941440
  • Last Price Increase: 2023-09-18
  • Automatic offers:
    • 0xmiroslav | Reviewer | 26732815
    • tewodrosGirmaA | Contributor | 26732817
    • tewodrosGirmaA | Reporter | 26732820
@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@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.

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

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

@shubham1206agra
Copy link
Contributor

Unable to repro in DEV

@AndrewGable
Copy link
Contributor

I can reproduce on staging, cannot reproduce on production. Not sure if it's a deploy blocker, but looking into possible offending PRs.

@AndrewGable
Copy link
Contributor

I can reproduce on Dev too.

@AndrewGable
Copy link
Contributor

Reverting #24544 fixes this, I am going to send the revert.

@Pujan92
Copy link
Contributor

Pujan92 commented Aug 22, 2023

Seems regression from this PR

Currently whenever report action updates it executes setDraft here and we do need to avoid it whenever message isn't there.

useEffect(() => {
if (props.draftMessage === props.action.message[0].html) {
return;
}
setDraft(Str.htmlDecode(props.draftMessage));
}, [props.draftMessage, props.action.message]);

I think we can add 1 more condition to return early when action message is empty.

if (!props.action.message[0].text || props.draftMessage === props.action.message[0].html) {

@AndrewGable
Copy link
Contributor

Thanks for looking @Pujan92 - I am glad we came to the same conclusion. I am going to revert as I think this will get this fixed the fastest, then we can work with the original author to improve their original PR.

@Pujan92
Copy link
Contributor

Pujan92 commented Aug 22, 2023

Fine 👍

@AndrewGable
Copy link
Contributor

AndrewGable commented Aug 23, 2023

This fix was deployed, the PR that added it was reverted and cherry picked to staging.

@tewodrosGirmaA
Copy link

Hy @AndrewGable am I eligible for payment?

@AndrewGable
Copy link
Contributor

Yes, @johncschuster - Can you handle this?

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav (External)

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

📣 @tewodrosGirmaA 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@johncschuster
Copy link
Contributor

@tewodrosGirmaA can you accept the offer for the Reporter role?

@Expensify Expensify deleted a comment from melvin-bot bot Sep 18, 2023
@Expensify Expensify deleted a comment from melvin-bot bot Sep 18, 2023
@tewodrosGirmaA
Copy link

tewodrosGirmaA commented Sep 18, 2023

@johncschuster I accept it thank you

@0xmiros
Copy link
Contributor

0xmiros commented Sep 18, 2023

I am also eligible payment for C+ review.
ref: #25690 (comment)

@johncschuster
Copy link
Contributor

Oh! I missed that too. Thanks @0xmiroslav!

@0xmiros
Copy link
Contributor

0xmiros commented Sep 18, 2023

Also, PR was merged before price drop announcement so original base price applies here which is flatten 1k.

@johncschuster
Copy link
Contributor

Ah yes, I see the merge date was Aug 28, which is 2 days before the change was made (Slack announcement here).

@johncschuster
Copy link
Contributor

@tewodrosGirmaA it looks like you've accepted the Contributor job, which is incorrect. Can you please accept the Report job instead?

@tewodrosGirmaA
Copy link

Excuse me, @johncschuster. I apologize for my mistake earlier. I was on the bus and didn't read carefully. I noticed that you had sent me two offers, but this issue was created before August 31st, which means the offer should be $250. However, you had offered $50 instead. Could you please clarify? Thank you

@johncschuster
Copy link
Contributor

I noticed that you had sent me two offers, but this issue was created before August 31st, which means the offer should be $250. However, you had offered $50 instead. Could you please clarify? Thank you

I sure can! The Upwork job was created automatically, and reflects the current amounts for newly created jobs. That said, I will manually adjust the reporter amount to $250, since this was reported before the change was made.

@johncschuster
Copy link
Contributor

@tewodrosGirmaA I've extended an offer for $250. Can you accept that one when you get a moment? I'll get that payment issued right away.

@tewodrosGirmaA
Copy link

@johncschuster , I accept it , Thank you 😊

@johncschuster
Copy link
Contributor

Payment issued! Thanks, @tewodrosGirmaA!

@johncschuster
Copy link
Contributor

Summary of payments:

Reporter: @tewodrosGirmaA - $250
Reviewer: @0xmiroslav - $1000

Both paid via Upwork

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@AndrewGable @johncschuster Be sure to fill out the Contact List!

@tewodrosGirmaA
Copy link

Thank you 😊

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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants