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] [Held requests] System message for hold/unhold request is not gray like other system messages #36894

Closed
6 tasks done
lanitochka17 opened this issue Feb 20, 2024 · 45 comments
Assignees
Labels
Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 20, 2024

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


Version Number: 1.4.43-2
Reproducible in staging?: Y
Reproducible in production?: N
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Request money from any user
  3. Navigate to request details page
  4. Edit amount/date/description
  5. Click 3-dot menu > Hold request
  6. Enter a reason and save it
  7. Click 3-dot menu > Unhold request

Expected Result:

The system message for hold and unhold request should be in gray like other system messages

Actual Result:

The system message for hold and unhold request is black instead of gray

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6385614_1708429169585.bandicam_2024-02-20_19-29-18-400.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d47ff630bf2f2350
  • Upwork Job ID: 1759954343597240320
  • Last Price Increase: 2024-02-20
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor labels Feb 20, 2024
@melvin-bot melvin-bot bot changed the title Hold Request - System message for hold/unhold request is not gray like other system messages [$500] Hold Request - System message for hold/unhold request is not gray like other system messages Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01d47ff630bf2f2350

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 20, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 20, 2024
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.

Copy link

melvin-bot bot commented Feb 20, 2024

Triggered auto assignment to @johnmlee101 (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@lanitochka17
Copy link
Author

lanitochka17 commented Feb 20, 2024

We think that this bug might be related to #vip-bills
CC @davidcardoza

@shubham1206agra
Copy link
Contributor

I don't think this is a deploy blocker. We have approve submit flow having non-gray messages.

@johnmlee101
Copy link
Contributor

I think this should definitely be fixed, although I don't know if its deploy-blocker worthy.

@johnmlee101
Copy link
Contributor

@BartoszGrajdek can you take a look at this?
as part of your PR here #33897

@BartoszGrajdek
Copy link
Contributor

Yeah, I'll create a fix for that 😄

@johnmlee101
Copy link
Contributor

Thanks! 🙇

@parasharrajat
Copy link
Member

Let me know if you need me to review this, I am available.

@trushpatel1
Copy link

Project Overview:
I understand that Expensify is transitioning its platform to React Native. The specific issue (#36894) is part of this larger migration effort.

Proposal Details:

Issue Analysis: My initial step will involve a thorough review of the issue as detailed in GitHub issue #36894. This includes understanding the existing challenges and the desired outcomes post-migration.

Technical Solution:

Code Refactoring: The existing code will be refactored for compatibility with React Native, ensuring a smooth transition across platforms.
Testing and Debugging: A comprehensive testing strategy will be employed to identify and resolve any bugs or issues. This includes unit testing for individual components and integration testing to ensure overall app stability.
Performance and Security: Special attention will be paid to maintaining high performance and robust security, especially given the financial nature of the Expensify app.
Documentation and Reporting:

Detailed documentation will be provided, outlining the changes made, reasons for these changes, and any new practices implemented.
Regular updates will be communicated throughout the project duration.
Final Submission:

The completed solution will be submitted via a pull request on the Expensify/App GitHub repository, in accordance with the contributing guidelines. This submission will include screenshots and evidence of testing on all platforms.
Budget and Timeline:

Proposed Fixed Price: $500

Copy link

melvin-bot bot commented Feb 20, 2024

📣 @trushpatel1! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@johnmlee101
Copy link
Contributor

@BartoszGrajdek what's the latest?

@BartoszGrajdek
Copy link
Contributor

BartoszGrajdek commented Feb 20, 2024

Sorry - too many fixes / other issues to handle at once 😅

It looks like the change in draft PR above fixes the problem, I need to test it first though

@robertjchen robertjchen removed DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor labels Feb 20, 2024
@youssef-lr youssef-lr added the Reviewing Has a PR in review label Mar 2, 2024
@melvin-bot melvin-bot bot removed the Overdue label Mar 2, 2024
@robertjchen robertjchen changed the title [$500] Hold Request - System message for hold/unhold request is not gray like other system messages [$500] [Held requests] System message for hold/unhold request is not gray like other system messages Mar 8, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek Eep! 4 days overdue now. Issues have feelings too...

@robertjchen
Copy link
Contributor

Awaiting tests by @youssef-lr (and we also switched focus to partial pay/approve, but will revisit after that is out)

@youssef-lr
Copy link
Contributor

youssef-lr commented Mar 12, 2024

Sorry haven't got to get back to that PR, but it's really just a clean up of the code now. The issue was fixed in Monil's PR.

Copy link

melvin-bot bot commented Mar 20, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@robertjchen
Copy link
Contributor

@youssef-lr let me know how I can help finish this one out? 🙏

@trjExpensify
Copy link
Contributor

What's the next step on this issue?

@youssef-lr
Copy link
Contributor

@trjExpensify this is fixed, but we wanted to cleanup the code in the PR instead, so it's not really affecting the product. I'll get back to the PR review tomorrow.

@trjExpensify
Copy link
Contributor

Great! How's that going?

Copy link

melvin-bot bot commented Apr 12, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Apr 16, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Apr 18, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Apr 22, 2024

@youssef-lr, @parasharrajat, @BartoszGrajdek 12 days overdue. Walking. Toward. The. Light...

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 25, 2024
Copy link

melvin-bot bot commented Apr 25, 2024

This issue has not been updated in over 14 days. @youssef-lr, @parasharrajat, @BartoszGrajdek eroding to Weekly issue.

@trjExpensify
Copy link
Contributor

Bump on this one. Can we get it done? Thanks!

@parasharrajat
Copy link
Member

parasharrajat commented May 7, 2024

Looks like it was solved already in #36851. Can we please get this retested?

@trjExpensify
Copy link
Contributor

The clean-up @youssef-lr mentions here?

@trjExpensify this is fixed, but we wanted to cleanup the code in the PR instead, so it's not really affecting the product. I'll get back to the PR review tomorrow.

@youssef-lr
Copy link
Contributor

I think we can close this for now. The issue is fixed, I'm not sure it's wroth spending time right now on the clean up as it's pretty minor and is not causing any issues. What do you think @trjExpensify?

@trjExpensify
Copy link
Contributor

I think it's fine to create another issue for it, but we shouldn't forget about drying up the code and avoiding duplicating the logic. Perhaps it can come in-line with the enhanced system message project? CC: @deetergp @dylanexpensify

@deetergp
Copy link
Contributor

deetergp commented May 8, 2024

What is it that needs to be cleaned up here?

@parasharrajat
Copy link
Member

Let's close this issue.

@trjExpensify
Copy link
Contributor

I'll let @youssef-lr make a call on that.

@parasharrajat parasharrajat removed their assignment May 29, 2024
@parasharrajat
Copy link
Member

Accidentally unassigned. Please assign me back if needed.

Any update on this issue? @youssef-lr @trjExpensify

@youssef-lr
Copy link
Contributor

From checking the latest code on main, I can see this has already been cleaned up. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Reviewing Has a PR in review Weekly KSv2
Projects
Archived in project
Development

No branches or pull requests