Skip to content

Prevent empty querysets to raise AssertionError. #2862

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

Merged
merged 2 commits into from
Apr 22, 2015

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Apr 22, 2015

No description provided.

@ticosax
Copy link
Contributor Author

ticosax commented Apr 22, 2015

This assert queryset exists to prevent DjangoModelPermissions to apply on views that doesn't define a .querysetattribute (or get_queryset()).
Unfortunately a queryset that is empty is also excluded like there was no queryset defined.

>>> bool(queryset) is False
True
>>> assert queryset, 'no queryset define on the view'
  AssertionError 'no queryset define on the view'

Which is a lie.

@carltongibson
Copy link
Collaborator

OK — this seems good to me.

@tomchristie I'm feeling rusty: can you just cast your eye over it — but looks good yes?

@carltongibson
Copy link
Collaborator

@xordoquy — Since you're about — can you look at this too? (Ta!)

@ticosax
Copy link
Contributor Author

ticosax commented Apr 22, 2015

I will rebase it

@ticosax ticosax force-pushed the allow-empty-queryset branch from 477a803 to 6f66798 Compare April 22, 2015 09:14
@ticosax
Copy link
Contributor Author

ticosax commented Apr 22, 2015

done.

@@ -120,7 +120,7 @@ def has_permission(self, request, view):
if queryset is None and getattr(view, '_ignore_model_permissions', False):
return True

assert queryset, (
assert queryset is not None, (
'Cannot apply DjangoModelPermissions on a view that '
'does not have `.queryset` property.'
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change this lines to:

'Cannot apply DjangoModelPermissions on a view that '
'does not have `.queryset` property nor redefines `.get_queryset()`'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright

@xordoquy xordoquy added this to the 3.1.2 Release milestone Apr 22, 2015
@ticosax
Copy link
Contributor Author

ticosax commented Apr 22, 2015

let me know if you prefer I squash the commits.

@xordoquy
Copy link
Collaborator

It's ok for me. I'm just waiting the tests to be finished and I'll merge it.

@ticosax
Copy link
Contributor Author

ticosax commented Apr 22, 2015

Excellent, thank you.

@carltongibson
Copy link
Collaborator

👍

xordoquy added a commit that referenced this pull request Apr 22, 2015
Prevent empty `queryset`s to raise AssertionError.
@xordoquy xordoquy merged commit 8beef47 into encode:master Apr 22, 2015
@xordoquy
Copy link
Collaborator

Thanks to you in the first place :)

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.

3 participants