search

darklow / django-suit

Modern theme for Django admin interface
http://djangosuit.com/
Other
2.32k stars 704 forks source link

Django 2.2: Improve `get_native_model_url` model handling #731

Closed amureki closed 5 years ago

amureki commented 5 years ago

Greetings @darklow !

Thanks for lovely package

In Django 2.2 there were some updates to sites framework: https://github.com/django/django/commit/a9f5652113f0721a7066e359ae28d14692ea3c47

Which led to changed model dict (includes admin_url and add_url by default). This breaks with django-suit 0.2.26 because of the way it handles model dictionary in get_native_model_url. In short: it does expect dict to not return admin_url and have a fallback for it, but if we do have admin_url=None in dict, it uses it.

That ends up with AttributeError: 'NoneType' object has no attribute 'rstrip':

# Django 2.1
old_model_dict = {'name': 'Items', 'object_name': 'Item', 'perms': {'add': True, 'change': False, 'delete': False, 'view': False}, 'admin_url': None, 'add_url': '/admin/items/item/add/'}
# Django 2.2
new_model_dict = {'name': 'Items', 'object_name': 'Item', 'perms': {'add': True, 'change': False, 'delete': False, 'view': False}, 'admin_url': None, 'add_url': '/admin/items/item/add/'}

# Piece that breaks
url_parts = self.get_native_model_url(model).rstrip('/').split('/')
amureki commented 5 years ago

I am figuring out how to write a test now. But I am not sure, what is the process of supporting of an old version? Do you still accept bugfixes for master @darklow ?

For our setup 0.2.26 perfectly works with Django 2.1.9 and works with this fix in Django 2.2.2. So I'd be happy to have it there, otherwise we have to use our fork.

Many thanks!

amureki commented 5 years ago

I just found that there is already a test assert for that, but it was commented out. https://github.com/darklow/django-suit/blob/master/suit/tests/templatetags/suit_menu.py#L316

I uncommented it back, all seems green with my fix.

Without this fix, this test is breaking.

Sult commented 5 years ago

Bumping for attention. I just ran into this exact same issue. Weird thing is, If you are superuser, this passes, as a staff_user you dont. (could have to do with model permissions)

amureki commented 5 years ago

@Sult hey!

Feel free to use our fork (which we are going to keep until this would be fixed or changed in this repository):

https://github.com/Thermondo/django-suit

pip install -e git+https://github.com/Thermondo/django-suit.git@get_native_model_url_fix#egg=django-suit