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

Fixes for generic foreign key raw id widget #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mikebq
Copy link
Contributor

@mikebq mikebq commented Feb 28, 2020

I am porting a Django app to Django 1.11 and found some issues in the latest version of django-authority while doing so.

The class GenericForeignKeyRawIdWidget has ForeignKeyRawIdWidget as a base class but its __init__ method calls forms.TextInput.__init__(I am guessing that this is why its class name starts with the word "Generic"). The following PR is my attempt to address errors when attempting to view an individual permission from the permissions list view.

The first commit on this branch addresses an issue that occurs when render is called on this widget. During render a call is made to forms.TextInput.render which then calls self.get_context. Unfortunately this goes to ForeignKeyRawIdWidget.get_context which then blows up as it the instance of the class is missing members that would have been added by ForeignKeyRawIdWidget.__init__. In order to avoid this I have added a get_context which simply delegates to forms.TextInput.get_context (resolves up; but it is calling into something that has been initialised).

The second commit addresses an issue that occurs when clicking the anchor of class related-lookup from the rendered widget. In the app that I am porting from Django 1.6.11, using django-authority 0.10 to Django 1.11 using the latest version of django-authority there are differences in the form of the URL where the widget is rendered. With the old version of Django and Django-Authority the path of the URL is like this admin/authority/permission/21852/ where as with Django 1.11 and latest Django-Authority I am seeing admin/authority/permission/21852/change/. Thus having related_url hard coded to "../../../" is not going to work for both paths. What I have done is past the request object into the widget so that it can work out how many ../ it should use to get back to the root of the "related" path.

If you think this PR is off course or wrong could you please let me know :) thank you.

@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #69 into master will increase coverage by 4.64%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage      39%   43.64%   +4.64%     
==========================================
  Files           7        7              
  Lines         423      433      +10     
  Branches       78       79       +1     
==========================================
+ Hits          165      189      +24     
+ Misses        250      233      -17     
- Partials        8       11       +3
Impacted Files Coverage Δ
authority/widgets.py 87.17% <100%> (+52.69%) ⬆️

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 3677b42...be36893. Read the comment docs.

@@ -149,16 +149,21 @@ class PermissionAdmin(admin.ModelAdmin):

def formfield_for_dbfield(self, db_field, **kwargs):
# For generic foreign keys marked as generic_fields we use a special widget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this as running tox locally would fail for py37-dj22 and report the following:

======================================================================
ERROR: test_raw_id_GenericForeignKey (authority.tests.PermissionAdminForDBFieldTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bia/django-authority/example/../authority/tests.py", line 490, in test_raw_id_GenericForeignKey
    raw_id_fields=['object_id'])
  File "/Users/bia/django-authority/example/../authority/tests.py", line 479, in assertFormfield
    ff = ma.formfield_for_dbfield(model._meta.get_field(fieldname), request=None)
  File "/Users/bia/django-authority/example/../authority/admin.py", line 154, in formfield_for_dbfield
    for f in self.model._meta.virtual_fields
AttributeError: 'Options' object has no attribute 'virtual_fields'

Seems that virtual_fields were deprecated in Django 1.10 and that 2.2 only has private_fields https://docs.djangoproject.com/en/2.2/_modules/django/db/models/options/

@mikebq mikebq marked this pull request as ready for review February 28, 2020 22:46
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.

1 participant