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

Use getattr to check is GuardedModelAdminMixin has the attribute queryset #311

Merged
merged 1 commit into from May 19, 2015

Conversation

frwickst
Copy link
Contributor

We can not check for queryset in Django 1.8 as the attribute is no longer available
The getattr function still checks the default parameter and fails with a AttributeError
as super(GuardedModelAdminMixin, self) has no attribute getquery. We also need to
pass a default parameter (None) to the second getattr call as this will otherwise fail
as well.

This fixes: #307

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.76% when pulling 393adb5 on frwickst:queryset_fix into 17fa48a on lukaszb:devel.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.76% when pulling 393adb5 on frwickst:queryset_fix into 17fa48a on lukaszb:devel.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.76% when pulling 393adb5 on frwickst:queryset_fix into 17fa48a on lukaszb:devel.

@ayc92
Copy link

ayc92 commented Apr 28, 2015

I think you also need to remove the django.VERSION < 1.6 check, otherwise in line 156 of obj_perms_manage_view, self.queryset will error out.

…yset

We can not check for queryset in Django 1.8 as the attribute is no longer available
The getattr function still checks the default parameter and fails with a AttributeError
as super(GuardedModelAdminMixin, self) has no attribute getquery. We also need to
pass a default parameter (None) to the second getattr call as this will otherwise fail
as well.
@frwickst
Copy link
Contributor Author

The above commits are just a rebase to the current 'devel' branch, no changes to the code yet. (The lined refered to by @ayc92 is now on line 151)

@frwickst
Copy link
Contributor Author

Would it be a better solution to change the queryset call at line 151 (and 224,227) to get_queryset instead? Ideas?

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.82%) to 92.07% when pulling 40ece18 on frwickst:queryset_fix into 8f82cf9 on lukaszb:devel.

@ayc92
Copy link

ayc92 commented Apr 29, 2015

@frwickst I'm in favor of your solution.

@frwickst
Copy link
Contributor Author

Ad pointed out in Issue #307 this might be a good time to remove support for older versions of Django. But for this to happen I want the go ahead from @lukaszb before going forward.

@TauPan
Copy link
Contributor

TauPan commented May 18, 2015

As a user: I'm also in favour of changing self.queryset in lines 151, 224 and 277 to self.get_queryset ... I don't see how these changes would hurt backwards compatibility, as the calls are specific to GuardedModelAdminMixin and decendants, and django < 1.6 apps will have the aliases. I'm going to deploy django-guardian in my Django 1.8 installation that way and notify you if I see any further problems.

(i.e. deploying with this patch:)

    Modified   guardian/admin.py
diff --git a/guardian/admin.py b/guardian/admin.py
index 5c83454..a884b77 100644
--- a/guardian/admin.py
+++ b/guardian/admin.py
@@ -148,7 +148,7 @@ class GuardedModelAdminMixin(object):
         shown. In order to add or manage user or group one should use links or
         forms presented within the page.
         """
-        obj = get_object_or_404(self.queryset(request), pk=object_pk)
+        obj = get_object_or_404(self.get_queryset(request), pk=object_pk)
         users_perms = SortedDict(
             get_users_with_perms(obj, attach_perms=True,
                 with_group_users=False))
@@ -221,7 +221,7 @@ class GuardedModelAdminMixin(object):
         Manages selected users' permissions for current object.
         """
         user = get_object_or_404(get_user_model(), pk=user_id)
-        obj = get_object_or_404(self.queryset(request), pk=object_pk)
+        obj = get_object_or_404(self.get_queryset(request), pk=object_pk)
         form_class = self.get_obj_perms_manage_user_form()
         form = form_class(user, obj, request.POST or None)

@@ -274,7 +274,7 @@ class GuardedModelAdminMixin(object):
         Manages selected groups' permissions for current object.
         """
         group = get_object_or_404(Group, id=group_id)
-        obj = get_object_or_404(self.queryset(request), pk=object_pk)
+        obj = get_object_or_404(self.get_queryset(request), pk=object_pk)
         form_class = self.get_obj_perms_manage_group_form()
         form = form_class(group, obj, request.POST or None)

@@ -443,4 +443,3 @@ class GroupManage(forms.Form):
         except Group.DoesNotExist:
             raise forms.ValidationError(
                 self.fields['group'].error_messages['does_not_exist'])
-

TauPan added a commit to TauPan/django-guardian that referenced this pull request May 18, 2015
lukaszb added a commit that referenced this pull request May 19, 2015
Use getattr to check is GuardedModelAdminMixin has the attribute queryset
@lukaszb lukaszb merged commit 267dab0 into django-guardian:devel May 19, 2015
@evandavey
Copy link

With 1.8 and on the guardian devel branch as at 112c373, I needed to change all lines guardian/admin.py

obj = get_object_or_404(self.queryset(request), pk=object_pk)

to

obj = get_object_or_404(self.get_queryset(request), pk=object_pk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django 1.8 GuardedModelAdminMixin.get_queryset exception
6 participants