search

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.27k stars 2.71k forks source link

[HOLD for payment 2024-08-02] [Workspace Feeds] [External] Create new Edit Limit Type page #44328

Closed mountiny closed 3 weeks ago

mountiny commented 2 months ago

Part of the Workspace feeds project.

Implement the following part of the design doc.

@koko57 @VickyStash @allgandalf @DylanDylann

melvin-bot[bot] commented 2 months ago

@MariaHCD, @mountiny Whoops! This issue is 2 days overdue. Let's get this updated quick!

mountiny commented 2 months ago

Later phase of implementation

mountiny commented 1 month ago

@VickyStash will work on this one

VickyStash commented 1 month ago

Hi, I'm Viktoryia from Callstack - expert contributor group - and I would like to work on this issue.

trjExpensify commented 1 month ago

Assigned you! :)

VickyStash commented 1 month ago

@MariaHCD @mountiny I have a couple of questions here:

  1. In docs it says If a card limit is changed so that the remaining limit will go from >$0 to $0, we’ll throw a warning that lets the admin know this will decline transactions. Could you confirm how the limit type change affects the remaining limit so I can calculate it right.
  2. In docs it says If the card is changed to a fixed limit type and the total spend on the card exceeds the “fixed” limit (i.e. total spend over 2 months is $1,800, and admin is changing the limit type with a limit of $1,000), we won’t show Fixed as an option to switch to, because this would just terminate the card. Where can I find the total spend value?
  3. Should the Smart Limit option be shown on the edit screen if the approvals are not configured in the policy?

image

Note: I guess the button in the screen should say 'Save' instead of the 'Next'

MariaHCD commented 1 month ago

@VickyStash

For 1 & 2:

Could you confirm how the limit type change affects the remaining limit so I can calculate it right. Where can I find the total spend value?

I think we might need some additional thought around how to implement this. Left some initial thoughts here but I will start a fresh discussion in the channel.

Should the Smart Limit option be shown on the edit screen if the approvals are not configured in the policy?

Ah, good point. I would imagine there is no point in the SmartLimit option if approvals are not configured.

VickyStash commented 1 month ago

Ah, good point. I would imagine there is no point in the SmartLimit option if approvals are not configured.

@MariaHCD does it mean it's possible this screen will have only one option - Monthly (if Fixed and Smart are hidden)

MariaHCD commented 1 month ago

Yes, that's a possibility!

VickyStash commented 1 month ago

Updates: WIP, needs to resolve a couple of things mentioned above. Updates can be found in the Draft PR

VickyStash commented 1 month ago

I think we might need some additional thought around how to implement this. Left some initial thoughts here but I will start a fresh discussion in the channel.

@MariaHCD @mountiny Maybe I can open a PR without specific checks for now, and then we can specify them when linking the page with the backend?

MariaHCD commented 1 month ago

@VickyStash Yes, that sounds good to me. I'm aiming to start a discussion in the channel today.

MariaHCD commented 1 month ago

Started a discussion here: https://expensify.slack.com/archives/C036QM0SLJK/p1721311425532449

VickyStash commented 1 month ago

Updates:

VickyStash commented 1 month ago

Hey @dubielzyk-expensify, I guess you were working on these mockups. Could you please confirm the buttons text

image

I guess it should be Save instead of Next. And maybe Change limit type instead of Change limit. WDYT?

cc @shawnborton

shawnborton commented 1 month ago

That makes sense to me. When you are just editing a single push-row and you are not in "step-by-step flow mode", we use "Save" as the button text. And your suggestion to use "Change limit type" makes sense too.

But let's definitely get a check from @dubielzyk-expensify too before moving forward.

dubielzyk-expensify commented 1 month ago

Yep agree that those labels make more sense 👍

mountiny commented 1 month ago

Thanks for raising these questions!

I agree with the latest copy changes, makes sense!

VickyStash commented 1 month ago

The PR has been opened for the review 👀

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.12-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-02. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 3 weeks ago

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

melvin-bot[bot] commented 3 weeks ago

@MariaHCD, @VickyStash, @mountiny Eep! 4 days overdue now. Issues have feelings too...

mountiny commented 3 weeks ago

Not overdue, the backend has been merged

mountiny commented 3 weeks ago

I have created a fresh issue for this one as I forgot we still had this one open. Nevertheless, I put all the details of the api command into the new issue https://github.com/Expensify/App/issues/46959 and I think its fine to close this one and handle the integration over there @waterim @VickyStash