Add payment details to invoice #5712

mbudm · 2020-07-01T02:18:53Z

What? Why?

Add more detail on payment history to invoices. This helps to clarify the invoice when the status is balance due or credit owing.

Concerns/possible changes are:

the payment partial is in the order_mailer view. The change will apply to other templates using this partial (emailed invoices?) I need to clarify if this expected/desired.
I added the css for the payments table to the same scss file where the invoice table is defined, app/assets/stylesheets/mail/email.scss which feels a bit like a dumping ground - should we create a separate scss file for the invoice styles?
I'm not sure about the column alignment, I've suggested in the issue that the amount column is moved to the right of the payment method column, for better balance and easier scanning of the figures
If there are no payments, should the info currently added to the invoice be shown? Also I'm unsure if there are ever orders without payments, as a checkout must occur for an order to appear?

What should we test?

Detailed steps to recreate are in the issue: #5208

create an order that is balance due or credit owing
print the invoice
the balance due or credit owing state is shown, including the amount. the payment history is also shown

A paid (completed) order - with a failed credit card attempt:

Release notes

Changed order invoices where balance owing or credit due, with details on payment history

Changelog Category: Changed

sigmundpetersen · 2020-07-01T08:01:30Z

FYI those failing enterprise_controller specs are also in master and will be fixed by #5706 , so you shouldn't worry about them 👍

mbudm · 2020-07-01T10:17:32Z

Thanks @sigmundpetersen - I think the failing ./spec/mailers/subscription_mailer_spec.rb are on this change though so will review that tomorrow

luisramos0

Nice one!

I think it's messy that the invoice templates are under orders, they should be a separate controller and views folder. And then it would be ok to have a dependency to the order_mailer... but this is not in scope for this PR.

I leave some comments below.

app/views/spree/admin/orders/_list.html.haml

app/views/spree/order_mailer/_payment.html.haml

app/assets/stylesheets/mail/email.scss

mbudm · 2020-07-07T12:34:22Z

Updated screenshots - changed the column order as discussed in the issue #5208 - invoice PDF:

Confirmation email:

And as it is affected by the addition of a helper, checking that the payments page is unchanged:

mbudm · 2020-07-07T12:37:21Z

app/helpers/order_helper.rb

@@ -4,4 +4,8 @@ module OrderHelper
  def last_payment_method(order)
    OrderPaymentFinder.new(order).last_payment&.payment_method
  end
+
+  def outstanding_balance_label(order)


@luisramos0 you mentioned extracting this logic to a service but it looked to me like it's more a helper type of thing...?

I understand if you call it a label, I think this is more like a state. This the balance state of the order. This is something that will/could be needed in different places (the api for example) not just views. I see it as business logic that belongs in the service layer.
I think it's important we move all logic to services out of the MVC. This is something we have been doing lately in OFN and it's really good that we have thinner helpers/controllers/models and an organized service layer.
Anyway, I think this is an acceptable solution!

mbudm · 2020-07-07T12:37:55Z

app/mailers/spree/order_mailer_decorator.rb

@@ -3,6 +3,7 @@
  helper CheckoutHelper
  helper SpreeCurrencyHelper
  helper OrderHelper
+  helper Spree::Admin::PaymentsHelper


Not sure if there's an issue with bringing in yet more helpers here

mbudm · 2020-07-07T12:47:24Z

Almost forgot invoice2:

luisramos0

Great!

Should I move this partial to app/views/spree/admin/shared?

app/views/spree/shared is fine 👍 ideally I'd prefer to see it outside the spree space, app/views/shared or app/views/admin/shared. Anyway, it's good as is 👍

I'm pretty sure we don't want the Spree based pretty_time method in more places, is there an alternate time formatter/technique we prefer that doesn't depend on Spree?

I dont know what's the best way to format a time BUT I dont see a problem of using spree code, as part of "bye bye spree" we will bring all the code we need from spree. base_helper is still on the spree side with a decorator on our side but we can definitely bring pretty_time to OFN and even improve it. I'd say let's use pretty time and improve it.

why is the order_mailer outside the admin dir anyway? Isn't it part of the orders admin form?

I am not sure why but all the spree mailers are outside admin. They are part of spree_core not spree_backend (which really is spree/admin).

luisramos0 · 2020-07-07T16:38:27Z

app/helpers/order_helper.rb

@@ -4,4 +4,8 @@ module OrderHelper
  def last_payment_method(order)
    OrderPaymentFinder.new(order).last_payment&.payment_method
  end
+
+  def outstanding_balance_label(order)


I understand if you call it a label, I think this is more like a state. This the balance state of the order. This is something that will/could be needed in different places (the api for example) not just views. I see it as business logic that belongs in the service layer.
I think it's important we move all logic to services out of the MVC. This is something we have been doing lately in OFN and it's really good that we have thinner helpers/controllers/models and an organized service layer.
Anyway, I think this is an acceptable solution!

app/assets/stylesheets/mail/payment_list.scss

luisramos0

oh, the build still has some relevant broken specs.

mbudm · 2020-07-07T23:45:58Z

oh, the build still has some relevant broken specs.

OK cool - I let them all run last night after doing some adhoc test runs locally, wasn't sure if the /spec/mailers/subscription_mailer_spec.rb was correlation or causation, i.e it was the right area but I wasn't clear how these changes had affected it - will investigate more deeply today.

EDIT: Yay for tests. The change moving payments partial to shared has wider impact that I first realised, so this is something we'd need to add in the test plan.

Impacted emails are:

app/views/subscription_mailer/confirmation_email.html.haml
app/views/subscription_mailer/placement_email.html.haml
app/views/spree/order_mailer/confirm_email_for_shop.html.haml
app/views/spree/order_mailer/confirm_email_for_customer.html.haml

Impacted invoice pdfs are:

app/views/spree/admin/orders/invoice2.html.haml
app/views/spree/admin/orders/invoice.html.haml

mbudm · 2020-07-08T02:35:59Z

One remaining failure on semaphore passes locally: spec/models/variant_override_spec.rb

Reran the build and it failed again. Locally it works reliably - not sure what to do here.

luisramos0

yeah, that's it, well done!
We need to fix these flaky specs, not on this PR....

filipefurtad0 · 2020-07-19T22:36:08Z

Hi @mbudm ,

I verified the issue described and used the steps described in #5208 to test the PR.

I checked 4 cases, in which this information should be visible:

Bulk printing an invoice (pdf), on the Orders page (/admin/orders?q[s]=completed_at+desc)
the functions under order edit page, (/admin/orders/R422624534/edit)
- Print invoice (pdf)
- Send the invoice (pdf per email)
- Resend (order confirmation) (email, no pdf)

Verified that the generated pdf documents above were identical in all cases.

By printing/sending an invoice of a complete/paid order: we can that the PR introduces information about the payment history:

Order confirmation per email:

PDF-invoice:

An issue not introduced by this PR: It's noticeable from the pics above, that shipping instructions appear on the email with the order confirmation, but are missing from the PDF file.

Removing an item from an order which is paid for (left side, before this PR, right side after this PR)

From the changes introduced by this PR, I would expected that "Credit Owed (£-12.00)" information would appear, somewhere in the area marked above. This is also not appearing in the order confirmation (email).

Adding further items, and a second (insufficient) payment now appears as well:

This looks good for merging, thank you for these improvements!

Regarding the issues found:

Missing shipping instructions on PDF-invoices (an issue will be opened)
Missing Credit Owed (£-xx.xx) information on Email (order confirmation) and PDF-invoices: does this need a separate issue? Would it be papercut-size?

luisramos0 · 2020-07-21T13:53:11Z

Re the 2 issues:

Missing shipping instructions on PDF-invoices (an issue will be opened)

Why do you think this is an issue? is it not a feature request? are the delivery instructions necessary on the invoice? why?

Missing Credit Owed (£-xx.xx) information on Email (order confirmation) and PDF-invoices: does this need a separate issue? Would it be papercut-size?

I think this is a bug in this PR. I think it could be the translation label :credit_owner missing from the en.yml file.
I think we could let @mbudm had a quick look at this second point and if it's easy fix it in this PR. What do you think @filipefurtad0 ?

filipefurtad0 · 2020-07-21T17:59:32Z

Hey @luisramos0,

Re the 2 issues:

        Missing shipping instructions on PDF-invoices (an issue will be opened)

Why do you think this is an issue? is it not a feature request? are the delivery instructions necessary on the invoice? why?

That's correct, that would be a feature request - not a bug. Invoice and order confirmation don't necessarily need the same info. Thanks!

    Missing Credit Owed (£-xx.xx) information on Email (order confirmation) and PDF-invoices: does this need a separate issue? Would it be papercut-size?

I think this is a bug in this PR. I think it could be the translation label :credit_owner missing from the en.yml file.
I think we could let @mbudm had a quick look at this second point and if it's easy fix it in this PR. What do you think @filipefurtad0 ?

Sounds good to me, especially knowing it's an easy fix. Moving back to In Dev 👍

Thanks again Luis!

filipefurtad0 · 2020-07-21T18:03:42Z

Hey @luisramos0,

Re the 2 issues:
        Missing shipping instructions on PDF-invoices (an issue will be opened)
Why do you think this is an issue? is it not a feature request? are the delivery instructions necessary on the invoice? why?

That's correct, that would be a feature request - not a bug. Invoice and order confirmation don't necessarily need the same info. Thanks!

    Missing Credit Owed (£-xx.xx) information on Email (order confirmation) and PDF-invoices: does this need a separate issue? Would it be papercut-size?
I think this is a bug in this PR. I think it could be the translation label :credit_owner missing from the en.yml file.
I think we could let @mbudm had a quick look at this second point and if it's easy fix it in this PR. What do you think @filipefurtad0 ?

Sounds good to me, especially if looks like an easy fix. Would have a second look on it @mbudm? Moving back to In Dev 👍
Thanks again Luis!

Matt-Yorkley · 2020-08-20T08:44:44Z

Rebased, resolved conflicts, and the missing "credit owed" display should now be fixed.

spec/lib/spree/core/mail_interceptor_spec.rb

- Partial is used by both the invoice pdf and the order confirmation email - separate scss file for new payment list table - extracted outstanding balance logic (also changed in payments view.. admin/orders/RXXX/payments) - translations in shared.payments_list and lazy loaded

…t thought (yay tests)

The `Spree::Order#paid?` method actually includes orders with `payment_state == "credit_owed"`, which was breaking the desired display logic here.

app/views/spree/shared/_payment.html.haml

luisramos0 · 2020-08-21T10:48:11Z

I am moving to In Dev, I think it's worth doing the simple change I suggest now, ok Matt?
I think you can move to Test Ready directly afterwards 👍

filipefurtad0 · 2020-08-27T15:51:58Z

Thanks @Matt-Yorkley
the PR adds the "Credit Owed" info to the invoice:

Ready to go.

mbudm changed the title ~~Add more details on payments to invoice.~~ Add payment details to invoice Jul 1, 2020

mbudm linked an issue Jul 1, 2020 that may be closed by this pull request

Invoices display incorrect information about payments if order has been edited #5208

Closed

sigmundpetersen assigned mbudm Jul 1, 2020

luisramos0 suggested changes Jul 1, 2020

View reviewed changes

luisramos0 reviewed Jul 1, 2020

View reviewed changes

app/assets/stylesheets/mail/email.scss Outdated Show resolved Hide resolved

mbudm commented Jul 7, 2020

View reviewed changes

mbudm mentioned this pull request Jul 7, 2020

Invoices display incorrect information about payments if order has been edited #5208

Closed

luisramos0 approved these changes Jul 7, 2020

View reviewed changes

luisramos0 suggested changes Jul 7, 2020

View reviewed changes

luisramos0 approved these changes Jul 8, 2020

View reviewed changes

Matt-Yorkley approved these changes Jul 15, 2020

View reviewed changes

filipefurtad0 self-assigned this Jul 19, 2020

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 19, 2020

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 19, 2020

lin-d-hop unassigned mbudm Aug 18, 2020

sigmundpetersen unassigned filipefurtad0 Aug 18, 2020

Matt-Yorkley self-assigned this Aug 20, 2020

Matt-Yorkley force-pushed the issue/5208 branch from b606ac2 to fa7a6ef Compare August 20, 2020 08:20

luisramos0 reviewed Aug 20, 2020

View reviewed changes

spec/lib/spree/core/mail_interceptor_spec.rb Outdated Show resolved Hide resolved

Matt-Yorkley force-pushed the issue/5208 branch from 9a63a8e to 7929f86 Compare August 20, 2020 12:59

Steve Roberts and others added 9 commits August 20, 2020 17:16

Add more details on payments to invoice.

8e5cfde

Clean up scss - picked up by codeclimate

a00e964

add vars to email.scss

1f47083

Fix issues raised by linter

ac69bef

Fix some more issues raised by semaphore

c79f232

Update invoice2 to also use the shared payment partial

4243ff2

Fix tests - the move of payments to shared has wider impact than firs…

50fd365

…t thought (yay tests)

Adjust payment state conditional

07b819a

The `Spree::Order#paid?` method actually includes orders with `payment_state == "credit_owed"`, which was breaking the desired display logic here.

Matt-Yorkley force-pushed the issue/5208 branch from 7929f86 to 07b819a Compare August 20, 2020 16:17

mkllnk approved these changes Aug 21, 2020

View reviewed changes

luisramos0 reviewed Aug 21, 2020

View reviewed changes

app/views/spree/shared/_payment.html.haml Outdated Show resolved Hide resolved

Simplify conditional statement

2f1fdc5

filipefurtad0 self-assigned this Aug 27, 2020

filipefurtad0 added pr-staged-au staging.openfoodnetwork.org.au pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-au staging.openfoodnetwork.org.au labels Aug 27, 2020

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 27, 2020

mkllnk merged commit 819a445 into openfoodfoundation:master Aug 27, 2020

filipefurtad0 mentioned this pull request Jun 24, 2021

Missing shipping method description and "ready for" information on both PDF-invoice templates openfoodfoundation/wishlist#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add payment details to invoice #5712

Add payment details to invoice #5712

mbudm commented Jul 1, 2020 •

edited by sigmundpetersen

Loading

sigmundpetersen commented Jul 1, 2020 •

edited

Loading

mbudm commented Jul 1, 2020

luisramos0 left a comment

mbudm commented Jul 7, 2020

mbudm Jul 7, 2020

luisramos0 Jul 7, 2020

mbudm Jul 7, 2020

mbudm commented Jul 7, 2020 •

edited

Loading

luisramos0 left a comment •

edited

Loading

luisramos0 Jul 7, 2020

luisramos0 left a comment

mbudm commented Jul 7, 2020 •

edited

Loading

mbudm commented Jul 8, 2020 •

edited

Loading

luisramos0 left a comment

filipefurtad0 commented Jul 19, 2020 •

edited

Loading

luisramos0 commented Jul 21, 2020

filipefurtad0 commented Jul 21, 2020 •

edited

Loading

filipefurtad0 commented Jul 21, 2020 •

edited

Loading

Matt-Yorkley commented Aug 20, 2020

luisramos0 commented Aug 21, 2020

filipefurtad0 commented Aug 27, 2020

Add payment details to invoice #5712

Add payment details to invoice #5712

Conversation

mbudm commented Jul 1, 2020 • edited by sigmundpetersen Loading

What? Why?

What should we test?

Release notes

sigmundpetersen commented Jul 1, 2020 • edited Loading

mbudm commented Jul 1, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

mbudm commented Jul 7, 2020

mbudm Jul 7, 2020

Choose a reason for hiding this comment

luisramos0 Jul 7, 2020

Choose a reason for hiding this comment

mbudm Jul 7, 2020

Choose a reason for hiding this comment

mbudm commented Jul 7, 2020 • edited Loading

luisramos0 left a comment • edited Loading

Choose a reason for hiding this comment

luisramos0 Jul 7, 2020

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

mbudm commented Jul 7, 2020 • edited Loading

mbudm commented Jul 8, 2020 • edited Loading

luisramos0 left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jul 19, 2020 • edited Loading

luisramos0 commented Jul 21, 2020

filipefurtad0 commented Jul 21, 2020 • edited Loading

filipefurtad0 commented Jul 21, 2020 • edited Loading

Matt-Yorkley commented Aug 20, 2020

luisramos0 commented Aug 21, 2020

filipefurtad0 commented Aug 27, 2020

mbudm commented Jul 1, 2020 •

edited by sigmundpetersen

Loading

sigmundpetersen commented Jul 1, 2020 •

edited

Loading

mbudm commented Jul 7, 2020 •

edited

Loading

luisramos0 left a comment •

edited

Loading

mbudm commented Jul 7, 2020 •

edited

Loading

mbudm commented Jul 8, 2020 •

edited

Loading

filipefurtad0 commented Jul 19, 2020 •

edited

Loading

filipefurtad0 commented Jul 21, 2020 •

edited

Loading

filipefurtad0 commented Jul 21, 2020 •

edited

Loading