-
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
[HOLD for payment 2024-06-11] [$250] Taxes - App allows deleted tax rate to be selected as currency default in offline mode #41066
Comments
We think this issue might be related to the #collect project. |
Triggered auto assignment to @JmillsExpensify ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Taxes - App allows deleted tax rate to be selected as currency default in offline mode What is the root cause of that problem?We don't have any logic to exclude the deleted options or to disable the option when deleted offline. What changes do you think we should make in order to solve the problem?We can solve the issue in 2 ways.
const enabledTaxRates = sortedTaxRates.filter((taxRate) => !taxRate.isDisabled && !(taxRate.pendingAction === 'delete'));
Note: There might be similar pages with similar functionality, we need to check for that also. Also, we might not need to make changes when pending action is update/add, that can be excluded. What alternative solutions did you explore? (Optional)We can directly disable the option in
Alternative solutions 2
|
Proposal Updated
|
@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Adding as polish for wave-collect and opening up to the community. |
Job added to Upwork: https://www.upwork.com/jobs/~0120c22699693bf475 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
Thanks for the proposal @Krishna2323 I tested Categories and the issue is reproducible. I am pretty sure the issue lies in other similar functionality as well. @JmillsExpensify Should we completely hide the deleted tax, or simply disable it in the list? |
@JmillsExpensify, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@JmillsExpensify, @sobitneupane Huh... This is 4 days overdue. Who can take care of this? |
@JmillsExpensify Any thoughts on #41066 (comment)? |
@JmillsExpensify @sobitneupane this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@JmillsExpensify, @sobitneupane Eep! 4 days overdue now. Issues have feelings too... |
Good question. I tend to think that we should hide it in those other lists shown in the OP, especially given that we already use the pending pattern where the actual deletion occurred. cc @trjExpensify in case you disagree. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 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 2024-06-11. 🎊 For reference, here are some details about the assignees on this issue:
|
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, @Beamanator, @sobitneupane, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@JmillsExpensify, bump for payments. |
@JmillsExpensify, @Beamanator, @sobitneupane, @Krishna2323 Still overdue 6 days?! Let's take care of this! |
@sobitneupane mind filling out the BZ checklist? In the meantime, payment summary as follows:
|
Contributor paid out and C+ is paid through New Expensify, so I'm going to close this issue and we can circle back on checklist during payment approvals. |
The issue was already present in Section List. It was resurfaced in the app with the introduction of fields like Category, Tag and Tax fields |
Regression Test Proposal:
Do we agree 👍 or 👎 |
Need someone else to confirm my payment summary. I've added the regression test in the meantime. |
Contributor: $250 @Krishna2323 paid via Upwork |
$250 approved for @sobitneupane |
Everyone paid out and regression test created, so I'm closing this one. |
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.66-1
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team
Action Performed:
Precondition:
Expected Result:
In Step 7, user should not be able to select the deleted tax rate.
Actual Result:
In Step 7, user is able to select the deleted tax rate, which results in all the tax rate being able to be deleted.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6462146_1714080991768.20240426_052954.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JmillsExpensifyThe text was updated successfully, but these errors were encountered: