-
-
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
Sequential invoice numbers per distributor #11696
Sequential invoice numbers per distributor #11696
Conversation
This PR is based on #11679 |
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 👍
I'm assuming the requirement about bulk invoice ordering is already covered in BulkInvoiceJob#sorted_orders
.
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 !
I think we need to get #11679 in first |
Conflicts need to be resolved. |
ce40c5e
to
0f393db
Compare
Rebased on master, ready for testing. (dunno why GitHub reported conflicts; my local Git was able to rebase without any conflicts) |
0f393db
to
5656d9e
Compare
@abdellani when staging I see it has automatically generated a number for invoices previously created under the feature toggle. And it created a sequential number per order. So with my release farm on staging Fr I have several times the number 2720-1 for example. As I'm not able to replicate this once I start doing invoices with the new code, I'm assuming this only occurs for enterprise which had been playing with the new invoice feature before- which won't happened in production. So we are safe with ignoring this case. Am I right? |
5656d9e
to
dcac2b2
Compare
@RachL I didn't handle the existing invoices, do you want me to do that? |
@abdellani no I don't think so as I guess it won't happen in production, that's what I wanted to double check :-) I think this needs a rebase before we merge: can you have a look? Thanks |
dcac2b2
to
4a22224
Compare
hehe @sigmundpetersen you are more courageous than me :D |
Shall we merge then? 😊 |
yeaaaah merging without waiting for requirements to be met! #howwild |
ah actually we can't do that, we need a review, moving back to code review then! |
I don't think there has been any actual changes since you tested, only rebasing. |
@sigmundpetersen I'm ok with that, but I don't have the power to merge. Ticking the box doesn't change anything on my side this time. |
What? Why?
Depends on another PR, this one starts at: bd8460d
What should we test?
Some possible scenarios
Scenario 1:
Scenario 2:
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