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

Invoice print spec: added tax #9495

Merged
merged 7 commits into from
Aug 9, 2022

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Jul 29, 2022

What? Why?

Relates to #7983.

Aim is to capture the current behavior for on what concerns the displaying of taxes on both legacy and alternative invoices.

  • changes the order and product factories to consider added (not-included) tax rates/amounts

  • fixes broken specs due to factory change

  • adds 4 test cases around these 4 invoices/test cases - displaying below for clarity:

    • included tax, legacy invoice
    • included tax, alternative invoice
    • added tax, legacy invoice
    • added tax, alternative invoice

image

Yellow boxes refer to issue #7983.

Pending examples have been numbered i) to v) and referenced in the pic above and spec.

What should we test?

Green build.

Release notes

Changelog Category: Technical changes

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

Dependencies

Relates to #7983.

Documentation updates

@filipefurtad0 filipefurtad0 self-assigned this Jul 29, 2022
@filipefurtad0 filipefurtad0 marked this pull request as draft July 29, 2022 19:07
@filipefurtad0 filipefurtad0 force-pushed the invoice_print_spec branch 2 times, most recently from 6c4a8ff to fade23e Compare August 2, 2022 15:54
@filipefurtad0 filipefurtad0 marked this pull request as ready for review August 2, 2022 16:18
Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this one @filipefurtad0 !
It's very valuable to have this kind of spec, on which we can rely on in case of changing something. Thanks! 🙏
I reviewed your PR, and didn't find anything wrong, but i must admit that it's difficult to read/understand all those values (GST, taxes, price, ...). Anyway, that's a (wonderful) starting point!

What's your point of view on how this can interact with #7983 ?

@jibees jibees requested a review from mkllnk August 5, 2022 09:22
@filipefurtad0
Copy link
Contributor Author

Thanks for the kind review 🙌

Yes, It's very difficult to read/follow the values... I've tried to simplify but it was quite challenging 😅 so I ended up taking the data setup from here.

The set up is repeated for two distinct orders (order1, order2) which corresponds to the two different tax scenarios (respectively: included tax, added tax). Many lines of code, again, maybe possible to simplify - but this way we are sure we have independent setups.

What's your point of view on how this can interact with #7983 ?

There are pending examples should correspond to the yellow blocks on the pictures. Specifically, the pending "closing #7983" can be changed to pass when the issue is addressed.

The other pending examples relate to the fact that $0.0 tax are currently not displayed for line items or enterprise fees, but I was not sure this was under scope of #7983.

I totally agree this is a starting point - Surely it can be improved.

@filipefurtad0
Copy link
Contributor Author

filipefurtad0 commented Aug 5, 2022

Hey @jibees ,

I've updated the picture above and added some other pending examples on the spec: they should match now. Hope this makes it easier to read 🙆‍♂️

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

spec/features/admin/invoice_print_spec.rb Show resolved Hide resolved
@mkllnk mkllnk merged commit 43e4047 into openfoodfoundation:master Aug 9, 2022
@filipefurtad0
Copy link
Contributor Author

This spec is introducing flakiness, on the calculation of the total order tax - the values below are incorrect:

image

But this is not a real bug (from staging-FR) - correct tax total values:

image

I think this is caused by not explicitly loading app/helpers/checkout_helper.rb

  def display_checkout_tax_total(order)
    Spree::Money.new order.total_tax, currency: order.currency
  end

@filipefurtad0
Copy link
Contributor Author

I'm blocked on this seemingly flaky spec. No idea why the invoice is not rendered correctly on the test environment, but correcltly in staging.

Opened this issue #9556.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants