Merge lp://staging/~akretion-team/stock-logistic-flows/70-product_serial-plus-plus into lp://staging/stock-logistic-flows

LGTM, lot's f welcome code cleaning, thanks.

product_serial looks like a great module but it already has 4 functional features.
I humbly suggest it would be better to split it into several small specialized modules, and let user cherry-pick the ones they need.
I would make the code easier to understand and to extend.

@Lionel

I agree with you ; my merge proposal is a first step in this direction, with the introduction of :
- product_serial_sale_stock
- product_serial_purchase

but I won't do the split you talk about in this merge proposal, because it's out of the scope of this MP. It would have to be done in a new MP dedicated to this task.

Sorry Alexis de Lattre, I didn't notice the modules you introduced.
You're right a dedicated MP for the splitting will be more effective.

I'll approve it under the condition this will be splited.
You should already create an other MP which has this one as prerequisite.

Will you split that added feature of serial number in a module as well ?

Otherwise thanks for the code cleaning.

Cheers

@Yannick:

As I said, this MP is a first step towards a split ; as you can see, it introduce 2 new modules : product_serial_sale_stock and product_serial_purchase. I am about to do an MP on lp:openerp-manufacturing for product_serial_mrp.

BUT I didn't say that i'll do all the work for the split by myself. You say : "You should already create an other MP" : you are also invited to do it, it's a community work, not just my own work ! I am not the original author of the module product_serial, I am just a modest contributor on this module.

* in select_or_create_prodlots_from_interval:

The following lines

282 + prodlot_seq = ['%%s%%0%dd%%s' % number_length % (
283 + prefix, current_number, suffix) for current_number in
284 + range(first_number, last_number+1)
285 + ]

should be rewritten using '%s%0*d%s' % (prefix, current_number, number_length, suffix) (see http://docs.python.org/2.7/library/stdtypes.html#string-formatting-operations)

I also suggest testing that the interval limits are > 0

* spell check: s/splitted/split/ everywhere

* product_serial_sale_stock/sale_stock.py : the code is not calling super()._create_pickings_and_procurements and will therefore break any addons which overloads this method. If you really need to reimplement the method without playing the super game, this needs at least to be proeminently documented in the module description. And I think monkey patching the official addon implementation would be less disruptive to other addons.

* there are no automated tests for the new feature.

@Alexandre

Thanks for your review.

You are right, the fact that I don't call super() on _create_pickings_and_procurements() in the module product_serial_sale_stock is a big problem and I am aware of that. But, unfortunately, I didn't find a solution that includes calling super(). If you think it's possible, please give me some ideas !

I am not an expert of monkey patching, but I'll see with my colleagues if it is possible.

And thanks for pointing out my mistakes on English irregular verbs :)

@Alexandre:

Could you update your review status "Needs fixing" following my commits 45 to 48 ?

Hi Alexis,

Thanks for this contribs. I checked the corrected points suggested by Alexandre and that's fine now !

The only point I see is that we have no tests provided. Can I ask you to add at least one when making the next MP on splitting the module ?

This is a feature we need to be able to rely on, therefor having tests is important here IMO.

I don't want to block this MP any longer, but please think on adding tests the next time.

Best regards,

Joël

The source code management for this project has been moved to https://github.com/OCA/stock-logistics-workflow

Could you resubmit this MP on the new site?

Well... product_serial will have to be largely re-written for v8, given all the big changes in stock management. If the modules in v7.0 stay on LP, we could keep this branch here.

Alexis, They don't! :)

On Thu, Jul 3, 2014 at 6:16 PM, Alexis de Lattre <email address hidden>
wrote:

> Well... product_serial will have to be largely re-written for v8, given all the big changes in stock management. If the modules in v7.0 stay on LP, we could keep this branch here.
> --
> https://code.launchpad.net/~akretion-team/stock-logistic-flows/70-product_serial-plus-plus/+merge/195144
> Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-flows.
> --
> Mailing list: https://launchpad.net/~openerp-community-reviewer
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~openerp-community-reviewer
> More help : https://help.launchpad.net/ListHelp