-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01d47ff630bf2f2350 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
👋 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:
|
Triggered auto assignment to @johnmlee101 ( |
We think that this bug might be related to #vip-bills |
I don't think this is a deploy blocker. We have approve submit flow having non-gray messages. |
I think this should definitely be fixed, although I don't know if its deploy-blocker worthy. |
@BartoszGrajdek can you take a look at this? |
Yeah, I'll create a fix for that 😄 |
Thanks! 🙇 |
Let me know if you need me to review this, I am available. |
Project Overview: 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. Detailed documentation will be provided, outlining the changes made, reasons for these changes, and any new practices implemented. 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. Proposed Fixed Price: $500 |
📣 @trushpatel1! 📣
|
@BartoszGrajdek what's the latest? |
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 |
@youssef-lr, @parasharrajat, @BartoszGrajdek Eep! 4 days overdue now. Issues have feelings too... |
Awaiting tests by @youssef-lr (and we also switched focus to partial pay/approve, but will revisit after that is out) |
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. |
@youssef-lr, @parasharrajat, @BartoszGrajdek Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@youssef-lr let me know how I can help finish this one out? 🙏 |
What's the next step on this issue? |
@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. |
Great! How's that going? |
@youssef-lr, @parasharrajat, @BartoszGrajdek Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@youssef-lr, @parasharrajat, @BartoszGrajdek 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
@youssef-lr, @parasharrajat, @BartoszGrajdek 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it! |
@youssef-lr, @parasharrajat, @BartoszGrajdek 12 days overdue. Walking. Toward. The. Light... |
This issue has not been updated in over 14 days. @youssef-lr, @parasharrajat, @BartoszGrajdek eroding to Weekly issue. |
Bump on this one. Can we get it done? Thanks! |
Looks like it was solved already in #36851. Can we please get this retested? |
The clean-up @youssef-lr mentions here?
|
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? |
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 |
What is it that needs to be cleaned up here? |
Let's close this issue. |
I'll let @youssef-lr make a call on that. |
Accidentally unassigned. Please assign me back if needed. Any update on this issue? @youssef-lr @trjExpensify |
From checking the latest code on main, I can see this has already been cleaned up. Thanks everyone! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:
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?
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
The text was updated successfully, but these errors were encountered: