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

[ADD][8.0] : Dynamic Price for POS #6

Merged
merged 13 commits into from
Feb 12, 2015

Conversation

AdilHoumadi
Copy link

@legalsylvain
Copy link
Contributor

Hi @ah-taktik. Thanks a lot to propose this module. Current POS is a mess for not managing price list.
I'll take a look on your PR at the end of the week.

Best Regards.

This reverts commit 7040fe9.
### Missing features :

- As you may know, product template is not fully implemented in the POS, so I decided to drop it from
this module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Product template is not managed at all in POS, or there is some pending work I don't know... ?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah absolutely. For v8.0 there no solution for this missing feature !
You can check this discussions :
odoo/odoo#3401 (comment)
odoo/odoo#3500

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.
I'm actually working on that feature.
UPDATE: available here #7

Copy link
Author

Choose a reason for hiding this comment

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

Nice 👍

@@ -0,0 +1,34 @@
Dynamic Price for Odoo Point of Sale
------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the module should be named better 'pos_pricelist'.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah why not :)

@legalsylvain legalsylvain added this to the 8.0 milestone Nov 30, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 978c741 on taktik:8.0-pos_dynamic_price into a7a8ddb on OCA:8.0.

@AdilHoumadi
Copy link
Author

At the end of week, I will write some JS tests.

@legalsylvain
Copy link
Contributor

Hi @ah-taktik,
Did you began to do write tests ? I'm very interesting to implement JS tests in my PR #7.
2 remarks:

  • better to change the name for pos_pricelist, for better visilibity (as said before);
  • Can you remove the readme file, and move the text into the description of the module in openerp file, it is more OCA "style" and the text will be visible, on web apps of Odoo, or in Odoo Modules list;

Except that point and other technical remarks (below) 👍 (Functional Test)

Thanks a lot for your contribution !

this.pricelist_item_sorted = [];
this.product_catrgory_by_id = {};
this.product_catrgory_children = {};
this.product_catrgory_ancestors = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean product_catEgory_by_id (and _children, ancestors...)

@pedrobaeza
Copy link
Member

README file is OK for version 8.0, because Odoo allows it and you can see also the parsed description on Github, but you have to rename it as README.rst for being parsed correctly in both sides.

/**
* override this method to merge lines
* TODO : find a better way to do it
* @param product
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you override this function. Can you explain ? the original function merges lines...

@legalsylvain
Copy link
Contributor

@ah-taktik can you send me your email : sylvain.legal @ akretion.com
Thanks.

@AdilHoumadi
Copy link
Author

@legalsylvain, I sent you a message, check your inbox.

Adil Houmadi added 2 commits December 17, 2014 23:49
[CHG] : Rename module to pos_pricelist instead of the old one
[FIX] : Fix typo mentioned on this comments :
 - OCA#6 (comment)
 - OCA#6 (comment)
[IMP] : Recover a missed feature while setting a price for an orderline OCA#6 (comment)
[IMP] : Switch to while loop while iterating over an array
[REM] : Avoid bad practice on Object Class : OCA#6 (comment)
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2092500 on taktik:8.0-pos_dynamic_price into a7a8ddb on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8be730e on taktik:8.0-pos_dynamic_price into a7a8ddb on OCA:8.0.

@legalsylvain
Copy link
Contributor

Hi @ah-taktik, Thanks a lot for the refactoring and the various improves !
I'm newb in JS Test, do you have documentation about what you did ? Do you know if Travis execute the tests ? It doesn't seem, or I don't see the result in the log...
Regards.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4cd140f on taktik:8.0-pos_dynamic_price into a7a8ddb on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4cd140f on taktik:8.0-pos_dynamic_price into a7a8ddb on OCA:8.0.

@AdilHoumadi
Copy link
Author

Hi @legalsylvain, we can share together our knowledge in JS :

For JS Test in v8.0 : You can check this links,
https://www.youtube.com/watch?v=0GUxV85DDm4#t=24560
https://github.com/odoo/odoodays-2014/blob/master/automated_tests/index.rst

For V7.0 and V8.0 : this one is usable for the two versions :
https://www.odoo.com/documentation/8.0/reference/javascript.html#testing-in-odoo-web-client

I will try to implement the second way when I have sometimes.
I don't think that Travis execute JS Test, maybe need some scripting to do that.

Thanks.

Merge remote-tracking branch 'origin/8.0' into 8.0-pos_dynamic_price

* origin/8.0:
  Remove second env list
  Separated Lint tests
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 90f9243 on taktik:8.0-pos_dynamic_price into 19ee2ae on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3bff8f1 on taktik:8.0-pos_dynamic_price into 19ee2ae on OCA:8.0.

@AdilHoumadi
Copy link
Author

hello,

@legalsylvain, @pedrobaeza, It's possible to merge this PR ?

Thanks

@OSguard
Copy link

OSguard commented Dec 28, 2014

@ah-taktik please offer some patience, the OCA handles a lot of pull request and the first goal is to keep up quality and avoid fast merges
acording our rules http://odoo-community.org/page/how-to the pull request needs two final reviews. So i think it will happen but all of us have spare time. So thank you for your contribution and a little more patience with us

@AdilHoumadi
Copy link
Author

@OSguard, Thank you for you feedback. You can take your time.

@pedrobaeza
Copy link
Member

It works like a charm, but I see only a minor issue: if you select a customer with another pricelist, price tags in the right part (in the upper right part of each product box) still reflects the initial price. Can this be easily changed?

I'm not talking about different prices depending on the quantity, but the base price. Obviously, if you extend also this computation to reflect in that tag all the possible prices (1: x €\n3: y € and so on), it will be marvelous.

@AdilHoumadi
Copy link
Author

@pedrobaeza, Thank you for you feed back.

  • For the first point, yeah sure we can do that.
  • For the second one, I will try to find an ergonomic way to represent data.

[IMP] : refresh product UI to reflects while selecting a customer
For more details see the first point of this comment : OCA#6 (comment)
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 90e2579 on taktik:8.0-pos_dynamic_price into 19ee2ae on OCA:8.0.

@legalsylvain
Copy link
Contributor

Hi Tarik,

Thanks a lot for this excellent module.
It displays now the correct unit price depending of the pricelist.

So for me it's a big 👍

For the rest I would prefer to accept this PR, and let other people to upgrade this module depending of them needs, because this module do the work. (and very correctly)
What do you think about that @pedrobaeza ?

@sebastienbeau
Copy link
Member

👍 great feature

legalsylvain added a commit that referenced this pull request Feb 12, 2015
[ADD][8.0] : POS - manage pricelist in Front End POS
@legalsylvain legalsylvain merged commit 7d14e55 into OCA:8.0 Feb 12, 2015
@pedrobaeza
Copy link
Member

OK, I have added in the README the feature not yet implemented as documentation.

Thanks for the hard work!

@AdilHoumadi
Copy link
Author

@legalsylvain, @sebastienbeau, @pedrobaeza, Thanks a lot :)

@AdilHoumadi AdilHoumadi deleted the 8.0-pos_dynamic_price branch February 12, 2015 15:19
adrienpeiffer pushed a commit to acsone/pos that referenced this pull request Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants