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

[8.0] Improve pos_pricelist #39

Merged
merged 27 commits into from
Aug 2, 2015
Merged

[8.0] Improve pos_pricelist #39

merged 27 commits into from
Aug 2, 2015

Conversation

AdilHoumadi
Copy link

[IMP]: improve JS indentation

[IMP]: remove _super method from Backbone.Model. Never manipulate the native Backbone.Model entity like this way because it is considered as bad practice.

[ADD]: Add support for price with taxes, new option is introduced in the POS config to let the user show price with taxes in product widget.
The UI is updated when we change the customer in order to adapt the prices
The computation take in account the pricelist and the fiscal position of the customer
The idea is based on what is done in this PR : #33 by @antespi

[IMP]: simplify JS for price computation (get_all_prices)

[ADD]: When we mouseover the price tag, a tooltip is shown to indicate the computation depending on the quantity like this output :

  • 1x -> 100 €
  • 3x -> 70 €
  • 5x -> 50 €

ui-prices

[IMP] : update readme to indicate the new improvements

Adil Houmadi added 6 commits July 12, 2015 01:43
Never manipulate the native Backbone.Model entity like this way because it is considered as bad practice.
…in the POS config to let the user show price with taxes in product widget.

- the UI is updated when we change the customer in order to adapt the prices
- the computation take in account the pricelist and the fiscal position of the customer
…te the computation depending on the quantity like this output :

1x -> 100 €
3x -> 70 €
5x -> 50 €
[IMP] : update readme to indicate the new improvements
@antespi
Copy link

antespi commented Jul 13, 2015

@ah-taktik thanks for this improvement. I've tried it in runbot and I've got this client error, with debug actived:

Odoo Client Error
Uncaught RangeError: Maximum call stack size exceeded
http://3110750-39-7537c2.runbot2.odoo-community.org/pos_pricelist/static/src/js/models.js:33

I've fixed with this ugly workaround:

    module.PosModelParent = module.PosModel;
    module.PosModel = module.PosModel.extend({
        /**
         * @param session
         * @param attributes
         */
        initialize: function (session, attributes) {
            // this.constructor.__super__.initialize.apply(this, arguments);
            module.PosModelParent.prototype.initialize.apply(this, arguments);
            this.pricelist_engine = new module.PricelistEngine({
                'pos': this,
                'db': this.db,
                'pos_widget': this.pos_widget
            });
            arrange_elements(this);
        },

In other hand, if you set pos.config.display_price_with_taxes = true, then price is showing without taxes in order line. I suggest you to extend get_display_price to show prices properly:

        get_display_price: function(){
            if (this.pos.config.display_price_with_taxes){
                return this.get_price_with_tax();
            }
            return this.constructor.__super__.get_display_price.apply(this, arguments);
        },

Finally, I've propused another PR (#38, pos_order_tax_detail), in order to track taxes associated to a pos.order. This is important in Spain, for example, because when POS order is printed we need a detailed tax table with total base and amounts for each tax, like invoices. This addon doesn't care fiscal position and calculates taxes in server side, just when POS order goes to paid state. But there's an issue (#38 (comment)) reported by @legalsylvain that will be fixed if JS code track taxes detailed totals and sync them to server like any other pos.order attribute. This will be the best implementation. Could you integrate this too in this improvement?

@AdilHoumadi
Copy link
Author

Hello @antespi, Thanks for your feedback 👍
I adapt the code as you mention, it works better now !

Thank you.

Clean way to inherit in JS And improve get_display_price method
initialize: function (attr, options) {
this._super('initialize', attr, options);
OrderlineParent.prototype.initialize.apply(this, arguments);
this.manuel_price = false;
Copy link

Choose a reason for hiding this comment

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

Is this a typo-error? Manuel is my son's name 😄 Should it be manual_price?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, exactly 😆

Thanks.

@antespi
Copy link

antespi commented Jul 14, 2015

@ah-taktik , thanks for your changes. Are you planning to implement taxes tracking? You should declare another Colletion in Order model, like TaxlineColletion, take care about base in module.Orderline.compute_all method and update collection for example in module.Order.getTaxDetails method with info returned by line.get_tax_details()

@legalsylvain legalsylvain added this to the 8.0 milestone Jul 15, 2015
@AdilHoumadi
Copy link
Author

Hello @antespi,
I am not sure if I understand your question, but each line has his own taxes which means that we don't need to track this changes, if you want to have taxes per line it's possible to override or create a new method to have this kind of output :


1x POS Product 1 : 121.00 €
--> VAT 21% - Services 21.00 €

1x POS Product 2 : 112.00 €
--> VAT 12% - Services 12.00 €

1x POS Product 3 : 127.00 €
--> VAT 21% - Services 21.00 €
--> VAT 6% - Services 6.00 €

Total: 360.00 €


If it's your request, Yeah we can propose a new module to override this method and customize the ticket.

Thanks.

[IMP] The pricelist in the current pos.config is used as default inst…
@antespi
Copy link

antespi commented Jul 17, 2015

Hi @ah-taktik, tax tracking in JS (POS offline client) side is ok. We don't need tax detail per line. But in Python (Odoo model) side, pos.order hasn't tax detail, only total tax amount, and when user print a ticket from backend then tax details doesn't appeas. It is quite different compared with ticket printed by POS client.

Some customers want to print tickets in PDF once per week to send them to theirs account department or subcontract account expert. In backend, user can select tickets for a week and print them together in one PDFs but account expert can't figure out what amount come from each tax.

Is this a common need? or this only happends in Spain?

It was adding to the product all the right-handed taxes found in the fiscal position, not taking into account the product source tax.
@pablocm-aserti
Copy link

Hi @antespi
That lack of tax tracking is not a problen just for us in Spain, but for everyone.
Currently, since the resulting taxes after computing the fiscal position mappings are not stored in the pos.order.line the same way it is done in sale.order.line, it just uses the taxes found in the product itself, as seen here.
It is not just a problem if you want to re-print the ticket. Although the module adds support for fiscal position mappings in the frontend, it does not in the back end and the resulting pos.order amounts are wrong.
Example (VAT 21% -> VAT21% included in the fiscal position):
captura de pantalla de 2015-07-21 12 06 26
captura de pantalla de 2015-07-21 12 52 17

I'll start working now in trying to add the taxes to the pos.order.line
If you are already on it, just let me know

@antespi
Copy link

antespi commented Jul 22, 2015

@pablocm-aserti We're not working on it right now. I've just overviewed a way to implement it. I think we have to do the same as Odoo standard do for order lines, reusing @ah-taktik code for calculating fiscal position and prices to apply.

So, IMHO, there are two collection of taxes:

  1. Taxes in Order line: A list of taxes applied only to the order line
    • You should declare another Colletion in module.Orderline model, for instance TaxlineColletion. An save them in module.Orderline.get_all_prices method.
  2. Taxes in Order: A list of taxes with total base and amount for each one
    • You should declare another Colletion in module.Order model, for instance TaxColletion. Take care about bases in module.Orderline.compute_all method and update collection for example in module.Order.getTaxDetails method with info returned by line.get_tax_details()

For example, this Order:

  1. Product 1: qty=10, price=50, taxes=20%, base=500, tax_amount=100, total=600
  2. Product 2: qty=1, price=20, taxes=10%, base=20, tax_amount=2, total=22

Now, we will care about customer with a fiscal position that add a 5% discount tax to all product, so taxes per Orderline are:

  1. Product 1:
    • base=500
    • tax20%: percent=0.20, amount=100
    • tax5%: percent=-0.05, amount=-25
    • total this line=575
  2. Product 2:
    • base=20
    • tax10%: percent=0.10, amount=2
    • tax5%: percent=-0.05, amount=-1
    • total this line=21

An now, at Order level, we have these taxes:

  • tax20%: base=500, percent=0.20, amount=100
  • tax10%: base=20, percent=0.10, amount=2
  • tax5%: base=520, percent=-0.05, amount=-26

If you add these two Collections, then when JS object is serialized and sent to Odoo server it will include tax information per pos.order.line and pos.order. So, you should extend those models in Odoo to save taxes. When user print pos.order there's no need to recalculate anything because it is already saved in pos.order, Odoo just print the tax info (per line, per tax, ...)

@pablocm-aserti
Copy link

@antespi I've come out with a solution for the backend tax incongruences when they were mapped on the frontend that is very similar to what you propose. Comments are welcome here ah-taktik/pos#2 Thanks!

@antespi
Copy link

antespi commented Jul 24, 2015

@ah-taktik, @pablocm-aserti this improvement rocks!!

For printing ticket from backend, this could be an example:
image

Taxes are printed at the end of ticket, aggregating each type of tax. This can be calculated in python-side from each order line tax info.

@antespi
Copy link

antespi commented Jul 24, 2015

@ah-taktik , @pablocm-aserti I'm working on it. I'll do a pull request to your branch. I've done before, it's just copy-paste for me. Wait me some time

@antespi
Copy link

antespi commented Jul 24, 2015

@ah-taktik , @pablocm-aserti, here https://github.com/ah-taktik/pos/pull/3 you have my part.
Please, review and merge if validated.

@AdilHoumadi
Copy link
Author

@antespi, @pablocm-aserti, Thank you for you contribution, it's great !

@antespi
Copy link

antespi commented Jul 28, 2015

Sorry @ah-taktik, but i forgot to add security ACL rules to __openerp__.py file
Here you have the PR: https://github.com/ah-taktik/pos/pull/5

This will fix runbot error

[FIX] Add security file to __openerp__.py
@antespi
Copy link

antespi commented Jul 29, 2015

👍
I think this is a great sample of community collaboration 😄

@legalsylvain
Copy link
Contributor

Hi @ah-taktik. Just a quick overview. Is it possible to move in a separate module taxes management ? It seem that your module include some taxes detail that could be better in (#38) (Or did I miss something ?)
Thanks for your clarification.

@AdilHoumadi
Copy link
Author

Hi @legalsylvain,
I think this PR replace this two PR (#33, #38). For the 1st one you can check this comment :
#33 (comment) and for the 2nd check this comment : #39 (comment)

@antespi, I'd like you feedback here.
@legalsylvain, Anyway if you find that it's better to split this feature, I am ok with it!

Thanks in advance.

@antespi
Copy link

antespi commented Jul 29, 2015

@legalsylvain, those PRs (#33, #38) are included in this PR and with a better implementation, because this one take care about pricelist and fiscal position when a customer is selected. This one track properly taxes in pos.order.line and this haven't issues (#38 (comment)) when a product change its taxes.

I think that "few addons" criteria is better when possible, and features implemented in this addon are related. So, I prefer to have only one addon: pos_pricelist. Maybe now this name doesn't represent properly features implemented.

IMHO, this features should be implemented in ofiicial point_of_sale addon. They aren't optional in any case. All our customers want pricelist and full tax management in POS orders as they have in sale orders and account invoices. In Spain, a POS order (a ticket) is as legal as an invoice, and we have to take care about fiscal position and taxes detail.

@rafaelbn
Copy link
Member

👍

@pedrobaeza
Copy link
Member

👍

Merging...

pedrobaeza added a commit that referenced this pull request Aug 2, 2015
@pedrobaeza pedrobaeza merged commit 5811f4f into OCA:8.0 Aug 2, 2015
@AdilHoumadi
Copy link
Author

@pedrobaeza, Thank you.

@AdilHoumadi AdilHoumadi deleted the 8.0-improve-pos-pricelist branch August 3, 2015 17:39
adrienpeiffer pushed a commit to acsone/pos that referenced this pull request Nov 30, 2015
…esheet_invoice

[ADD] analytic_partner_hr_timesheet_invoice
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.

6 participants