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] website_quote: portal optional products qty change #32715

Closed

Conversation

kebeclibre
Copy link
Contributor

Have a SO with a quotation template and some optional products set
Display it on the portal
On an optional product, click on the cart icon
to add it to the SO

Modify the quantity of the optional product

Before this commit, the feature was barely working:

  • the total price of the optional product did not change
  • negative quantities were allowed
  • there was no reaction when directly putting a number in the input
  • when decrementing the quantity, it crashed

After this commit:

  • the total price of the optional product changes as a function of the quantity input
  • negative quantities are not allowed
  • it is not possible to manually input the quantity with a keyboard
    only +/- buttons are used to change the quantity
  • decrementing the quantity works

OPW 1947769

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@kebeclibre
Copy link
Contributor Author

@KangOl
See #32712 for forwardport

@seb-odoo
Copy link
Contributor

seb-odoo commented Apr 16, 2019

I think this is not good enough to merge in stable.

As it is now, it might break the feature for existing users in different ways:

  • if just pull the code and not restart the server, because the JS will call a route that won't exist yet, without fallback on the previous one
  • if pull & restart, because the new JS requires a view change to work, again without fallback to something working if view not upgraded
    EDIT: actually this one is ok, the extra fix will not work without view upgrade but the rest could still be OK. But if there is a way to make the fix work without view upgrade that would be even better

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 16, 2019
@kebeclibre kebeclibre force-pushed the 10.0-website-quote-changeqty-lpe branch 2 times, most recently from 9d1e592 to bb054eb Compare April 16, 2019 14:06
@kebeclibre
Copy link
Contributor Author

@seb-odoo
Et voilà !
It is probably the less harming fix ever. But then again, code clarity is not improved by doing that

@seb-odoo
Copy link
Contributor

Ok this looks better (for compatibility, indeed not for code clarity but this is the price to pay I suppose).

Just wait a bit more before merging, I have some more comments to make, but I need to test first to be sure. I'll try to do that tomorrow.

@kebeclibre kebeclibre force-pushed the 10.0-website-quote-changeqty-lpe branch 2 times, most recently from 7878aee to e4df44c Compare April 17, 2019 07:21
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 17, 2019
Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

Overall this fixes the issue, but here are some comments about the details.

Also I'm not completely sure about the changes in the view. What absolutely requires the view change to work, and that does not work with the compatibility code only?

As for the disabled="1", I don't know if this is a good idea if someone made a custo to fix it or something. Also like this it feels like the quantity can't be updated at all, since unfortunately the disabled is the same color than the + and - buttons.

So not a fan of the disabled, would it be really too hard to just make it work?

return def;
},
_updateOrderValues: function (data) {
if(!data){
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have a diff on the line... Let's apply linter and add the space after if and after ).


// Compatibility: the module is not updgraded
// so classes in DOM are not present
if ( !$elPriceTotal.length && !$elPriceSubTotal.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

linter: no space after ( and before )

return false;
},
/**
* Only for compatibily reasons
Copy link
Contributor

@seb-odoo seb-odoo Apr 16, 2019

Choose a reason for hiding this comment

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

Small comment about wording here, this method is not only for compatibility since it calls the correct new route too.

Also missing doc about param, private and and return.

currency = Order.currency_id

return {
'order_line_product_uom_qty': str(quantity),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure we only support integer quantities, currently they are displayed as float.

var token = href.match(/token=([\w\d-]*)/)[1];

var callParams = {
'line_id': line_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have a diff on the line -> could add parseInt to be extra safe and consistent with order_id.

},
_updateOrderValues: function (data) {
if(!data){
location.reload();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here since we have diff... window.location.reload();

$elPriceTotal = $elPriceSubTotal = $parentTr.find('.oe_currency_value').last();
}

if ($elPriceTotal.length && linePriceTotal) {

This comment was marked as outdated.

@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 18, 2019
@kebeclibre kebeclibre force-pushed the 10.0-website-quote-changeqty-lpe branch 2 times, most recently from 13640ba to e181c23 Compare April 18, 2019 12:27
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 18, 2019
@kebeclibre
Copy link
Contributor Author

@seb-odoo
Done for the change on the input

The XML, the classes are so crappy that they need to be changed, plus, the code is so ugly without the classes that I am really in favor of changing the template

I split the compatibility layers from the business code

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 18, 2019
Have a SO with a quotation template and some optional products set
Display it on the portal
On an optional product, click on the cart icon
to add it to the SO

Modify the quantity of the optional product

Before this commit, the feature was barely working:
- the total price of the optional product did not change
- negative quantities were allowed
- there was no reaction when directly putting a number in the input
- when decrementing the quantity, it crashed
- untaxed and tax amounts were not dynamic

After this commit:
- the total price of the optional product changes as a function of the quantity input
- negative quantities are not allowed
- it is not possible to manually input the quantity with a keyboard
  only +/- buttons are used to change the quantity
- decrementing the quantity works
- untaxed and tax amounts are dynamic

OPW 1947769
@kebeclibre kebeclibre force-pushed the 10.0-website-quote-changeqty-lpe branch from 150572e to 3751921 Compare April 19, 2019 07:42
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 19, 2019
@kebeclibre
Copy link
Contributor Author

robodoo r+

@robodoo robodoo added error 🙅 and removed CI 🤖 Robodoo has seen passing statuses merging 👷 labels Apr 23, 2019
@robodoo
Copy link
Contributor

robodoo commented Apr 23, 2019

Staging failed: ci/runbot on d5a7bc8a369760c6c41ccefc735ff411b58f3478 (view more at http://runbot.odoo.com/runbot/build/504874)

@kebeclibre
Copy link
Contributor Author

robodoo retry

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 labels Apr 23, 2019
robodoo pushed a commit that referenced this pull request Apr 23, 2019
Have a SO with a quotation template and some optional products set
Display it on the portal
On an optional product, click on the cart icon
to add it to the SO

Modify the quantity of the optional product

Before this commit, the feature was barely working:
- the total price of the optional product did not change
- negative quantities were allowed
- there was no reaction when directly putting a number in the input
- when decrementing the quantity, it crashed
- untaxed and tax amounts were not dynamic

After this commit:
- the total price of the optional product changes as a function of the quantity input
- negative quantities are not allowed
- it is not possible to manually input the quantity with a keyboard
  only +/- buttons are used to change the quantity
- decrementing the quantity works
- untaxed and tax amounts are dynamic

OPW 1947769

closes #32715

Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 23, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 23, 2019
@fw-bot fw-bot deleted the 10.0-website-quote-changeqty-lpe branch October 19, 2019 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants