[Vouchers] Percentage rate #10821

rioug · 2023-05-09T00:11:43Z

ℹ️ Please track code review under the project ##10430 Vouchers - Aus Pt 2 (UK).

What? Why?

Last part of [Vouchers] Include unfixed amounts and percentage rates #10727

Add the ability to have a percentage based voucher. It supports tax included in price and tax excluded from price to same extend that flat rate voucher ie. we can't have order which mix tax included and tax exclude from price.

See calculation reference here : #10432 (comment)

What should we test?

Enable vouchers feature
Visit admin/feature-toggle/features and enable or add vouchers feature

In the backoffice logged as an enterprise user:

Visit the Enterprise tab and click on "Settings" for a shop of your choosing, and click on the "vouchers" in left hand side menu
Click on "Add New button" --> New Voucher page should now have a "Voucher type" and an "Amount" input
Enter a voucher code, an amount and choose "Percentage(%)" type , and click "Save"
--> It should redirect you to the vouchers page on the enterprise edit page
--> You should now see a list of vouchers with the one you just created, included the entered percentage rate

On the website logged as a customer :

Add one or more product in your cart from the shop above
Proceed to checkout
Enter details and shipping info, click on "Next - Payment method"
Enter the voucher code and click "Apply"
--> You should see a "xx % Voucher" box where xx is the amount you entered when creating a voucher
choose a payment method and click on "Next - Order summary"
--> you should see the voucher applied to the order in the order summary column, with the correct amount
Click on "Complete order"
--> you should see the voucher applied to the order in the order summary with the correct amount

Repeat the above scenario with tax included in price and tax excluded from price

Payment fees and voucher

Payment fees are calculated based on the item total, and are recalculated when confirming the order. Vouchers are calculated based on the order total. The main thing to check here, is to make sure :

voucher calculation include the payment fee
when using a percentage based payment fee, the payment fee shouldn't change when confirming an order with a voucher
when a voucher cover the order total or more, -> No payment required, so the payment fee shouldn't apply

See this #11117 (comment) for more background

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

Dependencies

This is base on this PR #11117, ~~which depends on #11135~~. They will need to be merged first.

Documentation updates

rioug · 2023-05-12T01:57:18Z

New commit start here: 28b3163

Admin screen and Calculation are done.
There is currently an issue with percentage voucher where the adjustment keeps getting recalculated during checkout, resulting in the wrong amount for the adjustment. As far as I can tell it's due to Spree::Adjustment.update_adjustment! being called each time @order.update_order! is called. So If the order.total has been updated to take in to account the voucher adjustment, any new call to Spree::Adjustment.update_adjustment!(calculable = nil, force: false) will update the voucher adjustment amount, because the calculation is based on the order.total see Voucher#compute_amount
Potential fix :

prevent voucher adjustment from being updated in Spree::Adjustment.update_adjustment!
refactor Voucher#compute_amount so it doesn't calculate the voucher amount based on the order.total, and maybe move this calculation in VoucherAdjustmentsService

TODO:

system spec for percentage voucher

rioug · 2023-05-12T01:58:43Z

From @mkllnk on slack :

refactor Voucher#compute_amount so it doesn't calculate the voucher amount based on the order.total, and maybe move this calculation in VoucherAdjustmentsService

This sounds like the long-term solution that also works with calculating tax adjustments for mixed in-ex-cluded taxes.

drummer83 · 2023-05-19T15:41:00Z

This is unblocked now since #10761 has been merged.

rioug · 2023-06-27T06:15:26Z

Reviewers new commit starts here b461dfd

mkllnk

WIP - didn't finish the review. I'll continue tomorrow.

db/migrate/20230508035306_add_type_to_vouchers.rb

mkllnk · 2023-06-28T06:59:01Z

spec/models/spree/order_spec.rb

@@ -1439,7 +1439,7 @@ def advance_to_delivery_state(order)
  describe "#voucher_adjustments" do
    let(:distributor) { create(:distributor_enterprise) }
    let(:order) { create(:order, user: user, distributor: distributor) }
-    let(:voucher) { create(:voucher, code: 'new_code', enterprise: order.distributor) }
+    let(:voucher) { create(:voucher_flat_rate, code: 'new_code', enterprise: order.distributor) }


You could have kept the voucher factory as an alias for specs which don't care.

app/models/voucher.rb

mkllnk

Nice work. I have a few comments but nothing blocking this.

app/services/voucher_adjustments_service.rb

spec/system/consumer/split_checkout_tax_not_incl_spec.rb

filipefurtad0 · 2023-07-13T14:34:01Z

~~Hey, there are some conflicts blocking testing on this one. Ping @rioug.~~
Apologies - this one is still in code review, my mixup 🙈

rioug · 2023-07-17T01:35:25Z

New commit starts here cb54397

mkllnk

Nice refactoring!

I have some comments but nothing blocking the delivery of this part.

app/models/vouchers/percentage_rate.rb

mkllnk · 2023-07-17T05:22:31Z

app/models/vouchers/percentage_rate.rb

    def compute_amount(order)
-      percentage = amount / 100
-      -percentage * order.pre_discount_total
+      rate(order) * order.pre_discount_total
+    end


I wonder if we should add compute_tax_amount to the model as well... or take this out of the model. But then this is to comply with the originator interface of the Spree adjustments, right? Nevermind...

Yes the compute_amount is there to comply with the originator interface, see CalculatedAdjustments concern. That said, I believe voucher adjustments are decoupled from other adjustment lifecycle, so I think we "should be able" to take it out of the model.

mkllnk · 2023-07-17T05:24:42Z

app/controllers/admin/vouchers_controller.rb

+      case params[:voucher][:voucher_type]
+      when "Vouchers::FlatRate"
+        @voucher =
+          Vouchers::FlatRate.create(permitted_resource_params.merge(enterprise: @enterprise))
+      when "Vouchers::PercentageRate"
+        @voucher =
+          Vouchers::PercentageRate.create(permitted_resource_params.merge(enterprise: @enterprise))
+      else
+        @voucher.errors.add(:type)
+        return render_error
+      end


You could constantize the type string after checking it against the known types. Maybe worth if we add more types.

something like ?

voucher_types = ["Vouchers::FlatRate", "Vouchers::PercentageRate"] if voucher_types.include?(params[:voucher][:voucher_type]) @voucher = params[:voucher][:voucher_type].constantize.create(permitted_resource_params.merge(enterprise: @enterprise)) else @voucher.errors.add(:type) return render_error end

@mkllnk I think this is just waiting on your re-review of the fix for this comment (see newer commit 229ae91)

app/controllers/admin/vouchers_controller.rb

db/migrate/20230508035306_add_type_to_vouchers.rb

app/controllers/admin/vouchers_controller.rb

rioug · 2023-08-07T23:54:50Z

We need to wait on this #11117 before merging, but yes we can create issues for the remaining problems. It'd be good to at least fix the error 500.

rioug · 2023-08-08T06:19:30Z

I have a fix for the 500 error but I kept it on a separate branch, I think this is big enough as it is 😄

drummer83 · 2023-08-08T19:15:49Z

@rioug I have created the following issues as an outcome of this testing a discussion:
#11362
#11363
#11364
wishlist openfoodfoundation/openfoodnetwork#11666

drummer83 · 2023-08-10T18:53:06Z

Hi @rioug,
#11117 has been merged, so I guess we can move on and work on the issues I have opened (see post above).
Shall we resolve the conflicts and merge this one here?
Thanks!

It is important that the calculated voucher adjustments don't change if they are recalculated and it is equally important that they are updated if the order has changed

And update related specs voucher_type doesn't do anything for now.

Plus specs

It include calculation for order with taxes included in the price

Add calculation when tax is not included in price

Similar to tax included in price scenario, adds a test for percentage based voucher to check the adjustments are recalculated when needed. Plus fix tax incluced in price specs to use new factory

Refactor voucher to use a single table inheritance. It will simplify the code and remove a bunch of conditional

We are taking advantage of having a FlatRate and a PercentageRate model to simplify the code a little

We now check against known type to instanciate the correct voucher instead of using a case

Co-authored-by: David Cook <david@redcliffs.net>

Add explicit 'order.item_total' to make specs more readable

It triggers a Brakeman error that can be safely ignored

rioug · 2023-08-11T06:06:40Z

Done ! Thanks for creating the issues @drummer83 !

sigmundpetersen assigned rioug May 9, 2023

sigmundpetersen changed the title ~~[Vouchers], percentage rate~~ [Vouchers] Percentage rate May 16, 2023

rioug force-pushed the 10727-vouchers-percentage-rate branch from 4fb198c to 2a85ddc Compare June 12, 2023 03:23

rioug force-pushed the 10727-vouchers-percentage-rate branch from 2a85ddc to 60b4b0c Compare June 27, 2023 04:57

rioug marked this pull request as ready for review June 27, 2023 06:15

mkllnk reviewed Jun 28, 2023

View reviewed changes

mkllnk approved these changes Jun 28, 2023

View reviewed changes

app/services/voucher_adjustments_service.rb Outdated Show resolved Hide resolved

spec/system/consumer/split_checkout_tax_not_incl_spec.rb Outdated Show resolved Hide resolved

rioug force-pushed the 10727-vouchers-percentage-rate branch 5 times, most recently from 385d087 to 85e1387 Compare July 4, 2023 05:47

rioug mentioned this pull request Jul 10, 2023

[vouchers] error moving between summary and cart pages #11117

Merged

sigmundpetersen added the priority We focus on this issue right now label Jul 12, 2023

filipefurtad0 added feedback-needed and removed feedback-needed labels Jul 13, 2023

rioug force-pushed the 10727-vouchers-percentage-rate branch 4 times, most recently from b04be5f to cc75d36 Compare July 17, 2023 01:24

mkllnk approved these changes Jul 17, 2023

View reviewed changes

