Skip to content
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

Conversation

abdellani
Copy link
Member

@abdellani abdellani commented Oct 21, 2023

What? Why?

Depends on another PR, this one starts at: bd8460d

What should we test?

Some possible scenarios
Scenario 1:

  1. Create an order for Distributor 1 (suppose id is 123)
  2. Generate an invoice, the invoice number should be: 123-1
  3. Update the order (quantity for example)
  4. Generate another invoice, the invoice number should be: 123-2

Scenario 2:

  1. Create an order O1 for Distributor 1 (suppose id is 123)
  2. Generate an invoice for O1, the invoice number should be: 123-1
  3. Generate another order O2
  4. Generate an invoice for O2, the invoice number should be: 123-2
  5. Edit O1 (update quantity for example) and generate another invoice for O1. The invoice number should be 123-3

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

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

Dependencies

Documentation updates

@abdellani abdellani self-assigned this Oct 21, 2023
@abdellani abdellani marked this pull request as ready for review October 23, 2023 15:36
@abdellani
Copy link
Member Author

This PR is based on #11679

Copy link
Member

@dacook dacook left a 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.

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good !

@sigmundpetersen
Copy link
Contributor

I think we need to get #11679 in first

@RachL RachL added technical changes only These pull requests do not contain user facing changes and are grouped in release notes and removed blocked labels Nov 3, 2023
@sigmundpetersen
Copy link
Contributor

Conflicts need to be resolved.

@dacook dacook force-pushed the sequential-invoice-numbers-per-distributor branch from ce40c5e to 0f393db Compare November 8, 2023 01:14
@dacook
Copy link
Member

dacook commented Nov 8, 2023

Rebased on master, ready for testing.

(dunno why GitHub reported conflicts; my local Git was able to rebase without any conflicts)

@abdellani abdellani force-pushed the sequential-invoice-numbers-per-distributor branch from 0f393db to 5656d9e Compare November 13, 2023 06:28
@RachL RachL self-assigned this Nov 16, 2023
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Nov 20, 2023
@RachL
Copy link
Contributor

RachL commented Nov 21, 2023

@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?

@RachL RachL added feedback-needed and removed pr-staged-fr staging.coopcircuits.fr labels Nov 21, 2023
@abdellani abdellani force-pushed the sequential-invoice-numbers-per-distributor branch from 5656d9e to dcac2b2 Compare November 28, 2023 16:12
@abdellani
Copy link
Member Author

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?

@RachL
This was happening with the existing invoices created before this PR).
The invoice's number uniqueness scope was the order, not the distributor.

I didn't handle the existing invoices, do you want me to do that?

@RachL
Copy link
Contributor

RachL commented Nov 29, 2023

@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

@sigmundpetersen sigmundpetersen force-pushed the sequential-invoice-numbers-per-distributor branch from dcac2b2 to 4a22224 Compare December 8, 2023 12:33
@RachL
Copy link
Contributor

RachL commented Dec 8, 2023

hehe @sigmundpetersen you are more courageous than me :D

@sigmundpetersen
Copy link
Contributor

Shall we merge then? 😊

@RachL
Copy link
Contributor

RachL commented Dec 8, 2023

yeaaaah merging without waiting for requirements to be met! #howwild

@RachL
Copy link
Contributor

RachL commented Dec 8, 2023

ah actually we can't do that, we need a review, moving back to code review then!

@sigmundpetersen
Copy link
Contributor

sigmundpetersen commented Dec 8, 2023

I don't think there has been any actual changes since you tested, only rebasing.
So in my view this doesn't need another review

@RachL
Copy link
Contributor

RachL commented Dec 8, 2023

@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.

@sigmundpetersen sigmundpetersen merged commit 7b7b4ce into openfoodfoundation:master Dec 8, 2023
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Invoices numbers] Invoices should have sequential numbers by enterprise
5 participants