Include vouchers in report: Enterprise Fees With Tax Report By Order #11985

chahmedejaz · 2023-12-26T21:37:57Z

⚠️ ALL dev, code review, product and testing etc should use the Clockify code attached to this Epic #11922 - Citi OFN Vouchers Upgrade

What? Why?

Closes [Vouchers] [Reports] Include vouchers in report: Enterprise Fees With Tax Report By Order #11768

What should we test?

Please validate the Steps to Reproduce
Along with above, please make sure that the report in question is working as expected without vouchers as well

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

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

mkllnk

Good work!

mkllnk · 2024-01-03T01:17:47Z

app/models/spree/order.rb

+      return BigDecimal(0) unless (voucher_adjustment = voucher_adjustments.take)
+
+      voucher = voucher_adjustment.originator


I wonder if this can be better expressed as AR association. has_many :vouchers, through: :voucher_adjustments should work. Maybe even has_one?

Yes it's the best scenario for the situation at hand.
I'd go for the has_many because this would help us incorporate multiple vouchers if in future we have this feature without making any schema level changes.

We can have an issue to incorporate this comment and use this relationship wherever required. What do you say?

I think it's fine to do it in this issue. I don't think we had a need for it so far so that's probably why it hasn't been done, and yes we should use has_many.

mkllnk · 2024-01-03T01:18:08Z

app/models/spree/order.rb

+    # @return [BigDecimal] The rate of the voucher if applied to the order
+    def applied_voucher_rate
+      # As an order can have only one voucher, hence using +take+ because each voucher adjustment will have the same voucher
+      return BigDecimal(0) unless (voucher_adjustment = voucher_adjustments.take)


Assignment within a conditional is hard to read. I would split this.

mkllnk · 2024-01-03T01:21:10Z

lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_order.rb

+
+        def apply_voucher_on_amount(order, amount)
+          rate = order.applied_voucher_rate
+          amount + (amount*rate)


Mathematically, this can be simplified to amount * (1 + rate).

mkllnk · 2024-01-03T01:22:00Z

spec/models/spree/order_spec.rb

@@ -1544,4 +1544,50 @@ def advance_to_delivery_state(order)
      expect(order.voucher_adjustments).to eq(expected_adjustments)
    end
  end
+
+  describe '#applied_voucher_rate' do


For future reference: it's better to include the spec in the same commit as the code change needing the spec.

mkllnk · 2024-01-03T01:24:04Z

lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_order.rb

+        def apply_voucher_on_amount(order, amount)
+          rate = order.applied_voucher_rate
+          result = amount + (amount * rate)
+          BigDecimal(result.to_s).round(2, BigDecimal::ROUND_HALF_UP)


It's a shame that we have to round. Unfortunately, there's no better way with the current data structure of vouchers. But I'm pretty sure that we will have rounding errors one day.

Yeah I was also thinking that we might face the data round off issues. I was wondering how we can avoid this thing. 😅

The only solution I can think of is a complete re-modelling of the voucher adjustments. Instead of adjusting the whole order, we could adjust each line item. The total of all voucher adjustments could still be different to the rate applied to the whole order but that's easier to explain. It could also be easier to use those adjustments in reports like this one. But it's a big chunk of work that I wouldn't start right now. Gaetan and Lynne also expressed their concerns over starting this work while there's not big reason for it.

rioug · 2024-01-04T00:38:28Z

spec/models/spree/order_spec.rb

+
+        it 'returns the BigDecimal 0 value' do
+          actual = order.applied_voucher_rate
+          expect(actual.class).to eq(BigDecimal)


I would use be_kind_of(BigDecimal) matcher, but this works as well.

TIL: be_kind_of matcher. Thanks :)

There's also be_a doing the same.

rioug · 2024-01-04T00:40:25Z

app/models/spree/order.rb

+      # As an order can have only one voucher,
+      # hence using +take+ as each voucher adjustment will have the same voucher


Note for next time, it's better to fix Rubocop issue in a separate commit.

rioug · 2024-01-04T00:44:04Z

app/models/spree/order.rb

+      return BigDecimal(0) unless (voucher_adjustment = voucher_adjustments.take)
+
+      voucher = voucher_adjustment.originator


I think it's fine to do it in this issue. I don't think we had a need for it so far so that's probably why it hasn't been done, and yes we should use has_many.

dacook

It looks like consensus is to change to an AR association like has_many :vouchers, through: :voucher_adjustments

dacook · 2024-01-09T23:04:46Z

lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_order.rb

          adjustment_ids = enterprise_fee_adjustment_ids(query_result_row)
          query = Spree::Adjustment.tax
          query = query.where(included: true) unless included.nil?
          query = query.where(originator_id: tax_rate_id(query_result_row)) unless all == true
-          query.where(order_id:)
+          tax_amount = query.where(order_id:)


We could actually remove the order_id variable and refer directly to order. AR automatically uses the ID anyway.

That's great! 😍

It's updated here: 1d44a3f

chahmedejaz · 2024-01-10T09:49:04Z

It looks like consensus is to change to an AR association like has_many :vouchers, through: :voucher_adjustments

Just to re-iterate, this should be handled in a separate issue as we may need to replace code in many other places which might be out of the issue's scope. Thanks. :)
ref: #11985 (comment)

dacook

Ok, I didn't realise it was a big change.

drummer83 · 2024-01-11T19:13:18Z

Hi @chahmedejaz,
I have tested this on staging AU today. It's looking good, but there is one thing I don't understand.
For orders with tax incl. in price there are somehow additional 0.08 $ displayed in the report. However, it doesn't show up in the summary row.

To calculate the numbers (maybe I'm wrong):

the enterprise fee is 10 $ incl. tax
the voucher is a 10 % voucher
tax: 10 $ = 9.09 $ fee + 0.91 $ tax
voucher: 10 % = 0.91 $ fee discount + 0.09 $ tax discount
total: 9 $ = 8.18 $ fee + 0.82 $ tax

Can you find out where those additional 8 ct. in the report are coming from? I don't think it's correct. But maybe you can convince me that it is. 😉

I will move this back to In Dev so you can check this is more detail.

Here are the order details:

Report setting for reference:

Thanks!

- For applied_voucher_rate method in order

- Fix lint issues

chahmedejaz · 2024-01-18T21:04:41Z

It's looking good, but there is one thing I don't understand.
For orders with tax incl. in price there are somehow additional 0.08 $ displayed in the report. However, it doesn't show up in the summary row.

Hey @drummer83 - Sorry for getting back on this late. Yeah, you are right. I've fixed this issue. It should be good now.
Thanks for raising this one ❤️

Hey @dacook, @mkllnk - Can you please review this small fix: ff48825 Thanks ❤️

dacook

Great work on picking that up Konrad, and for fixing it Ahmed!

drummer83 · 2024-01-24T21:57:38Z

Thanks, @chahmedejaz!

Yes, it's looking good now. Same orders, same report, correct numbers. 👍

Merging! 🎉
🚀

chahmedejaz changed the title ~~11768: apply voucher~~ 11768: Include vouchers in report: Enterprise Fees With Tax Report By Order Dec 26, 2023

chahmedejaz changed the title ~~11768: Include vouchers in report: Enterprise Fees With Tax Report By Order~~ Include vouchers in report: Enterprise Fees With Tax Report By Order Dec 26, 2023

chahmedejaz marked this pull request as ready for review December 27, 2023 10:53

mkllnk approved these changes Jan 3, 2024

View reviewed changes

sigmundpetersen assigned chahmedejaz Jan 3, 2024

rioug reviewed Jan 4, 2024

View reviewed changes

dacook requested changes Jan 9, 2024

View reviewed changes

mkllnk approved these changes Jan 10, 2024

View reviewed changes

mkllnk requested a review from dacook January 10, 2024 21:30

dacook approved these changes Jan 10, 2024

View reviewed changes

drummer83 self-assigned this Jan 11, 2024

drummer83 added no-staging-AU A tag which does not trigger deployments, indicating a server is being used pr-staged-au staging.openfoodnetwork.org.au and removed no-staging-AU A tag which does not trigger deployments, indicating a server is being used labels Jan 11, 2024

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jan 11, 2024

drummer83 removed their assignment Jan 11, 2024

chahmedejaz added 5 commits January 19, 2024 01:52

11768: apply voucher

38b7616

11768: add specs

a3592cb

- For applied_voucher_rate method in order

11768: add roundoff

cb9c296

- Fix lint issues

11768: replace order_id with order

3a8e4d2

11768: fix the amount with voucher

ff48825

chahmedejaz force-pushed the task/11768-add-voucher-in-enterprise-fees-with-tax-by-order-report branch from 0c0c9c7 to ff48825 Compare January 18, 2024 20:53

mkllnk approved these changes Jan 18, 2024

View reviewed changes

dacook approved these changes Jan 18, 2024

View reviewed changes

drummer83 self-assigned this Jan 24, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jan 24, 2024

drummer83 merged commit ca12eab into openfoodfoundation:master Jan 24, 2024
52 checks passed

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jan 24, 2024

mkllnk added the user facing changes Thes pull requests affect the user experience label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include vouchers in report: Enterprise Fees With Tax Report By Order #11985

Include vouchers in report: Enterprise Fees With Tax Report By Order #11985

chahmedejaz commented Dec 26, 2023 •

edited by rioug

mkllnk left a comment

mkllnk Jan 3, 2024

chahmedejaz Jan 3, 2024

rioug Jan 4, 2024

mkllnk Jan 3, 2024

mkllnk Jan 3, 2024

mkllnk Jan 3, 2024

mkllnk Jan 3, 2024

chahmedejaz Jan 3, 2024

mkllnk Jan 3, 2024

rioug Jan 4, 2024

chahmedejaz Jan 4, 2024

mkllnk Jan 4, 2024

rioug Jan 4, 2024

rioug Jan 4, 2024

dacook left a comment

dacook Jan 9, 2024

chahmedejaz Jan 10, 2024

chahmedejaz commented Jan 10, 2024

dacook left a comment

drummer83 commented Jan 11, 2024

chahmedejaz commented Jan 18, 2024

dacook left a comment

drummer83 commented Jan 24, 2024

		return BigDecimal(0) unless (voucher_adjustment = voucher_adjustments.take)

		voucher = voucher_adjustment.originator

		# As an order can have only one voucher,
		# hence using +take+ as each voucher adjustment will have the same voucher

Include vouchers in report: Enterprise Fees With Tax Report By Order #11985

Include vouchers in report: Enterprise Fees With Tax Report By Order #11985

Conversation

chahmedejaz commented Dec 26, 2023 • edited by rioug

What? Why?

What should we test?

Release notes

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahmedejaz commented Jan 10, 2024

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Jan 11, 2024

chahmedejaz commented Jan 18, 2024

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Jan 24, 2024

chahmedejaz commented Dec 26, 2023 •

edited by rioug