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

[10.0][ADD] add delivery price simulation for cart #352

Conversation

acsonefho
Copy link
Contributor

@acsonefho acsonefho commented Jun 28, 2019

Forward port of delivery_carrier service (cherry-pick of acsone/odoo-shopinvader@d710b6a ).
And add possibility for the user to specify a country/zip code

Expected behaviour:
Based on country/zip, compute the delivery price of the existing cart (provided by session). Then it returns carrier information with adapted price.
The partner is not updated with the country/zip (it's only simulation).

Bonus:
I extract the abstract sale into a new python file to avoid confusion.

@codecov-io
Copy link

codecov-io commented Jun 28, 2019

Codecov Report

Merging #352 into 10.0 will increase coverage by 0.13%.
The diff coverage is 98.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             10.0     #352      +/-   ##
==========================================
+ Coverage   89.36%   89.49%   +0.13%     
==========================================
  Files         139      141       +2     
  Lines        3619     3684      +65     
==========================================
+ Hits         3234     3297      +63     
- Misses        385      387       +2
Impacted Files Coverage Δ
shopinvader_delivery_carrier/controllers/main.py 80% <ø> (-5.72%) ⬇️
...ader_delivery_carrier/services/delivery_carrier.py 100% <100%> (ø)
...nvader_delivery_carrier/models/delivery_carrier.py 100% <100%> (ø)
shopinvader_delivery_carrier/services/cart.py 95.55% <93.75%> (+0.81%) ⬆️
...invader_delivery_carrier/services/abstract_sale.py 93.75% <0%> (-6.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4627e01...16950a0. Read the comment docs.

@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch 2 times, most recently from 58484e6 to 6ccbc14 Compare July 1, 2019 09:03
@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from 6ccbc14 to 2186a61 Compare July 8, 2019 13:15
@rousseldenis rousseldenis added this to the 10.0 milestone Aug 2, 2019
Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

The behaviour has to be changed

@rousseldenis
Copy link
Contributor

@lmignon

@lmignon lmignon changed the title [ADD][10.0] add delivery price simulation for cart [10.0][ADD] add delivery price simulation for cart Aug 29, 2019
Copy link
Collaborator

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

The way it's implemented add a lot of burden on the cart service. Moreover a partner is really created into the DB to simulate the price computation (IMO this is against RGPD).
All the functionalities related to delivery should be provided by dedicated services. A first step has already been implemented for version 9.0 shopinvader/odoo-shopinvader-carrier#1 (comment) and provides a more modular approach and a cleaner code.

@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from 2186a61 to 5510a97 Compare September 18, 2019 13:47
@acsonefho acsonefho changed the title [10.0][ADD] add delivery price simulation for cart WIP [10.0][ADD] add delivery price simulation for cart Sep 18, 2019
@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from 5510a97 to 357baf9 Compare September 18, 2019 14:17
@acsonefho acsonefho changed the title WIP [10.0][ADD] add delivery price simulation for cart [10.0][ADD] add delivery price simulation for cart Sep 19, 2019
Copy link
Collaborator

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

We must wait for #325 since some code into this PR is also into the other

shopinvader_delivery_carrier/services/abstract_sale.py Outdated Show resolved Hide resolved
acsonefho and others added 2 commits September 20, 2019 14:34
…_carrier service

The goal is to have a dedicated service to retrieve information of available delivery carriers
@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from 357baf9 to a233c02 Compare September 20, 2019 12:36
Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review. Seems ok

@lmignon
Copy link
Collaborator

lmignon commented Oct 28, 2019

@acsonefho Can you take care of the comments from @simahawk PLZ

@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from a233c02 to b854516 Compare October 29, 2019 07:16
@acsonefho acsonefho force-pushed the 10.0-shopinvader_delivery_estimation branch from b854516 to 16950a0 Compare October 29, 2019 12:55
@simahawk
Copy link
Contributor

@lmignon one thing more to add to #416 ;)

@lmignon
Copy link
Collaborator

lmignon commented Oct 30, 2019

/ocabot merge minor

@shopinvader-git-bot
Copy link

What a great day to merge this nice PR. Let's do it!
Prepared branch 10.0-ocabot-merge-pr-352-by-lmignon-bump-minor, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Oct 30, 2019
Signed-off-by lmignon
@lmignon lmignon mentioned this pull request Oct 30, 2019
77 tasks
@shopinvader-git-bot
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@shopinvader-git-bot shopinvader-git-bot merged commit 16950a0 into shopinvader:10.0 Oct 30, 2019
@shopinvader-git-bot
Copy link

Congratulations, your PR was merged at 43201c5. Thanks a lot for contributing to shopinvader. ❤️

@lmignon lmignon deleted the 10.0-shopinvader_delivery_estimation branch October 30, 2019 09:01
lmignon added a commit to acsone/odoo-shopinvader that referenced this pull request Oct 31, 2019
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.

7 participants