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

[12.0][ADD] Add delivery service #479

Merged

Conversation

lmignon
Copy link
Collaborator

@lmignon lmignon commented Oct 30, 2019

Forward port of #325
requires OCA/server-tools#1711

@lmignon lmignon added the 12.0 label Oct 30, 2019
@lmignon lmignon added this to the 12.0 milestone Oct 30, 2019
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LG. Minor changes for tests, no blockers.

@lmignon additional consideration for this and other modules: we should include in README some documentation on the usage and the behavior of the rest endpoints in regards of the client (locomotive basically but better if generic).
Something like: "on your website client you can call this service like $snippet and you have to configure $this and take care of $that."
This is especially true for important changes like the one that happened on payments "this change requires you update you client template like $this and change $that" etc.
I mean, I know that the client should have its own docs but the functionalities are declared server side as well as the way someone should interact w/ them. IMO we must have some generic client-oriented documentation in each module. What do you think?

for current_data, picking in zip(data, pickings):
carrier_dict = current_data.get("carrier", {})
sale_dict = current_data.get("sale", {})
self.assertEquals(current_data.get("delivery_id"), picking.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

assertEquals is sort of deprecated in py3, use assertEqual

self.assertEquals(
sale_dict.get("state"), picking.sale_id.state
)
self.assertAlmostEquals(
Copy link
Contributor

Choose a reason for hiding this comment

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

same -s

partner=self.service.partner, sale=True
)
pickings = picking1 | picking2 | picking3 | picking4
self.assertEquals(picking1.partner_id, self.service.partner)
Copy link
Contributor

Choose a reason for hiding this comment

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

these assertions seem superfluous as you've just created the pickings w/ this partner

@lmignon
Copy link
Collaborator Author

lmignon commented Oct 30, 2019

LG. Minor changes for tests, no blockers.

@lmignon additional consideration for this and other modules: we should include in README some documentation on the usage and the behavior of the rest endpoints in regards of the client (locomotive basically but better if generic).
Something like: "on your website client you can call this service like $snippet and you have to configure $this and take care of $that."
This is especially true for important changes like the one that happened on payments "this change requires you update you client template like $this and change $that" etc.
I mean, I know that the client should have its own docs but the functionalities are declared server side as well as the way someone should interact w/ them. IMO we must have some generic client-oriented documentation in each module. What do you think?

@simahawk I fully agree with you. It's the most important missing part of this project. We must also provide a clear changelog. I would like to start using towncrier to manage the changelog. @sbidoul plan to run towncrier in the bumpversion step of "ocabot merge" to generate the changelog OCA/maintainer-tools#432

@lmignon lmignon mentioned this pull request Oct 30, 2019
77 tasks
@codecov-io
Copy link

codecov-io commented Oct 30, 2019

Codecov Report

Merging #479 into 12.0 will decrease coverage by 51.89%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             12.0     #479      +/-   ##
==========================================
- Coverage   91.35%   39.46%   -51.9%     
==========================================
  Files         105       41      -64     
  Lines        2926     1746    -1180     
==========================================
- Hits         2673      689    -1984     
- Misses        253     1057     +804
Impacted Files Coverage Δ
...ader/wizards/shopinvader_variant_binding_wizard.py 22.8% <0%> (-73.69%) ⬇️
shopinvader/models/shopinvader_partner.py 27.11% <0%> (-72.89%) ⬇️
...der/wizards/shopinvader_category_binding_wizard.py 27.65% <0%> (-70.22%) ⬇️
shopinvader/models/shopinvader_variant.py 32.4% <0%> (-66.67%) ⬇️
shopinvader/models/sale.py 28.75% <0%> (-65%) ⬇️
shopinvader/services/abstract_sale.py 34.14% <0%> (-63.42%) ⬇️
shopinvader/services/abstract_mail.py 26.53% <0%> (-63.27%) ⬇️
shopinvader/services/customer.py 28.07% <0%> (-61.41%) ⬇️
shopinvader/services/cart.py 26.63% <0%> (-61.31%) ⬇️
shopinvader/controllers/main.py 37.14% <0%> (-60%) ⬇️
... and 88 more

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 48c4730...3884530. Read the comment docs.

@lmignon lmignon force-pushed the 12.0-shopinvader_delivery_service branch from 5fb0ba2 to 3884530 Compare October 30, 2019 12:37
@lmignon
Copy link
Collaborator Author

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 12.0-ocabot-merge-pr-479-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
@shopinvader-git-bot shopinvader-git-bot merged commit 3884530 into shopinvader:12.0 Oct 30, 2019
@shopinvader-git-bot
Copy link

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

@lmignon lmignon deleted the 12.0-shopinvader_delivery_service branch October 30, 2019 15:44
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.

5 participants