-
-
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
a new invoice should refer the latest invoice. #11704
a new invoice should refer the latest invoice. #11704
Conversation
this PR is based on #11696 |
28a9bb6
to
5723644
Compare
ca424c4
to
ff2fd99
Compare
ff2fd99
to
915afd8
Compare
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, a straightforward addition to the invoices template.
I have a few suggestions, but I think they're non-essential so will approve 👍
@@ -30,4 +30,8 @@ def cancel_previous_invoices | |||
def display_number | |||
"#{order.distributor.id}-#{number}" | |||
end | |||
|
|||
def previous_invoice | |||
order.invoices.where("id < ?", id).first |
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.
If an order can have more that one previous invoice, we should use ordering to make sure it's the most recent one.
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'm intrigued to see that the spec does cover this. But it still seems worth making it explicit here.
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.
Hi @dacook
Invoices are sorted by default as follows:
default_scope { order(created_at: :desc) }
@@ -46,6 +46,8 @@ | |||
%br | |||
= "#{t :invoice_number}:" | |||
= @order.display_number | |||
- if @order.previous_invoice.present? | |||
= "#{t :invoice_cancel_and_replace_invoice} #{ @order.previous_invoice.display_number}" |
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'm not sure if it will matter in other languages, but just in case I'd suggest using interpolation here, just in case some translations want to write something like
"cancels and replaces number %{canceled_invoice_number} invoice."
context "Order doesn't have previous invoices" do | ||
it "should display the invoice number" do | ||
login_as_admin | ||
visit spree.print_admin_order_path(order, params: {}) |
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.
probably can leave out params: {}
(also below)
@@ -1991,6 +1991,7 @@ en: | |||
invoice_column_price_per_unit_without_taxes: "Price Per unit (Excl. tax)" | |||
invoice_column_tax_rate: "Tax rate" | |||
invoice_tax_total: "GST Total:" | |||
invoice_cancel_and_replace_invoice: "cancels and replaces invoice" |
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 just realised that these invoice keys could be scoped under a namespace.
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.
Looks good 👍
This is looking good! I noticed some other bugs while testing, but not linked to this PR so will open new issues - merging :) |
What? Why?
What should we test?
One possible scenario:
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates