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.56k stars 2.9k forks source link

[OldDot Rules Migration] Category Rules #47015

Closed marcaaron closed 1 month ago

marcaaron commented 3 months ago

Part of the OldDot Rules Migration project

Main issue: https://github.com/Expensify/Expensify/issues/413886

Feature Description

2024-08-07_14-01-08 2024-08-07_14-39-49 2024-08-07_14-40-01 ![2024-08-07_14-40-16] 2024-08-07_14-40-21 (https://github.com/user-attachments/assets/dba5b219-9d51-4935-909f-ce17e635bdb0) 2024-08-07_14-40-36 2024-08-07_14-40-47

High Level Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.dk0p2mr4zxie

Detailed Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.dk0p2mr4zxie

Manual Test Steps

TBD

Automated Tests

TBD

Issue OwnerCurrent Issue Owner: @WojtekBoman
WojtekBoman commented 3 months ago

Hi! I'm going to work on this :)

WojtekBoman commented 2 months ago

@Expensify/design

I have one question about the Approver page.

I see we have a similar page in the workflows section and it looks like this: Screenshot 2024-08-26 at 13 19 55

Can the category and tag approver selection pages be implemented similarly this page?

image

I'm asking because I don't know if I should display the selected approver at the top of the page and the All section below.

WojtekBoman commented 2 months ago

@shawnborton @dannymcclain Could you take a look at it when you have some time? https://github.com/Expensify/App/issues/47015#issuecomment-2309974690

shawnborton commented 2 months ago

Can the category and tag approver selection pages be implemented similarly this page?

I think the answer is yes, but only after there is an approver selected. Otherwise I assume we start off having no approver selected for those two things, and thus the list would not have anything at the top - it would just be a list of all available options.

Also, let's make sure we take a look at the updated flows for approvers as I think what you are showing is the "old" approval UI, right?

dannymcclain commented 2 months ago

I think the answer is yes, but only after there is an approver selected. Otherwise I assume we start off having no approver selected for those two things, and thus the list would not have anything at the top - it would just be a list of all available options.

Agree.

WojtekBoman commented 2 months ago

Also, let's make sure we take a look at the updated flows for approvers as I think what you are showing is the "old" approval UI, right?

Yes, when I was writing this question I didn't notice it had been updated :)

WojtekBoman commented 2 months ago

@marcaaron

I have a few questions about the category rules :)

  1. On the Require receipts over page we should display policy?.maxExpenseAmount with policy?.outputCurrency for the default option, right? Is it appropriate to display $0 * Default if policy?.maxExpenseAmount is 0.
  2. I'm wondering if we should use onyx optimistic data for updating the approver and default tax rate, because items that we store in policy?.rules?.expenseRules and policy?.rules?.approvalRules have own ids. Should I generate optimistic data manually on the frontend side without ids?
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @twisterdotcom (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

marcaaron commented 2 months ago

On the Require receipts over page we should display policy?.maxExpenseAmount with policy?.outputCurrency for the default option, right? Is it appropriate to display $0 * Default if policy?.maxExpenseAmount is 0.

Yes. If the policy default is actually 0 we should show $0 • Default. The default for a new policy will not be 0, but 10000000000 (so in that case we won't show 0 because there is practically "no limit"). I'm not sure if we want to show anything special for that case though. Probably fine to have it just say Default or maybe Unlimited • Default.

I'm wondering if we should use onyx optimistic data for updating the approver and default tax rate, because items that we store in policy?.rules?.expenseRules and policy?.rules?.approvalRules have own ids. Should I generate optimistic data manually on the frontend side without ids?

I can't think of a reason why we would need those to be generated in the frontend.

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @twisterdotcom, @marcaaron, @WojtekBoman, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

twisterdotcom commented 1 month ago

How is this going? Is it done? This seems old.

marcaaron commented 1 month ago

This has been done for a while. But @dukenv0307 needs to be paid for the C+ role?

dukenv0307 commented 1 month ago

@twisterdotcom I think we need to process payment. I reviewed this PR so long ago

twisterdotcom commented 1 month ago

Payment Summary:

dukenv0307 commented 1 month ago

Thank you @twisterdotcom I accepted the offer