-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Invoices #10532
Invoices #10532
Conversation
d983096
to
d217b4d
Compare
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
Questions:
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):
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:
|
@abdellani thank you !! Here are some answer:
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:
--
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.
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.
When bulk invoice, we need to generate invoices, which means:
A bit linked to the question I've asked here: #10725
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.
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...)? |
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.
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.
Just |
We have other models like Enterprise and Customer which are not in the Spree namespace if you are looking for some inspiration. |
I don't think we need to send them, but I just wanted to confirm.
In my opinion, we should have the same behavior when the user clicks on
What do you mean by Do you mean sending multiple invoices at the same time?
With the current progress, I don't know yet. I think we can decide later. |
For the first step, here's how I see the solution:
The invoice template ( To generate the I suggest using 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:
From #10725
what do you think? |
Ah yes, correct!
Agreed.
Sorry the PR is still in test ready, but it will be a feature soon: #10337 |
Good idea. But we could also create a data model inside the invoice class: 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 I would create a hash for significant fields and use equality on those hashes. |
After trying this approach of serializing the Order, I faced some issues with:
For the models' methods: 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?) For the helpers: |
Updates will need care from now on. Any output can change. For example, a price can be I'm wondering if we should use an 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? |
I like the idea of using Before your comment, I tried to serialize using 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 |
I started by adding new serializers ( I guess I'm on the right path :) what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are on a good path. I think it's time to squash some of the fixes and add some specs.
app/models/spree/order.rb
Outdated
def can_generate_new_invoice? | ||
return true if invoices.empty? | ||
|
||
invoices.last.data["order"].to_json != to_json(include: :line_items) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check needs changing now, right? Or is to_json
finding the right serializer already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted the comparison logic into a separated class.
app/services/order_invoice_comparator.rb
I use a non-recursive BFS to compare invoices.
9d62a56
to
71e33d6
Compare
I added some changes:
Can you please review those changes? if the logic is good, I'll:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
ef98a50
to
dc2e563
Compare
c1f7daa
to
3550993
Compare
Previously this template was calling various bits of data from the @order object and not the @invoice_presenter object
…e same interface as order
These methods are slightly different and they're both needed in the template for invoice4
This is now twice as fast and triggers half the number of database queries if both comparison methods get called
Co-authored-by: Maikel <maikel@email.org.au>
Co-authored-by: Maikel <maikel@email.org.au>
@abdellani 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. |
What? Why?
related topics
#10724
#10725
What should we test?
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