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] shopinvader.partner: switch to ondelete=restrict #563

Merged
merged 1 commit into from
Mar 21, 2020

Conversation

alexis-via
Copy link
Contributor

I propose to switch the link between res.partner and shopinvader.partner to ondelete='restrict' instead of 'cascade', to avoid the situation where someone deletes a partner that has an account on shopinvader without being conscious of it.

@rousseldenis rousseldenis added this to the 10.0 milestone Jan 27, 2020
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.

Makes sense !

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.

makes sense, but at the same time I'm thinking if we shouldn't handle this on the application side w/ a constraint + ctx flag to bypass. What if we really want to delete the binding? Eg: removing the user from the shop but keep the partner?

@codecov-io
Copy link

codecov-io commented Jan 27, 2020

Codecov Report

Merging #563 into 10.0 will increase coverage by 1.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             10.0     #563      +/-   ##
==========================================
+ Coverage   88.36%   89.61%   +1.24%     
==========================================
  Files         125      142      +17     
  Lines        3162     3735     +573     
==========================================
+ Hits         2794     3347     +553     
- Misses        368      388      +20
Impacted Files Coverage Δ
shopinvader/models/shopinvader_partner.py 100% <ø> (ø) ⬆️
shopinvader_delivery_carrier/controllers/main.py 80% <0%> (-5.72%) ⬇️
shopinvader/services/cart.py 87.93% <0%> (-3.44%) ⬇️
shopinvader/models/shopinvader_category.py 95.77% <0%> (-2.96%) ⬇️
shopinvader/services/service.py 77.5% <0%> (-2.17%) ⬇️
shopinvader_delivery_carrier/services/cart.py 95.55% <0%> (-1.46%) ⬇️
shopinvader/services/abstract_mail.py 88.63% <0%> (-1.16%) ⬇️
shopinvader/models/shopinvader_product.py 89.36% <0%> (-0.44%) ⬇️
product_stock_state/models/product_product.py 100% <0%> (ø) ⬆️
shopinvader_image/hooks.py 100% <0%> (ø) ⬆️
... and 43 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 d4c0deb...fe41752. Read the comment docs.

@rousseldenis
Copy link
Contributor

makes sense, but at the same time I'm thinking if we shouldn't handle this on the application side w/ a constraint + ctx flag to bypass. What if we really want to delete the binding? Eg: removing the user from the shop but keep the partner?

Normal partner will remain as no unlink() has been implemented in shopinvader.partner to do so on res.partner (e.g.: same behaviour for product.product).

@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). 🤖

@lmignon lmignon mentioned this pull request Mar 19, 2020
77 tasks
Copy link
Contributor

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

/ocabot merge minor

@Cedric-Pigeon
Copy link

/ocabot merge minor

@shopinvader-git-bot
Copy link

This PR looks fantastic, let's merge it!
Prepared branch 10.0-ocabot-merge-pr-563-by-Cedric-Pigeon-bump-minor, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 20, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-563-by-Cedric-Pigeon-bump-minor, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 20, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-563-by-Cedric-Pigeon-bump-minor, awaiting test results.

@shopinvader-git-bot shopinvader-git-bot merged commit 40d6075 into shopinvader:10.0 Mar 21, 2020
@shopinvader-git-bot
Copy link

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

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.

8 participants