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.53k stars 2.88k forks source link

[$250] Rules - Expense rules are not empty by default #48387

Open isagoico opened 2 months ago

isagoico commented 2 months 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: Reproducible in staging?: Yes Reproducible in production?: Yes

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/424689

Issue reported by: Applause - Internal team

Action Performed:

  1. As the admin of a collect workspace - Navigate to "More Features"
  2. Enable Rules
  3. On the paywall - Click upgrade to enable the Control plan in the policy
  4. Navigate to the Rules page in workspace edito

Expected Result:

When rules are first enabled no default will be set for Require receipt amount, Max expense amount, and Max expense age (days). As Described in the design doc in the "Individual expense rules" in "UI Additions & Changes Figma".

Actual Result:

Require receipt amount, Max expense amount, and Max expense age (days) have default values.

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

image

View all open jobs on GitHub

CC @JmillsExpensify @marcaaron - I think this should be/should have been implemented in the following issue https://github.com/Expensify/App/issues/47013

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834670232070295133
  • Upwork Job ID: 1834670232070295133
  • Last Price Increase: 2024-09-13
  • Automatic offers:
    • suneox | Reviewer | 103951620
    • CyberAndrii | Contributor | 103951623
Issue OwnerCurrent Issue Owner: @marcaaron
melvin-bot[bot] commented 2 months ago

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

📣 @goncastrum! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
CyberAndrii commented 2 months ago

Proposal

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

Expense rules are not empty by default

What is the root cause of that problem?

We set default values when a workspace is being upgraded

https://github.com/Expensify/App/blob/0e9f918425fb6c4a742516ea1aaa02a68a19527b/src/libs/actions/Policy/Policy.ts#L3413-L3415

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

Remove the 3 lines mentioned above. Additionally remove DEFAULT_MAX_EXPENSE_AGE, DEFAULT_MAX_EXPENSE_AMOUNT, and DEFAULT_MAX_AMOUNT_NO_RECEIPT from the CONST file as they will be no longer used.

JmillsExpensify commented 2 months ago

@marcaaron should we keep this internal as part of the on-going implementation or handle externally?

melvin-bot[bot] commented 1 month ago

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 1 month ago

Posted in Slack.

marcaaron commented 1 month ago

I think the solution proposed above will work so we can make this External. There's no BE changes needed AFAICT. We just need to update these values to the DISABLED_MAX_EXPENSE_VALUE.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)

melvin-bot[bot] commented 1 month ago

📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

📣 @CyberAndrii 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

CyberAndrii commented 1 month ago

Hm, when I went to record videos, my fix worked fine on the desktop app, but then in Safari it didn't. There seems to be a bug that prevents applying onyx data from the BE which tricked me into thinking this is a FE only issue. I managed to reproduce it by creating a workspace while offline. This causes the CreateWorkspace request to wait indefinitely for a response (you can see a loading indicator in devtools), which stalls the update queue.

So this also needs a fix on the backend. Should I still submit a PR?

https://github.com/user-attachments/assets/5d01c8a3-6d3f-42ca-9e19-d65213920f70

suneox commented 1 month ago

@marcaaron We need the backend to update the response value without the maxExpense... values

Screenshot 2024-09-15 at 09 56 21

then we can apply FE change to update optimisticData without default values

melvin-bot[bot] commented 1 month ago

@JmillsExpensify @suneox @marcaaron @CyberAndrii 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!

marcaaron commented 1 month ago

Ah bummer. Ok, I will raise the PR to fix this.

So this also needs a fix on the backend. Should I still submit a PR?

I mean we are still setting the optimistic data incorrectly so yes?

marcaaron commented 1 month ago

Also just to make sure this is needed...

@JmillsExpensify the doc actually says:

We won’t set a default for Require receipt amount, Max expense amount, and Max expense age (days) when Rules are first enabled.

I'm not sure that was intended to mean "when a user upgrades", but just when they "enable" rules - which is kind of distinct in that you could already be upgraded and enable or disable rules. It also seems like the "upgrade" path has logic to reset these back to defaults cc @youssef-lr who implemented the UpgradeToCorporate.

My question is whether we still want those defaults baked in? I'm seeing the existing behavior on OldDot is:

Which makes me wonder whether we need to do anything here or if we can just close this one out.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 1 month ago

Still deciding if we want to do this, but I will make it a Weekly for now. The changes are pretty minor.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Huh... This is 4 days overdue. Who can take care of this?

marcaaron commented 1 month ago

I'm gonna make this a monthly and try to get to it when I return from OOO. If we are passionate about it then we can get someone to help sooner.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify @suneox @marcaaron @CyberAndrii this issue is now 4 weeks old, please consider:

Thanks!

suneox commented 1 month ago

We’re still waiting to the internal team to make a decision regarding the default value.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Whoops! This issue is 2 days overdue. Let's get this updated quick!

suneox commented 1 month ago

Same above

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Whoops! This issue is 2 days overdue. Let's get this updated quick!

CyberAndrii commented 1 month ago

We have both daily and monthly labels here. Let's remove the daily one?

suneox commented 1 month ago

@MelvinBot Not overdue

melvin-bot[bot] commented 3 weeks ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

suneox commented 3 weeks ago

Same above https://github.com/Expensify/App/issues/48387#issuecomment-2383559786

melvin-bot[bot] commented 3 weeks ago

@JmillsExpensify, @suneox, @marcaaron, @CyberAndrii Whoops! This issue is 2 days overdue. Let's get this updated quick!

JmillsExpensify commented 3 weeks ago

Not a high priority.

marcaaron commented 3 weeks ago

No update yet.

marcaaron commented 2 weeks ago

No update yet.