Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$250] [OldDot Rules Migration] - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time #49254

Open IuliiaHerets opened 3 days ago

IuliiaHerets commented 3 days ago

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: 9.0.34-2 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Rules.
  3. Enable "Auto-pay approved reports".
  4. Note that the amount for "Auto-pay reports under" is 0.
  5. Disable "Auto-pay approved reports".
  6. Reenable "Auto-pay approved reports".
  7. Note that the amount for "Auto-pay reports under" is 100.00.

Expected Result:

Since the default value after reenabling "Auto-pay approved reports" is 100 (Step 7), when enabling it for the first time (Step 4), the value should be 100.

Actual Result:

In Step 4, when enabling "Auto-pay approved reports" for the first time, it is 0. In Step 7, after disabling and reenabling it, it becomes 100.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/6df166cd-75df-42f2-a266-f0ae39b64c95

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021835706244754359508
  • Upwork Job ID: 1835706244754359508
  • Last Price Increase: 2024-09-16
Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 3 days ago

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 3 days ago

@joekaufmanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

DylanDylann commented 3 days ago

@BrtqKr It seems this relates to this issue https://github.com/Expensify/App/issues/47014. Could you take a look?

etCoderDysto commented 3 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time

What is the root cause of that problem?

When user enable Auto-pay approved reports for the first time, enabled is true, autoReimbursementValues.limit won't be assigned CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENT which is 100. https://github.com/Expensify/App/blob/e9c7377bfaa96365a4676fa9eaa58420f959289c/src/libs/actions/Policy/Policy.ts#L4487 https://github.com/Expensify/App/blob/e9c7377bfaa96365a4676fa9eaa58420f959289c/src/libs/actions/Policy/Policy.ts#L4489-L4495 Since autoReimbursementValues.limit is not updated above, it will default to policy.autoReimbursementValues.limit which has a value of 0

Then when the user disables Auto-pay approved reports, enabled is false, autoReimbursementValues.limit will be assigned CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENT which is 100.

Then when the user enables they will see 100

What changes do you think we should make in order to solve the problem?

We should remove ! before enable so that we should assign a default value of 100 to autoReimbursementValues.limit every time the user enables the switch. https://github.com/Expensify/App/blob/e9c7377bfaa96365a4676fa9eaa58420f959289c/src/libs/actions/Policy/Policy.ts#L4487

What alternative solutions did you explore? (Optional)

We can set CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENT` to 0 to make the default value of Auto-pay approved reports amount to 0

nkdengineer commented 3 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

In Step 4, when enabling "Auto-pay approved reports" for the first time, it is 0. In Step 7, after disabling and reenabling it, it becomes 100.

What is the root cause of that problem?

When we enable we update nothing in optimistic data then the default limit amount is 0. After we disable and enable again, the amount is 100 because it's updated in optimistic data when we disable the limit amount.

https://github.com/Expensify/App/blob/19d037b3a900b08de1f8ba2f22624ba445abe3a5/src/libs/actions/Policy/Policy.ts#L4481-L4489

What changes do you think we should make in order to solve the problem?

We should change !enabled condition here to enabled to update the correct optimistic data.

https://github.com/Expensify/App/blob/19d037b3a900b08de1f8ba2f22624ba445abe3a5/src/libs/actions/Policy/Policy.ts#L4481-L4489

I checked after reset cache and restart again the amount limit still is 0 so we also need a back-end change to update the correct limit amount value when we enable Auto-pay approved reports

What alternative solutions did you explore? (Optional)

joekaufmanexpensify commented 3 days ago

I couldn't reproduce it 100% on the first try. The first time I tried it, I saw $100 initially, but then when I toggled the setting back off, the app crashed. When I reloaded, and tried a second time, I saw the behavior of defaulting to $0.

Then, when I tried with a second workspace, I saw the bug on the first try. So it def exists, and maybe the crash is a related issue? Both are shown below:

https://github.com/user-attachments/assets/5c207171-0836-4874-9207-0b4adafce6fa

https://github.com/user-attachments/assets/74ed2f59-04dc-4d5e-a673-5fdc6196e75c

joekaufmanexpensify commented 3 days ago

@DylanDylann assigning you as C+ here as I see you worked on the original PR

joekaufmanexpensify commented 3 days ago

cc @marcaaron too as I see you're the project lead

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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

joekaufmanexpensify commented 3 days ago

If this is a FE fix, and @BrtqKr is available, that makes sense to me as I see Software Mansion is working on this project

marcaaron commented 3 days ago

@joekaufmanexpensify did you see any JS console logs when the crash happened?

This issue LGTM. I assigned myself so I can help look into the backend changes. @nkdengineer's proposal sounds correct and we will need some kind of change.

marcaaron commented 3 days ago

I also noticed that the current code is wrong as it sets the autoApproval.auditRate to 5 which is not correct. It should be a float value as per the design doc:

Random report audit
Command name: SetPolicyAutomaticApprovalAuditRate
API request parameters: policyID: String, auditRate: Int
New values:
{
    autoApproval: {
        auditRate: Float
    }
}

API Note: We will accept Int values to the API, but end up using this as a fraction so 1 = 0.1 and 100 = 1. Values over 100 are not permitted.

I think the best thing would be to just make it a float value in the Onyx data and for what gets sent to the API. So the new "default" would be 0.05 not 5. Does it make sense?

marcaaron commented 3 days ago

I think I'm gonna take over this whole issue sorry. It's just gonna be easier for me to implement than to explain exactly what needs to be done.

joekaufmanexpensify commented 2 days ago

@joekaufmanexpensify did you see any JS console logs when the crash happened?

I did not at the time. I just tried again a few times now and can no longer reproduce the crash behavior, so thinking we can do nothing if/until it comes up again.

marcaaron commented 2 days ago

Ok looked at this some more today. We are also sending the incorrect params to the API. Fixing it now...

joekaufmanexpensify commented 17 hours ago

Various PRs in review here