[ADD] module 'stock_picking_invoice_product_group'

[tafaRU]

Invoices created from picking grouped by product

This module allows you to group invoices generated from picking by product or by product category.
This is possible by selecting the related option in the wizard used for creating draft invoices.

Example

Created the following pickings:

1. picking_1 - partner_1
   1. product_A - category_1
   2. product_B - category_2
2. picking_2 - partner_2
   1. product_C - category_2
   2. product_D - category_3
3. picking_3 - partner_1
   1. product_C - category_2
   2. product_D - category_3
   3. product_A - category_1

Selecting option 'Group by product category' I get the following invoices:

1. invoice_1 - partner_1
   1. product_A - category_1 --> picking_1
   2. product_A - category_1 --> picking_3
2. invoice_2 - partner_1
   1. product_B - category_2 --> picking_1
   2. product_C - category_2 --> picking_3
3. invoice_3 - partner_1
   1. product_D - category_3 --> picking_3
4. invoice_4 - partner_2
   1. product_C - category_2 --> picking_2
5. invoice_5 - partner_2
   1. product_D - category_3 --> picking_2

On the contrary if I select option 'Group by product' I get the following invoices:

1. invoice_1 - partner_1
   1. product_A - category_1 --> picking_1
   2. product_A - category_1 --> picking_3
2. invoice_2 - partner_1
   1. product_B - category_2 --> picking_1
3. invoice_3 - partner_1
   1. product_C - category_2 --> picking_3
4. invoice_4 - partner_1
   1. product_D - category_3 --> picking_3
5. invoice_5 - partner_2
   1. product_C - category_2 --> picking_2
6. invoice_6 - partner_2
   1. product_D - category_3 --> picking_2

[moylop260]

v8 don't need relation, column1, column2 params
You can use
related_picking_ids = fields.Many2many('stock.picking', string='Related Pickings'

[tafaRU]

@moylop260 thanks for your review.
Yes I know, the official documentation says:

The attributes relation, column1 and column2 are optional. If not given, names are automatically generated from model names, provided model_name and comodel_name are different!

but It is not entirely clear to me how table and columns names are generated so that I can use them in post_init_hook.

Any insight would be very useful and highly appreciated.

Thanks.

[moylop260]

@tafaRU
I got it
Thank you

[moylop260]

👍

[tafaRU]

@pedrobaeza, I would appreciate it if you could review this PR.
Thank you in advance.

[pedrobaeza]

s/picking/pickings

[tafaRU]

Travis build errors are related to #185

[espo-tony]

This module seems to work without any trouble and implements a very useful functionality.

In the code review, though, I'd like to suggest some lesser improvements.

[espo-tony]

This method implements the api.multi decorator, which makes theoretically possible to have more than one record in self (also if I understand that in a transient model this is very unlikely to happen). If this happens, the syntax self.group_type will surely raise an error.

I think that a better way to write this is either change the decorator from 'api.multi' to 'api.one' or to use the 'self.ensure_one()' method in order to be sure that self always contain just one record, or to manually iterate the use of this module into the recordset using a syntax like 'for record in self'.

[tafaRU]

Fixed, thanks!

[espo-tony]

Here, in the else statement, is basically developed an implicit "default" for the field group_type: if group_type has not the value 'group_by_product', then the system should suppose that it has the value 'group_by_product_category'. This is actually not correct (the field is not declared as required, therefore it could already assume the None value) and not safe in the perspective of further developments.

I think that a better way to achieve the same result could be to explicitly set a default for the field 'group_type' and to declare it as required (either on the model definition or on the view definition).

In any case, I believe it's better to use an elif branch in order to handle the 'group_by_product_category' case, reserving the else statement to eventually handle any unexpected value.

[tafaRU]

I'd prefer to leave the possibility to choose 'Group by partner' option, this is the reason for that kind of implementation.

[espo-tony]

Of course, I'm not suggesting to prevent the user from choosing an option instead of the other. I'm just suggesting to handle the two different cases ('group_by_product' and 'group_by_product_category') in an if/elif statement rather than in an if/else statement. Something like:

if group_type == 'group_by_product':
    key = (partner.id, product.id)
elif group_type == 'group_by_product_category':
    key = (partner.id, categ.id)
else:
    #Some kind of exception raising/handling

That's because an else statement in this case would implicitly define a default: let's assume the user doesn't select any group type in the wizard (which is currently possible, being the field not required), the system would behave exactly like if he selected the option 'group_by_product_category', regardless if he really did it or not.

If this is the desired behavior, it would be better to explicitly define that option as a default for the field. Moreover, I believe that the system should force the user to choose an option.

[tafaRU]

@espo-tony, could you please have a final review?
Thank you in advance,

[espo-tony]

@tafaRU,
It seems to me that the Travis error is unrelated to this PR, being rather due to the module account_invoice_merge_purchase which is not fully migrated yet.
Therefore in my opinion this PR is ready to be merged. 👍