rioug force-pushed the 10727-vouchers-percentage-rate branch from cc75d36 to 7e2b104 Compare July 18, 2023 03:26

github-advanced-security bot found potential problems Jul 18, 2023

View reviewed changes

app/controllers/admin/vouchers_controller.rb Fixed Show fixed Hide fixed

rioug force-pushed the 10727-vouchers-percentage-rate branch from 7e2b104 to e803980 Compare July 18, 2023 03:43

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Aug 8, 2023

drummer83 mentioned this pull request Aug 8, 2023

[Vouchers] Error 500 (snail) when accessing /admin/enterprises/ENTERPRISE-NAME/edit (staging AU) #11319

Closed

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Aug 9, 2023

drummer83 removed their assignment Aug 9, 2023

rioug added 10 commits August 11, 2023 14:19

Add specs to cover re calculation

1be8daa

It is important that the calculated voucher adjustments don't change if they are recalculated and it is equally important that they are updated if the order has changed

Add voucher_type to voucher

619285a

And update related specs voucher_type doesn't do anything for now.

Add voucher type to admin screen

cc9069e

Plus specs

Add calculation for percentage voucher

cdb33aa

It include calculation for order with taxes included in the price

Finish calculation for percentage voucher

70bd714

Add calculation when tax is not included in price

Add system specs for percentage based voucher

959e230

Similar to tax included in price scenario, adds a test for percentage based voucher to check the adjustments are recalculated when needed. Plus fix tax incluced in price specs to use new factory

Refactor voucher: 1 base voucher class

204f393

Refactor voucher to use a single table inheritance. It will simplify the code and remove a bunch of conditional

Refactor voucher: 2 FlatRate voucher

def594a

Refactor voucher: 3 percentage rate voucher

46e04ca

Fix admin pages to work with refactored vouchers

29a3846

rioug force-pushed the 10727-vouchers-percentage-rate branch from 9e7e987 to 7a563cd Compare August 11, 2023 05:25

rioug and others added 7 commits August 11, 2023 15:41

Refactor VoucherAdjustment service

2828bd0

We are taking advantage of having a FlatRate and a PercentageRate model to simplify the code a little

Remove left over comment

e8b374d

Fix Rubocop warning

3f63cfb

Per review , Simplify creation of Voucher

e6b53c0

We now check against known type to instanciate the correct voucher instead of using a case

Use a better describe

054eac0

Co-authored-by: David Cook <david@redcliffs.net>

Per review, clean up voucher specs

9c9a623

Add explicit 'order.item_total' to make specs more readable

Add a comment around the use of safe_constantize

a2def24

It triggers a Brakeman error that can be safely ignored

rioug force-pushed the 10727-vouchers-percentage-rate branch from 7a563cd to a2def24 Compare August 11, 2023 05:41

rioug merged commit 61e6d55 into openfoodfoundation:master Aug 11, 2023
50 checks passed

drummer83 mentioned this pull request Dec 22, 2023

[Vouchers] Improve presentation of tax discounts in order summary for tax incl. in price openfoodfoundation/wishlist#494

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vouchers] Percentage rate #10821

[Vouchers] Percentage rate #10821

rioug commented May 9, 2023 •

edited

rioug commented May 12, 2023

rioug commented May 12, 2023

drummer83 commented May 19, 2023

rioug commented Jun 27, 2023

mkllnk left a comment

mkllnk Jun 28, 2023

mkllnk left a comment

filipefurtad0 commented Jul 13, 2023 •

edited

rioug commented Jul 17, 2023

mkllnk left a comment

mkllnk Jul 17, 2023

rioug Jul 18, 2023

mkllnk Jul 17, 2023

rioug Jul 18, 2023

dacook Jul 27, 2023

rioug commented Aug 7, 2023

rioug commented Aug 8, 2023

drummer83 commented Aug 8, 2023

drummer83 commented Aug 10, 2023

rioug commented Aug 11, 2023

[Vouchers] Percentage rate #10821

[Vouchers] Percentage rate #10821

Conversation

rioug commented May 9, 2023 • edited

What? Why?

What should we test?

Payment fees and voucher

Release notes

Dependencies

Documentation updates

rioug commented May 12, 2023

rioug commented May 12, 2023

drummer83 commented May 19, 2023

rioug commented Jun 27, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jun 28, 2023

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jul 13, 2023 • edited

rioug commented Jul 17, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jul 17, 2023

Choose a reason for hiding this comment

rioug Jul 18, 2023

Choose a reason for hiding this comment

mkllnk Jul 17, 2023

Choose a reason for hiding this comment

rioug Jul 18, 2023

Choose a reason for hiding this comment

dacook Jul 27, 2023

Choose a reason for hiding this comment

rioug commented Aug 7, 2023

rioug commented Aug 8, 2023

drummer83 commented Aug 8, 2023

drummer83 commented Aug 10, 2023

rioug commented Aug 11, 2023

rioug commented May 9, 2023 •

edited

filipefurtad0 commented Jul 13, 2023 •

edited