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

[IMP] website_sale_delivery: load carrier price async #31857

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@rdeodoo
Copy link
Contributor

rdeodoo commented Mar 14, 2019

Before this commit:
User landing on checkout payment step had to select a carrier.
These carriers only showed 'Select to compute price'. This was done to
prevent the page to take seconds to load, since it would compute all the
carrier price by querying these carrier API then display the page.
The users would still click on every carrier anyway to check all the prices and
select the cheapest one.

Now:
Once the page is loaded, we will asynchronously retrieve every carrier prices.
That way, the page rendering page will not be affected and the user will be
able to see the price of every carrier after a few seconds without having to do
anything.

task-1867265

@robodoo robodoo added the seen 🙂 label Mar 14, 2019

@C3POdoo C3POdoo added the RD label Mar 14, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch to 0dac059 Mar 14, 2019

@robodoo robodoo added the CI 🤖 label Mar 14, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from 0dac059 to 8d8f5a3 Mar 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 15, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from 8d8f5a3 to 0c51273 Mar 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 15, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from 0c51273 to 386ad34 Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from 386ad34 to be5c059 Mar 15, 2019

@robodoo robodoo added the CI 🤖 label Mar 15, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from be5c059 to 8fbcd77 Mar 18, 2019

@robodoo robodoo removed the CI 🤖 label Mar 18, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from 8fbcd77 to caa2bbc Mar 18, 2019

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

rdeodoo commented Mar 18, 2019

@rdeodoo rdeodoo requested review from qsm-odoo and JKE-be Mar 18, 2019

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

rdeodoo commented Mar 18, 2019

This has to be reviewed alongside with the enterprise counterpart to make sense.

@robodoo robodoo added the CI 🤖 label Mar 18, 2019

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

I supposed you added JKE to review the python so I did not look at it

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from caa2bbc to d0c7755 Mar 19, 2019

@robodoo robodoo removed the CI 🤖 label Mar 19, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from d0c7755 to f7e9b71 Mar 19, 2019

@robodoo robodoo added the CI 🤖 label Mar 19, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from f7e9b71 to fc1ad32 Mar 20, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 20, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from fc1ad32 to f49cfa8 Mar 20, 2019

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 20, 2019

[FIX] website_sale_delivery: remove useless check
This commit remove part of the condition as an empty jQuery element is always
truthy.

This commit is cleaning code to prepare the new feature to allow carrier prices
to be loaded asynchronously on cart, see odoo#31857 and odoo/enterprise#3844 (1867265).

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 20, 2019

[FIX] website_sale_delivery: don't replace error message by 'Free'
When a delivery carrier can't compute the delivery price, it returns an error
that is shown instead of the price (eg 'bpost Domestic is used only to ship
inside Belgium..').

But since 1ab53c2 everytime the user compute a carrier price and there is a
coupon allowing free delivery, every listed delivery carrier would have their
badges set to 'Free', including also carrier in error state.

This commit will not replace the error message by 'Free'.

Coming from odoo#31857

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 20, 2019

[FIX] website_sale_delivery: show 'Free' instead of '$0'
As coupon does, show 'Free' instead of '$0' when a delivery is free
(`free_over` field).

That is more coherent and also prevent flicker when coupon and delivery are
both installed.
Step to reproduce:
  - Have a free promotion from `sale_coupon` giving free delivery
  - Have a delivery carrier free over x$
  - Go to /shop/payment with over x$ in cart
  - Select the delivery carrier, it will show 'Free' then will be replaced a
    few seconds later by '$0'

Coming from odoo#31857

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 20, 2019

[IMP] website_sale_coupon_delivery: encapsulate code for task-1867265
This commit does nothing except encapsulating code from
_handleCarrierUpdateResult related to badges.

This is needed for the new feature to allow carrier prices to be loaded
asynchronously, see odoo#31857 and odoo#3844.

task-1867265

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 20, 2019

[IMP] website_sale_delivery: load carrier price async
Before this commit:
User landing on checkout payment step had to select a carrier.
These carriers only showed 'Select to compute price'. This was done to
prevent the page to take seconds to load, since it would compute all the
carrier price by querying these carrier API then display the page.
The users would still click on every carrier anyway to check all the prices and
select the cheapest one.

Now:
Once the page is loaded, we will asynchronously retrieve every carrier prices.
That way, the page rendering page will not be affected and the user will be
able to see the price of every carrier after a few seconds without having to do
anything.

Closes odoo#31857, coming with odoo/enterprise#3844
task-1867265

@robodoo robodoo removed the CI 🤖 label Mar 20, 2019

rdeodoo added some commits Mar 20, 2019

[FIX] website_sale_delivery: remove useless check
This commit remove part of the condition as an empty jQuery element is always
truthy.

This commit is cleaning code to prepare the new feature to allow carrier prices
to be loaded asynchronously on cart, see #31857 and odoo/enterprise#3844 (task-1867265).
[FIX] website_sale_delivery: don't replace error message by 'Free'
When a delivery carrier can't compute the delivery price, it returns an error
that is shown instead of the price (eg 'bpost Domestic is used only to ship
inside Belgium..').

But since 1ab53c2 everytime the user compute a carrier price and there is a
coupon allowing free delivery, every listed delivery carrier would have their
badges set to 'Free', including also carrier in error state.

This commit will not replace the error message by 'Free'.

Coming from #31857
[FIX] website_sale_delivery: show 'Free' instead of '$0'
As coupon does, show 'Free' instead of '$0' when a delivery is free
(`free_over` field).

That is more coherent and also prevent flicker when coupon and delivery are
both installed.
Step to reproduce:
  - Have a free promotion from `sale_coupon` giving free delivery
  - Have a delivery carrier free over x$
  - Go to /shop/payment with over x$ in cart
  - Select the delivery carrier, it will show 'Free' then will be replaced a
    few seconds later by '$0'

Coming from #31857
[IMP] website_sale(_delivery): remove coupon code from delivery module
As there is no bridge module between `website_sale_delivery` and
`website_sale_coupon` module, commit 1ab53c2 introduce coupon code into
delivery module, and coupon+delivery code into `website_sale` module.

That was the only way in stable but was nasty as it is not only code from one
module in another module but also code from enterprise module in community
module..

This commit revert that code as it will now be properly handled by the new
`website_sale_coupon_delivery` module in enterprise, see odoo/enterprise#3844 (task-1867265).

(Note that `sale_coupon_delivery` already exists)
[IMP] website_sale_coupon_delivery: encapsulate code for task-1867265
This commit does nothing except encapsulating code from
_handleCarrierUpdateResult related to badges.

This is needed for the new feature to allow carrier prices to be loaded
asynchronously, see #31857 and #3844.

task-1867265
[IMP] website_sale_delivery: load carrier price async
Before this commit:
User landing on checkout payment step had to select a carrier.
These carriers only showed 'Select to compute price'. This was done to
prevent the page to take seconds to load, since it would compute all the
carrier price by querying these carrier API then display the page.
The users would still click on every carrier anyway to check all the prices and
select the cheapest one.

Now:
Once the page is loaded, we will asynchronously retrieve every carrier prices.
That way, the page rendering page will not be affected and the user will be
able to see the price of every carrier after a few seconds without having to do
anything.

Closes #31857, coming with odoo/enterprise#3844
task-1867265

@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-async-load-carriers-price-checkout-rde branch from f49cfa8 to 3661fd1 Mar 20, 2019

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

rdeodoo commented Mar 20, 2019

Everything has been reviewed/processed.
I rewrote every commit message, everything is ready to be merged (correct PR links etc).
This is good for me

@robodoo robodoo added the CI 🤖 label Mar 20, 2019

$computeBadge.addClass('d-none');
$computeBadge.removeClass('o_wsale_delivery_carrier_error');
} else {
console.error(result.error_message);

This comment has been minimized.

@JKE-be

JKE-be Mar 20, 2019

Contributor

can we remove this ?

This comment has been minimized.

@rdeodoo

rdeodoo Mar 21, 2019

Author Contributor

I don't know, I prefer not do, it does no harm and it was part of the IMP commit da3c561#diff-54ba6dd06746c8beb778b0150f0e37f7R26 that was reviewed so I don't think it should be removed

This comment has been minimized.

@JKE-be

JKE-be Mar 21, 2019

Contributor

imo (and after discuss with your old collegua @qsm-odoo) there are no sense to log.error if it not a real error.

My idea is, if one day with have a crash manager. Do we need to show this error in a modal like a traceback?
For me not, it is just a message that is already displayed to the end user. And if really you want to keep the line it should be just for debug and so console.debug.

About " that was reviewed ":
So If I follow your idea, we cannot make any fix and improvement since the initial code was reviewed previously?

This comment has been minimized.

@rdeodoo

rdeodoo Mar 21, 2019

Author Contributor

I mean it is not an console.error that was forgotten during the development phase.
It was explicitly and willingly added there.
And as it does no harm, that was not the purpose of this PR to remove that code.

My idea is, if one day with have a crash manager. Do we need to show this error in a modal like a traceback?

One day.. 🎶
I don't know.. Was I supposed to decided on this PR what should be done for the possible futur task of the crash manager?

So If I follow your idea, we cannot make any fix and improvement since the initial code was reviewed previously?

Removing that console.error would neither be a fix or an improvement that's why I did not touch that line (that does no harm).

But I am not against removing that line now that it has been debated and that I also think that it will be better to not throw an error for the Odoo v18 when we will have the crash manager
I will remove it

This comment has been minimized.

@qsm-odoo

This comment has been minimized.

@JKE-be

JKE-be Mar 21, 2019

Contributor

ok thank you I will remove it myself before Odoo v18.

This comment has been minimized.

@JKE-be

JKE-be Mar 21, 2019

Contributor

And yes, it is a improvement to avoid to log error while it is a catched and expected error.

This comment has been minimized.

@JKE-be

JKE-be Mar 21, 2019

Contributor

thanks for your help, we will finalize this task internally

if (result.status === true) {
// if free delivery (`free_over` field), show 'Free', not '$0'
if (parseInt(result.new_amount_delivery)) {
$carrierBadge.children('span').text(result.new_amount_delivery);

This comment has been minimized.

@JKE-be

JKE-be Mar 20, 2019

Contributor

use jsclass instead

This comment has been minimized.

@JKE-be

JKE-be Mar 20, 2019

Contributor

eg. o_wsale_delivery_badge_price?

This comment has been minimized.

@rdeodoo

rdeodoo Mar 21, 2019

Author Contributor

We can't add a class to that span, that is the span auto populated by the monetary widget.
eg:

<span class="myclass" t-field="delivery.fixed_price" t-options='{"widget": "monetary", "from_currency": delivery.product_id.company_id.currency_id, "display_currency": website_sale_order.currency_id}'/>

becomes something like

<span class="myclass">
     <span>$</span>
     <span>24</span>
</span>

@JKE-be JKE-be closed this Mar 21, 2019

@robodoo robodoo added closed 💔 and removed CI 🤖 labels Mar 21, 2019

@JKE-be JKE-be reopened this Mar 21, 2019

@robodoo robodoo added the CI 🤖 label Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.