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

[FIX] remove unrelated code to pos_pricelist and fix #128 #236

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

legalsylvain
Copy link
Contributor

Hi all,

I was trying to investigate on the bug #128, and for the time being, the bug occurs on a fresh database with pos_pricelist installed.
the reason is the change with an overwrite of the function addProduct (without calling super)

Looking more deeply, I just realized that this function has nothing to do with pos_pricelist and allow to merge lines, even if it's not the last line.
this feature can be great, but has nothing to do with this module.

I so removed the full function :

@imaralux : can you test ?
@AdilHoumadi, could you take a look ?

CC : @StefanRijnhart, @pablocm-aserti, @antespi

@pedrobaeza
Copy link
Member

Change module version number

@legalsylvain
Copy link
Contributor Author

Thanks for the review @pedrobaeza. Done.

line.set_quantity(options.quantity);
}
if (options.price !== undefined) {
line.set_unit_price(options.price);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is for setting the correct price and it's needed?

Copy link
Contributor Author

@legalsylvain legalsylvain Jan 8, 2018

Choose a reason for hiding this comment

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

No it's not. you'll find the same line in the Odoo code.
see original overwritten function :
https://github.com/odoo/odoo/blob/8.0/addons/point_of_sale/static/src/js/models.js#L1066

and

the comment of the function in the OCA module talks about lines merging.
https://github.com/OCA/pos/pull/236/files/c879f74940fbd11ed3f13d75c820fd500980d3c4#diff-747f62729cafe771e948f5cfeb9aff46L81

regards.

if (self.get('orderLines').models !== undefined) {
orderlines = self.get('orderLines').models;
}
for (var i = 0; i < orderlines.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's this hidden feature: upstream Odoo only considers the last line to merge with, this change considers all lines on the order. It should probably go into OCB instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it's not the place. (and overwrite function is allways a good way to generate bugs).

Ideally, it should be an option. In some cases, users want to merge all lines, sometime no lines. (but it's another story).

thanks for the review @StefanRijnhart.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

OK then

@legalsylvain legalsylvain merged commit 8cdb3ef into OCA:8.0 Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants