In my opinion the handling of invoice numbers is the main feature of this module. This have to be stable but it isn't yet as we have some issues in mass generating invoices.
#1397248: problems with mass generation for example views bulk operations

And I had some discussion on IRC and user group meeting about stability and interfaces for the invoice number.

Personally I'd like to have more flexibility with a token based number . The invoice number should more an ID possibly with strings inside if somebody need this. So maybe we call this external ID because there is still an internal ID which is an integer number used as entity ID. Currently it is very difficult to switch the invoice generation schema. When you change the setting there is some manual changing in database needed. And somebody wants a counter preset: #1643462: Add ability to change starting numbers of invoices

The interface idea is about using invoice numbers/external IDs from outside this module. This is useful if other business software or very special customized invoice number generation which is more complex then just tokenized generation. For this I will implement a hook or the call of an external function (defined in admin setting) which can be used in other modules to realize this. Maybe somebody want's to call a webservice to get the number. Then this can be done in a custom module. Or someone makes a module to manage invoice numbers on a central place for using in commerce invoice, manual invoice generation on a desktop and possibly providing a webservice where other business software and webshops can receive the invoice number togerther with commerce invoice.

There is an old ToDo-Comment there is probably a better way to do this in (protected function generate_invoice_id()). Let's start working on this.


cspitzlay’s picture

Hello Carsten,

following up to our recent discussion I prepared a patch that adds
a new callback-based method for invoice number generation.
The callback function receives the invoice object as parameter,
and starting from there it can get the order object if needed.

The list of callback functions is compiled using a hook so third party
or custom modules (like my own) can implement as complex
an algorithm for number generation as they like / need.

What do you think?

cspitzlay’s picture

I revised the above patch:
I moved the DB transaction boundaries to include the invoice number generation
and removed the incorrect dependency on ctools.

C_Logemann’s picture

Thank you Christian (@cspitzlay) for this improvement. I like the callback strategy.

I have just fixed the handling if no callback function is provided (empty array) and used the $callbacks directly as form options for human readable names including t()-Function. But This was only minor changes. I committed this with your authorship:

We need a full API description for this but for now a small code example for how to use:

function custom_commerce_invoice_number_callbacks() {
  return array(
'custom_invoice_gen' => t('my custom invoice callback'));
custom_invoice_gen($invoice = NULL) {
$order_id = $invoice->order_id;
'custom prefix' . $order_id;

I keep the issue open because there are some other things to solve.

garphy’s picture

Unless I'm completely mistaken, there's a major design flaw in the way the callback-based method is implemented :

The 'switch/case' statement that give the callback a chance to be called actually resides in an if statement conditioned by the existence of some past invoices.

As a result the callback is never called if no invoice exists.

Here's a patch that move the callback logic outside this statement as the check of past invoice is in the scope of the numbering strategy itself, IMHO.

As a side note, I think we should completely split the provided date-based strategy in a separate module and treat it equally with third party strategy. (but it's the main concern of this issue, isn't it?)

cspitzlay’s picture

@garphy: No, you are not mistaken, you found an embarrassing flaw.
I just happened to have invoices before I switched to the custom method.

But it seems to me this is broken for all the other cases, too, except the one that counts up from 1.

garphy’s picture

Any chance to see the underlying bugfix committed anytime soon ?
Do you want me to open another issue to address it specifically ?

cspitzlay’s picture

I am not a maintainer of this module so that would be up to C_Logemann.
@Carsten: What do you think?

BTW: I agree with your side note that the existing date-based methods
should be reimplemented as separate (sub)modules.

C_Logemann’s picture

Sorry I am very busy at moment.
@garphy: This issue is for discussion and an individual issue would be helpful for automated tests.
If the automated test is successful and @cspitzlay or other users mark this patch as RTBC I will commit it ASAP.

garphy’s picture

Sorry I am very busy at moment.

Is this a call for an additional co-maintainer ?

@garphy: This issue is for discussion and an individual issue would be helpful for automated tests.

Note sure about how to handle the automated tests.

If the automated test is successful and @cspitzlay or other users mark this patch as RTBC I will commit it ASAP.

cspitzlay’s picture

@C_Logemann: Do you mean we need to write and set up tests first? As far as I can tell there aren't any tests in commerce_invoice yet ...

C_Logemann’s picture

@cspitzlay: Yes, I just recognized that testing was not activated for issues. This is now fixed and I triggered the issue to start testing. The test system is working now.

cspitzlay’s picture

Erm, ok, it runs all the tests now. But last time I counted them they still amounted to zero (and the test bot agrees). ;-)

garphy’s picture

At least it checks that the patch applies cleanly ;)

C_Logemann’s picture

Is this a call for an additional co-maintainer ?

No this was not directly a call for this. But because of your question I was thinking about this.
I asked Christian (cspitzlay) who helped me before on some issues and already committed a good improvement.

@cspitzlay: It's a good idea to have tests for this module and not only test the integration with others. Feel free to integrate and welcome as the third maintainer.

cspitzlay’s picture

BTW: I committed garphy's patch from #1928798: Invoice number generation is broken

cspitzlay’s picture

Issue summary:View changes

Alternative idea for hook implemention

dwkitchen’s picture

Status:Active» Closed (won't fix)

Closing all Drupal 7 issues as this project will only be supported for Drupal 8