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

Fee calculators: Remove the currency input field #10272

Closed
drummer83 opened this issue Oct 20, 2022 · 9 comments · May be fixed by #10333
Closed

Fee calculators: Remove the currency input field #10272

drummer83 opened this issue Oct 20, 2022 · 9 comments · May be fixed by #10333
Assignees
Labels
papercut Labels papercuts after they've moved from the "Papercuts prioritized" column

Comments

@drummer83
Copy link
Contributor

drummer83 commented Oct 20, 2022

Description

- As a: enterprise or super admin
- On pages : /admin/enterprise_fees?enterprise_id=XXXX for all calculators

/admin/shipping_methods/new + edition page

/admin/payment_methods/new + edition page

- I want to be able to do: not seeing the currency field anymore. By default, it should use the currency of the instance.

Acceptance Criteria & Tests

  1. Login
  2. Create or edit shipping issues with fees (all calculators) must still work
  3. Create or edit payment issues with fees (all calculators) must still work
  4. Create or edit enterprise fees (all calculators) must still work
@RachL
Copy link
Contributor

RachL commented Jan 4, 2023

papercut confirmed!

@RachL RachL transferred this issue from openfoodfoundation/wishlist Jan 13, 2023
@RachL RachL added papercut Labels papercuts after they've moved from the "Papercuts prioritized" column and removed Confirmed Papercut labels Jan 13, 2023
@jibees jibees self-assigned this Jan 17, 2023
@dacook
Copy link
Member

dacook commented Jan 19, 2023

Hiding the currency works, but from what I can see I think it creates tech debt and is not a papercut. Maybe I'm not seeing the full picture but this is what I can see so far:

Calculator are classes that can be used for enterprise fees, shipping methods or payment methods. They have 'preferences', and some calculators have a currency preference (eg Flat Rate).

It's possible that some fees/methods have been created that have a different currency set.
Does the currency do anything?

  1. If not, why don't we remove the currency preference from the calculators entirely?
  2. If we need to keep the currency on the calculators, we could remove the stored preferences, and refer directly to the system default. In this case it's important to note that any fees/methods with a different currency would suddenly be using the system default.

Does this make sense, and does anyone agree with either option (@jibees or @drummer83)?

@lin-d-hop
Copy link
Contributor

The desired solution is to hide the unwanted UX from the user. and autofill with the instance currency Yes it creates tech debt to remove this functionality but it improves the user experience so perhaps we can use Danni's phrase and 'put lipstick on the pig'?

If any calculator has a different currency set it should be ignored to use the isntance default.

@dacook Can we remove the currency from calculators entirely within the scope of a papercut?

@dacook
Copy link
Member

dacook commented Jan 25, 2023

Apologies, I thought I had responded earlier!

Thanks for confirming, so the currency does nothing on calculators and can be removed.

We have a quick change to simply hide it for now. Perhaps we can call that step 1. Unfortunately it adds complexity (rather than taking it away), but we could ship it with the goal to clean up later.

But... It's probably not that much effort to remove the currency entirely.. actually why don't I just try it and see.
..Ok I have an alternate PR :)

@RachL
Copy link
Contributor

RachL commented Feb 1, 2023

@filipefurtad0 I think that #10333 will close this issue completely?

@filipefurtad0
Copy link
Contributor

Oops 😅

@filipefurtad0 filipefurtad0 reopened this Feb 1, 2023
@filipefurtad0
Copy link
Contributor

Not introduced here as these PRs are not yet in production, but perhaps good to document: we've observed this bugsnag error which seems related to currencies in shipping method calculators:

https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/63da7254a81c370008605893?event_id=63da725400ad2e7b61620000&i=sk&m=nw

@dacook
Copy link
Member

dacook commented Feb 1, 2023

Strange. But I think it should no longer happen after this new PR anyway 🤞

@dacook dacook assigned dacook and unassigned jibees Feb 14, 2023
dacook added a commit to dacook/openfoodnetwork that referenced this issue Mar 3, 2023
We no longer use currency on calculators (openfoodfoundation#10272).
@dacook
Copy link
Member

dacook commented Mar 6, 2023

The main part was merged on 25th Jan: hiding the currency input field.

The cleanup is a bit more complicated, so I've moved to a new tech debt issue: #10521

@dacook dacook closed this as completed Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papercut Labels papercuts after they've moved from the "Papercuts prioritized" column
Projects
6 participants