Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add cascade deletion for application_user_roles #56

Merged
merged 4 commits into from
Jan 24, 2019

Conversation

keigohtr
Copy link
Member

What is this PR for?

Because of the ForeignKey, we cannot delete Application model when we use an authentication.

This PR includes

  • Add backref to delete orphan
  • Change the way of deletion since query().delete() doesn't support cascade deletion (See link)

What type of PR is it?

Bugfix

What is the issue?

N/A

How should this be tested?

python -m unittest drucker_dashboard/test/test_access_control.py

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #56 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   79.12%   79.26%   +0.13%     
==========================================
  Files          36       36              
  Lines        1586     1596      +10     
==========================================
+ Hits         1255     1265      +10     
  Misses        331      331
Impacted Files Coverage Δ
drucker_dashboard/apis/api_evaluation.py 57.27% <ø> (-0.39%) ⬇️
drucker_dashboard/models/evaluation_result.py 81.48% <100%> (+2.31%) ⬆️
drucker_dashboard/models/application_user_role.py 100% <100%> (ø) ⬆️
drucker_dashboard/models/evaluation.py 93.75% <100%> (+0.41%) ⬆️
drucker_dashboard/utils/__init__.py 78.72% <100%> (+3.11%) ⬆️
drucker_dashboard/apis/api_kubernetes.py 86.69% <100%> (ø) ⬆️

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 16eaf44...9b0079f. Read the comment docs.

@keigohtr keigohtr force-pushed the fix/cascade-deletion-for-application_user_roles branch from 12c4afd to d84bea3 Compare January 22, 2019 07:42
@yoquankara
Copy link
Member

If you are using backref and setting ondelete='CASCADE, I think bulk delete is possible by adding passive_deletes=True to backref.
By doing so, there is no need to modify drucker_dashboard/apis/api_kubernetes.py?

Ref: https://docs.sqlalchemy.org/en/latest/orm/collections.html#using-passive-deletes

@keigohtr
Copy link
Member Author

@yoquankara Thank you for reviewing this!
I have updated to use passive_delete -> 5b4950b

@keigohtr keigohtr force-pushed the fix/cascade-deletion-for-application_user_roles branch from 5b4950b to e57a8f2 Compare January 23, 2019 23:22
@keigohtr
Copy link
Member Author

@yuki-mt I have changed evaluation related models. I confirmed it passes test but could you check it please? -> 1fedc08

@yuki-mt
Copy link
Member

yuki-mt commented Jan 24, 2019

@keigohtr Looks good! Could you also delete the line https://github.com/rekcurd/drucker-dashboard/blob/master/drucker_dashboard/apis/api_evaluation.py#L87-L88 ?
It is not necessary when ondelete='CASCADE' works.

@keigohtr
Copy link
Member Author

@yuki-mt Thank you for reviewing it!
I have removed unnecessary line you mentioned. -> 9b0079f

Copy link
Member

@yuki-mt yuki-mt left a comment

Choose a reason for hiding this comment

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

LGTM!

@keigohtr keigohtr merged commit d903d54 into master Jan 24, 2019
@keigohtr keigohtr deleted the fix/cascade-deletion-for-application_user_roles branch January 24, 2019 05:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants