-
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-20] [HOLD for payment 2024-06-18] [$250] IOU – Disabled distance rate is present in the rate list in confirmation page #42284
Comments
Triggered auto assignment to @JmillsExpensify ( |
We think this issue might be related to the #collect project. |
ProposalPlease re-state the problem that we are trying to solve in this issue.IOU – Disabled distance rate is present in the rate list in confirmation page What is the root cause of that problem?We don't filter out the distances which are disabled. App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx Lines 55 to 65 in fb14d5a
What changes do you think we should make in order to solve the problem?Filter out the disabled rates and rates with pending action of delete. What alternative solutions did you explore? (Optional)Or we can add new parameters ( App/src/libs/OptionsListUtils.ts Line 1390 in fb14d5a
App/src/libs/OptionsListUtils.ts Line 1038 in fb14d5a
App/src/libs/OptionsListUtils.ts Line 1181 in fb14d5a
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Disabled rates are present when choosing the rates for distance requests. What is the root cause of that problem?We get information about the rates from the prop passed to the component : App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx Lines 38 to 44 in fb14d5a
The value of App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx Lines 101 to 102 in fb14d5a
But when we see the This is because we do not return the prop value of App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx Lines 102 to 103 in fb14d5a
This is because in App/src/libs/DistanceRequestUtils.ts Lines 202 to 208 in a03fecc
So even before thinking to filter the App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx Lines 58 to 63 in fb14d5a
What changes do you think we should make in order to solve the problem?So to start with first we need to return the value of enabled in `getMileageRates``, we can do it by updating the return logic here: Object.entries(distanceUnit.rates).forEach(([rateID, rate]) => {
mileageRates[rateID] = {
rate: rate.rate,
currency: rate.currency,
unit: distanceUnit.attributes.unit,
name: rate.name,
customUnitRateID: rate.customUnitRateID,
enabled: rate.enabled,
};
}); We also need to update the type of If there is extra change needed to deal with the case of offline mode, we can consider that too during the PR phase. So now we only need to update the following mapping to filter the enabled rates below mapping:
const sections = Object.values(rates)
.filter(rate => rate.enabled === true)
.map((rate) => { Result VideoScreen.Recording.2024-05-16.at.7.27.03.PM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.We've encountered two issues:
What is the root cause of that problem?Bug 1: App/src/libs/DistanceRequestUtils.ts Lines 255 to 264 in c19eea6
We don't currently check if lastSelectedDistanceRates is disabled. Additionally, in the getDefaultMileageRate function, we're not excluding disabled rates either. Bug 2: App/src/libs/DistanceRequestUtils.ts Lines 189 to 211 in c19eea6
What changes do you think we should make in order to solve the problem?App/src/libs/DistanceRequestUtils.ts Line 263 in c19eea6
This is draft implementation: #42330 (UPDATED) What alternative solutions did you explore? (Optional)If we don't want to add new param to getMileageRates function we can manually filter out the disable rate in
App/src/libs/DistanceRequestUtils.ts Line 263 in c19eea6
and keep this bullet point within the primary solution.
|
@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too... |
Job added to Upwork: https://www.upwork.com/jobs/~01c1179d796babea5c |
Opening this up to the community. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 ( |
This comment was marked as outdated.
This comment was marked as outdated.
@ikevin127, test branch for my alternative solution. This will make the distance selector behave exactly like tag/category/tax pickers. Currently we don't have search functionality also. |
@Krishna2323 Thank you for your proposal. Your proposal's RCA is incomplete as only filtering out the disabled distance rates from the Rate option selector is not enough to fix this issue. See cretadn22's proposal RCA for more details. Your main solution is not detailed enough for me to assess whether or not it would fix the issue. Even if it would be, since the RCA is not complete, it would most likely not tackle the issue fully. Regarding your alternative solution, I think it adds too much overhead and new code when not necessary - besides this, I'm reserved when it comes to adding 2 additional arguments to a function that already has 23 arguments. I checked out your test branch and the same thing mentioned above applies:
Besides not fixing the issue completely, due to the changes from your test branch -> now there's no more difference in the UI when it comes to
|
@GandalfGwaihir Thank you for your proposal. Your proposal's RCA is incomplete as only filtering out the disabled distance rates from the Rate option selector is not enough to fix this issue. See cretadn22's proposal RCA for more details. Your proposed solution only filters out the disabled distance rates from the Rates selector but if you follow the OP video carefully, once the new distance rate was added, the existing (default) one was disabled -> when reaching the confirmation page even though the existing (default) distance rate is disabled -> it still shows up as the default selected rate in the Rate option for both |
@cretadn22 Thank you for your proposal. Your proposal's RCA is correct. Your solution is problematic since you don't clearly mention which part of the issue the suggested change is targeting. Additionally, your test branch does not help me in assessing whether your solution is working because of the following reasons:
|
Notes for contributorsSome pointers regarding issue complexity This issue has 2 sides to it, see cretadn22's proposal RCA for more details. To get a better sense of the root cause I'd advise following these steps: Tip
Now before following the rest of the steps from OP, know that there are 2 different ways the UI can look when arriving on the Distance request confirmation page, and this differs based on whether your account
Note: This can be enabled / disabled from Some RCA & Solution pointers (from a reviewer's POV) ✅ Have a clear RCA that accounts for the entirety of the issue. ❌ Avoid adding extra parameters to existing functions which already have too many parameters. Important The first contributor to update / post a new proposal considering all of the above will be selected on a cc @Krishna2323 @GandalfGwaihir @cretadn22 |
@ikevin127 I updated proposal (I updated the detail implementation to address your comment)
In certain instances, you may require the disabled rate, while in others, filtering out the disabled rate is necessary. Therefore, I propose the addition of the 'includeDisableRate' parameter. This adjustment enhances the flexibility of the getMileageRates function and ensures adherence to the DRY (Don't Repeat Yourself) principle |
ProposalPlease re-state the problem that we are trying to solve in this issue.Disabled distance rate is present in the rate list in confirmation page What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
App/src/libs/DistanceRequestUtils.ts Line 51 in aa9bbcf
What alternative solutions did you explore? (Optional)NA |
♻️ Reviewing proposals once again. |
@nkdengineer Thank you for your proposal. Your proposal's RCA is correct and complete since it was already pointed out previously. As far as your solution goes, though it would fix the issue, due to the different route you approached when it comes to disabling the Rate list options -> this does not match with the expected result which states:
If the assigned CME decides that we want to involve design and adjust the expected result to match your proposed solution, then I would be willing to reconsider assignment. |
@cretadn22's updated proposal looks good to me! They were first to identify the root cause correctly and completely and first to update their proposal after initial review. Their updated main solution / test branch fixes the issue accordingly as per issue's expected result by:
🎀👀🎀 C+ reviewed |
@ikevin127 The PR here #42330. It is ready for your review |
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. |
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. |
This comment has been minimized.
This comment has been minimized.
@ikevin127 Our PR isn't reverted, regressions also be fixed by another PR. No action is required on our part in this situation. |
@cretadn22 Sorry, my bad - I just noticed the revert PR being closed and the issue being fixed somewhere else. Note: The initial PR was merged 7-days ago but never deployed to staging / production. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 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-18. 🎊 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:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 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-20. 🎊 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:
|
Regression Test ProposalTest 1Precondition: User does NOT have betas enabled, hence no Rate field will be displayed in Distance confirmation page.
Test 2Precondition: User does have betas enabled, hence the Rate field will be displayed in Distance confirmation page.
Do we agree 👍 or 👎. |
Summary:
Note: Some regressions were associated with this PR, which if confirmed -> the compensation will be reduced. I asked @neil-marcellini to confirm this above in #42284 (comment) so that everything is clear and ready for payment when due. |
Yes there was a regression caused by #42330 |
Payment issued to @ikevin127 and @cretadn22 factoring in regression 👍 |
done |
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: v1.4.74-1
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team
Action Performed:
Expected Result:
Disabled distance rate is not present in the rate list in confirmation page
Actual Result:
Disabled distance rate is present in the rate list in confirmation page
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6482488_1715847912862.D_rate.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: