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

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

Closed
6 tasks done
IuliiaHerets opened this issue Sep 16, 2024 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 16, 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: 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:

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

Screenshots/Videos

Bug6602422_1726236737147.20240913_220021.mp4

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: @
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

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
Copy link
Author

@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
Copy link
Contributor

@BrtqKr It seems this relates to this issue #47014. Could you take a look?

@etCoderDysto
Copy link
Contributor

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.

const autoReimbursementValues = !enabled ? {limit: CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENTS} : {};

const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
autoReimbursement: {
...autoReimbursementValues,

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.

const autoReimbursementValues = !enabled ? {limit: CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENTS} : {};

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
Copy link
Contributor

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.

const autoReimbursementCleanupValues = !enabled
? {
pendingFields: {
limit: null,
},
}
: {};
const autoReimbursementFailureValues = !enabled ? {autoReimbursement: {limit: policy?.autoReimbursement?.limit, ...autoReimbursementCleanupValues}} : {};
const autoReimbursementValues = !enabled ? {limit: CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENTS} : {};

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.

const autoReimbursementCleanupValues = !enabled
? {
pendingFields: {
limit: null,
},
}
: {};
const autoReimbursementFailureValues = !enabled ? {autoReimbursement: {limit: policy?.autoReimbursement?.limit, ...autoReimbursementCleanupValues}} : {};
const autoReimbursementValues = !enabled ? {limit: CONST.POLICY.AUTO_REIMBURSEMENT_DEFAULT_LIMIT_CENTS} : {};

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
Copy link
Contributor

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:

2024-09-16_11-37-33.mp4
2024-09-16_11-39-40.mp4

@joekaufmanexpensify
Copy link
Contributor

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

@joekaufmanexpensify
Copy link
Contributor

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

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 16, 2024
@melvin-bot melvin-bot bot changed the title Rules - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time [$250] Rules - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

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

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

melvin-bot bot commented Sep 16, 2024

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

@joekaufmanexpensify
Copy link
Contributor

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 marcaaron self-assigned this Sep 16, 2024
@marcaaron
Copy link
Contributor

@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
Copy link
Contributor

marcaaron commented Sep 16, 2024

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 marcaaron moved this to Polish in [#whatsnext] #expense Sep 16, 2024
@marcaaron marcaaron changed the title [$250] Rules - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time [$250] [OldDot Rules Migration] - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time Sep 16, 2024
@marcaaron
Copy link
Contributor

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.

@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 16, 2024
@joekaufmanexpensify
Copy link
Contributor

@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
Copy link
Contributor

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

@marcaaron marcaaron added the Reviewing Has a PR in review label Sep 18, 2024
@joekaufmanexpensify
Copy link
Contributor

Various PRs in review here

@joekaufmanexpensify joekaufmanexpensify added Weekly KSv2 and removed Daily KSv2 labels Sep 19, 2024
@joekaufmanexpensify
Copy link
Contributor

Web PR merged pending deploy. App PR on hold for that.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 26, 2024
@marcaaron
Copy link
Contributor

Just down to the final App PR now.

@joekaufmanexpensify
Copy link
Contributor

App PR on staging

@joekaufmanexpensify
Copy link
Contributor

App PR went out to prod on the 30th, but automation failed. Payment is due today

@joekaufmanexpensify joekaufmanexpensify changed the title [$250] [OldDot Rules Migration] - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time [hold for payment 2024-10-07] [$250] [OldDot Rules Migration] - Auto-pay approved reports amount is 0 instead of 100 when enabled for the first time Oct 7, 2024
@joekaufmanexpensify
Copy link
Contributor

Only payment here is $250 to @DylanDylann for C+ review of App PR via upwork

@joekaufmanexpensify
Copy link
Contributor

@DylanDylann offer for $250 sent!

@DylanDylann
Copy link
Contributor

@joekaufmanexpensify Accepted 🙏

@joekaufmanexpensify
Copy link
Contributor

@DylanDylann $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

All set, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

6 participants