Discuss the strategy for future invoice number handling: stability, flexibility and interface [#1728106]

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 commerce_invoice_contrioller.inc (protected function generate_invoice_id()). Let's start working on this.

Comment	File	Size	Author
#4	implement-new-callback-based-method-1728106-4.patch	1.69 KB	garphy
#4
#2	implement-new-callback-based-method-1728106-2.patch	9.01 KB	cspitzlay
#2
#1	implement-new-callback-based-method-1728106-1.patch	5.95 KB	cspitzlay
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 10 September 2012 at 16:32

File	Size
implement-new-callback-based-method-1728106-1.patch	5.95 KB

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?

Comment #2

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 12 September 2012 at 09:38

File	Size
implement-new-callback-based-method-1728106-2.patch	9.01 KB

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

Comment #3

C-Logemann

German

Frankfurt/M, Germany

CreditAttribution: C-Logemann commented 2 October 2012 at 10:09

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:
http://drupalcode.org/project/commerce_invoice.git/commit/ac44c9f

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'));
}
function custom_invoice_gen($invoice = NULL) {
  $order_id = $invoice->order_id;
  return 'custom prefix' . $order_id;
}

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

Comment #4

garphy CreditAttribution: garphy commented 29 January 2013 at 13:10

File	Size
implement-new-callback-based-method-1728106-4.patch	1.69 KB

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

Comment #5

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 5 February 2013 at 14:49

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

Comment #6

garphy CreditAttribution: garphy commented 26 February 2013 at 13:05

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

Comment #7

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 26 February 2013 at 17:46

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.

Comment #8

C-Logemann

German

Frankfurt/M, Germany

CreditAttribution: C-Logemann commented 26 February 2013 at 19:16

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.

Comment #9

garphy CreditAttribution: garphy commented 27 February 2013 at 09:00

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.

http://drupal.org/node/1928798

Comment #10

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 27 February 2013 at 09:15

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

Comment #11

C-Logemann

German

Frankfurt/M, Germany

CreditAttribution: C-Logemann commented 27 February 2013 at 09:19

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

Comment #12

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay commented 27 February 2013 at 09:32

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

Comment #13

garphy CreditAttribution: garphy commented 27 February 2013 at 09:33

At least it checks that the patch applies cleanly ;)

Comment #14

C-Logemann

German

Frankfurt/M, Germany

CreditAttribution: C-Logemann commented 28 February 2013 at 20:13

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.