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
Jkmarx/fix transfer ownership bug #3055
Conversation
jkmarx
commented
Oct 11, 2018
- Resolves Transfer Data Set Bug #3045
- Removes a custom data_set get_owner method which used add_ perms
…he transfer data set bug.
@@ -364,10 +367,36 @@ class SharableResource(OwnableResource): | |||
def __unicode__(self): | |||
return self.name | |||
|
|||
def get_owner(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from the data_set class. Fritz noted this method improved performance 10%-20% percent.
Codecov Report
@@ Coverage Diff @@
## develop #3055 +/- ##
===========================================
- Coverage 60.66% 59.78% -0.89%
===========================================
Files 434 434
Lines 27871 27111 -760
Branches 1275 1275
===========================================
- Hits 16908 16208 -700
+ Misses 10963 10903 -60
Continue to review full report at Codecov.
|
pass | ||
|
||
return owner | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to ignore, since this is just a move, but:
- There is an
exists()
, but I'd be surprised if it's much of an improvement overcount()
. - Maybe just return in the try? (and get rid of
owner = None
andreturn owner
) - I can see that it's tested below, but I can't remember what it would mean to be ownerless, and how that would be useful.
def test_set_owner_data_sets_read_meta_perms(self): | ||
self.data_set.set_owner(self.user) | ||
user_perms = get_perms(self.user, self.data_set) | ||
self.assertTrue('read_meta_dataset' in user_perms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have done all of these at once with issubset
, but this works, too.