Invoices

[abdellani]

What? Why?

• Closes #

related topics
#10724
#10725

What should we test?

• enable invoices on the features toggle
• check the invoice on the order details page.
  
• generate a new invoice.
• check the invoices list.
• try to update the order, then try to generate the invoice. Depending on the update (please check this document):
  • a new invoice will be generated.
  • the latest invoice will be updated.

Release notes

Changelog Category: User facing changes | Technical changes

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

Dependencies

Documentation updates

[abdellani]

Hi @openfoodfoundation/developers @openfoodfoundation/testers

I wrote a draft of one possible way to implementation of the new requirements related to the invoices.

I added a new model Invoice

• order_id: reference to the order
• status: can be active or canceled (from what I can see on As an enterprise admin, I can print and send invoices on the edit order page #10725)
• Number: Number of the invoice ( I still don't know if it's just a number, or if it has a format for example 16-001)
• position: to sort the invoices (maybe we can use created_at instead ?)
• data: will store the data used to render the invoice.

Questions:

1. Is there something that needs to change in the model?
2. Is it better to use Invoice or Spree::Invoice as a name for the model?
   Spree::Admin::BaseController expects the order's associated models to start with Spree
   

   openfoodnetwork/app/controllers/spree/admin/base_controller.rb

   Lines 34 to 39 in 2daa494

   	def model_class
   	const_name = controller_name.classify
   	return "Spree::#{const_name}".constantize if Spree.const_defined?(const_name)
   	nil
   	end

I did a quick search for the code used for printing, I found that we trigger printing from 3 different parts (please correct me if I'm wrong):

1. Spree::Admin::OrdersController#print:

   • Called when the user clicks Print Invoice on the Order's edit page.
     

2. Spree::Admin::OrdersController#invoice (relais on OrderMailer):

   • Called when the user clicks Send Invoice on the Order's edit page.

3. Spree::Admin::InvoicesController#create (Bulk Invoice):

   • Called when multiple orders are selected
     

This diagram when how classes' dependencies:

We'll need to add a new endpoint (or edit an existing one) to let the user generate a specific version of an invoice.

Questions:

1. Is there a need to send old invoices by email?
2. what will be the default behavior if an order doesn't have any invoices? (bulk invoice, sending order)
3. In the case of the bulk invoice, do use the latest invoice version for every order? or do we give the possibility to select a specific version for some orders?
4. what are the different invoice statuses? (from the screenshot: 'active', and 'canceled', but are there others?)
5. How is the invoice status connected to the order status (and shipment, payment, etc...)? for example: if the order is canceled, will this move the invoice to canceled status?

[RachL]

@abdellani thank you !! Here are some answer:

I still don't know if it's just a number, or if it has a format for example 16-001)

So far we chose to handle numbering in the future, so we would just keep order number for now (like we do today). Otherwise it legally needs to be sequential number per shop.

This is the example I've put in the epic description:

JB orders at Filipe shop
Filipe generates an invoice to JB. Let's say this invoice is number 30
Rachel orders at Filipe. when Filipe invoice Rachel, this invoice has number 31
Let's say JB now ask Filipe to make changes on the order (add a product for example)
Filipe generates a new order for JB: this invoice has now the number 32 if the order was unpaid. If JB had already paid, two invoices are generated: one credit invoice with number 32 (with a mention that this credits invoice 30) and one new invoice with number 33.
Ideally, we would allow each hub to set an invoice prefix (in business details for example) and the number at which their invoices should start. From there, all invoices from this enterprise have the same prefix and a sequential number.

--

Is there a need to send old invoices by email?

What do you mean by "old" invoices. The ones we have currently? As they are not stored this work will completely replace the old invoice format.

what will be the default behavior if an order doesn't have any invoices?

Bulk invoice printing needs to generate the invoice if there isn't any. I would say same for bulk sending. However note the first PR should only focus on the edit order page. As we are working with a feature toggle we can work bits by bits.

In the case of the bulk invoice, do use the latest invoice version for every order? or do we give the possibility to select a specific version for some orders?

When bulk invoice, we need to generate invoices, which means:

• if the order has changed, we will need to cancel the previous invoice and create a new one
• if the order has not changed, we generate an invoice if there wasn't any, or we use the one already there

what are the different invoice statuses?

A bit linked to the question I've asked here: #10725
If capturing a payment would pass the invoice in a "paid" status. I think we would have paid/unpaid/cancelled. Active would be replace by paid or unpaid. What do you think?

How is the invoice status connected to the order status (and shipment, payment, etc...)? for example: if the order is canceled, will this move the invoice to canceled status?

Shipment is not displayed currently on the invoice I think, so we can leave it aside. If the order is canceled, it depends on the payment status.
If the order is unpaid, the invoice is cancelled.
If the order was paid (partially or totally) - these are the test cases 8 and 9 I've tried to describe in 401, but perhaps I've forgotten something:

8. Generate a new invoice : if the order is unpaid, check the previous one is cancelled. If the order is paid, check there are now 3 lines in the table / 3 files available for download : 1. The previous invoice cancelled 2. The credit invoice 3. The new invoice with new amount

9. Credit invoices are issued when the full amount of the original order must be refunded. But there is also the case of the partial refund: an item gets removed for example and a refund must be done. In this case, the amount the order is reduced from must be displayed as line item and order total be re-calculated.

So credit invoices cannot be paid, they can only be refunded. Do we want to add a status for this?

This makes me realize that maybe just working on the edit page is too big. Maybe we should divide the work even more and work with one issue per action (delete product, capture payment, partial refunds...)?

[mkllnk]

position: to sort the invoices (maybe we can use created_at instead ?)

I would think that we don't need manual sorting. As you suggest, they should be sorted by creation date for now. Once we have sequential invoice numbers then we can order by those.

data: will store the data used to render the invoice.

This is an interesting part. If we change the data format in the future, we probably need to run a migration for all old invoices. We'll see.

Is it better to use Invoice or Spree::Invoice as a name for the model?

Just Invoice. The Spree namespace is a legacy which I would like to see removed one day.

[mkllnk]

Spree::Admin::BaseController expects the order's associated models to start with Spree

We have other models like Enterprise and Customer which are not in the Spree namespace if you are looking for some inspiration.

[abdellani]

@RachL

What do you mean by "old" invoices. The ones we have currently? As they are not stored this work will completely replace the old invoice format.

No, I referred to:

I don't think we need to send them, but I just wanted to confirm.

When bulk invoice, we need to generate invoices, which means:

if the order has changed, we will need to cancel the previous invoice and create a new one
if the order has not changed, we generate an invoice if there wasn't any, or we use the one already there

In my opinion, we should have the same behavior when the user clicks on Send invoice or Print invoice. What do you think?

Bulk invoice printing needs to generate the invoice if there isn't any. I would say same for bulk sending. However note the first PR should only focus on the edit order page. As we are working with a feature toggle we can work bits by bits.

What do you mean by bulk sending?

Do you mean sending multiple invoices at the same time?
if yes, I missed that on the code. Can you please show me how can I trigger that from the UI?

If capturing a payment would pass the invoice in a "paid" status. I think we would have paid/unpaid/cancelled. Active would be replace by paid or unpaid. What do you think?
...
So credit invoices cannot be paid, they can only be refunded. Do we want to add a status for this?

With the current progress, I don't know yet. I think we can decide later.

[abdellani]

@mkllnk

For the first step, here's how I see the solution:

InvoiceRenderer will receive an invoice instance instead of an order.

The invoice template (spree/admin/orders/invoice2) will no longer have direct access to the order.

To generate the data attribute on the Invoice, I'll add a new class InvoiceDataGenerate to serialize the orders.

I suggest using to_json to serialize the orders, the result of their formatting methods (display_item_total for example), and their related associations (payments and line_items).
For example:

order.to_json(only: :note, methods: :display_item_total  ,include:[distributor:{ only: :name }])

To say if one invoice is no longer active, and we can generate a new invoice:

1. load the latest invoice of the order latest_invoice
2. serialize the current order current_state_invoice
3. Compare the two invoices json.
4. Some fields will be ignored in the comparison, for example the enterprise legal name

From #10725

If the enterprise legal name changes or the address changes, currently we don't regenerate invoices, we keep previous invoices as is.

what do you think?

[RachL]

@abdellani

I don't think we need to send them, but I just wanted to confirm.

Ah yes, correct!

In my opinion, we should have the same behavior when the user clicks on Send invoice or Print invoice. What do you think?

Agreed.

Do you mean sending multiple invoices at the same time?
if yes, I missed that on the code. Can you please show me how can I trigger that from the UI?

Sorry the PR is still in test ready, but it will be a feature soon: #10337

[mkllnk]

To generate the data attribute on the Invoice, I'll add a new class InvoiceDataGenerate to serialize the orders.

Good idea. But we could also create a data model inside the invoice class: Invoice::Data. The initializer takes a hash but it can also have a method to serialize an order: Data.from_order(order)

The advantage of this approach is that it contains all the knowledge of the data structure and it can implement equality checks. So if we override hash which is used for eql? (and I think for ==) then we can easily compare current_data == stored_data. What do you think?

I would create a hash for significant fields and use equality on those hashes.

[abdellani]

@mkllnk

After trying this approach of serializing the Order, I faced some issues with:

1. The models' methods, there are two types
   1. The methods that return simple values like integers and booleans.
      For example: order.outstanding_balance? and order.paid?. Those are easy to serialize.
   2. The methods that return other models like: order.sorted_line_items.
2. Helpers for example:
   • Order::Helper.last_payment_method
   • Spree::PaymentMethodsHelper.payment_method_name
   • OrderHelper.outstanding_balance_label

For the models' methods:
In the beginning, I thought about serializing the output of the methods. The problem with this approach is that if any of the methods is updated to generate a different output, this will probably invalidate all the existing invoices.

Is it better to serialize the output of the models` methods? (and maybe avoid comparing the output of the methods during the comparison of the invoices?)
Or to write new methods that will use the serialized data? with this approach, we can edit the models without worrying about the invoices.

For the helpers:
the same problem and the same question.

[mkllnk]

For the models' methods:
In the beginning, I thought about serializing the output of the methods. The problem with this approach is that if any of the methods is updated to generate a different output, this will probably invalidate all the existing invoices.

Updates will need care from now on. Any output can change. For example, a price can be 2.5 now but may become "$2.50" in the future. I reckon that we will have to migrate the data when that happens. So if we change any of the output for invoices, we have to migrate existing invoices to the new format. Alternatively, we could support different versions of the invoice format and have several templates but that would become a lot over time.

I'm wondering if we should use an ActiveModel::Serializer to represent all data needed for an invoice. We can call to_json on it to store the data. But reading a hash in the view could be awkward. So maybe we would have an InvoiceDataPresenter which takes the JSON and has some "helper" methods for the view. These separate helpers can also be useful to present data in different ways. So you have scope to compute the result from the data. You just have to be careful that you can't use any data from the environment, for example the default currency. All needed data has to be in the data object. So we probably want to avoid classes like Spree::Money in the presenter.

Does that help?

So store only needed data. Have a stable interface via a serializer (change of serializer requires a database migration). And create a presenter with new helper methods. Or can you think of a simpler way?

[abdellani]

I like the idea of using ActiveModel::Serializer

Before your comment, I tried to serialize using to_json(). This approach will generate a huge JSON object that will be difficult to manipulate. I felt that it'll be difficult to maintain.

    order.to_json({
                    only: [:number, :special_instructions, :note],
                    methods: [:outstanding_balance?, :display_outstanding_balance, :paid?],
                    include: {
                      distributor: {
                        only: [:name, :abn, :invoice_text],
                        include: {
                          address: {
                            methods: [:full_address]
                          },
                          business_address: {
                            only: [:company]
                          },
                          contact: {
                            only: [:email]
                          }
                        }
                      },
                      bill_address: {
                        only: [:phone],
                        methods: [:full_address]
                      },
                      customer: {
                        only: [:code, :email]
                      },
                      order_cyle: {
                        only: [:name]
                      },
                      shipping_method: {
                        only: [:require_ship_address]
                      },
                      ship_address: {
                        only: [:phone],
                        method: [:full_name]
                      },
                      payments: {
                        only: [:name, :state, :description, :created_at],
                        #missing method name
                        methods: [:display_amount]
                      },
                      line_items: {
                        only: [],
                        include: {
                          supplier: {
                            only: [:name]
                          }
                        }
                      }
                    }
                  })

I'll create new object serializers that will be located on app/serializers/invoice.

[abdellani]

@mkllnk

I started by adding new serializers (app/serializers/invoice) that will be used on the invoices.
I updated the template to use the invoice's data presenter. The presenter is composed of objects that match the data model.

I guess I'm on the right path :)

what do you think?

[mkllnk]

Yes, you are on a good path. I think it's time to squash some of the fixes and add some specs.

[mkllnk]

This check needs changing now, right? Or is to_json finding the right serializer already?

[abdellani]

I extracted the comparison logic into a separated class.
app/services/order_invoice_comparator.rb

I use a non-recursive BFS to compare invoices.

[abdellani]

@mkllnk

I added some changes:

• refactored the presenters.
• Implemented an invoice comparator

Can you please review those changes?

---

if the logic is good, I'll:

1. add more tests to the new parts
2. ask the team members for the relevant attributes of each model for comparison.

[mkllnk]

Great work.

[drummer83]

@abdellani
I rebased and merged this PR. Thanks a lot!

I created issues for my findings and added them to the Invoices column of the board and to the Epic.

Let me know if you have any questions around this.
Thanks!