admin: automatic link to files for FileFields in change_list breaks link to change_view (v1.8, #14497)

[Florian Demmer]

Hi guys,

i am confused/surprised by this ticket and already merged feature. it took me a while to find out what was happening in my admin:

https://code.djangoproject.com/ticket/14497

- it was merged into 1.8, right? the ticket says 1.2.
- there was no documentation in the patch, no info about it in the release notes or did i overlook something?
- as far as i can see, the patch was not changed after paulcollins' remark on the list_display-behaviour, so it was merged as is/was
- it has great potential to break one's admin and requires some digging in the source to find out why

So, what this new feature does is: it adds a link to the file of the `FileField` in the change_list and read-only version of the form-field in the change_view.
In the change_list that link is also added, when the `FileField` is the first column or it is in `list_display_links`.

The result is two a-tags in the resulting html, where the link to the file takes precedence. There is no way to go to the change_view or pick the row in a raw_id popup anymore. And there is no way to disable the feature.

Both change_list and change_view (for read-only fields) use `display_for_field()`, that's why both are affected by the change. That might not have been on purpose...

Imho, the automatically added link to the file should *not* have precedence over `list_display_links` and definitely should *not* break the automatically added link to the change_view, when the `FileField` happens to be first in `list_display`.

What do you think?

br,
Florian

[Riccardo Magliocchetti]

Hi Florian,

Il 22/11/2015 21:14, Florian Demmer ha scritto:
> Hi guys,
>
> i am confused/surprised by this ticket and already merged feature. it took me a
> while to find out what was happening in my admin:
>
> https://code.djangoproject.com/ticket/14497
>
> - it was merged into 1.8, right? the ticket says 1.2.

That was set at the time of filing the issue supposedly
It's pretty clear it has been applied to master and then backported:
https://code.djangoproject.com/ticket/14497#comment:32

> - there was no documentation in the patch, no info about it in the release notes
> or did i overlook something?

If it's not in the release notes i think this has been considered as a minor change

> - as far as i can see, the patch was not changed after paulcollins' remark on
> the list_display-behaviour, so it was merged as is/was

Yep, it looks like i completely missed that

> - it has great potential to break one's admin and requires some digging in the
> source to find out why

indeed, fortunately using a file field as first element is not so common

> So, what this new feature does is: it adds a link to the file of the `FileField`
> in the change_list and read-only version of the form-field in the change_view.
> In the change_list that link is also added, when the `FileField` is the first
> column or it is in `list_display_links`.
>
> The result is two a-tags in the resulting html, where the link to the file takes
> precedence. There is no way to go to the change_view or pick the row in a raw_id
> popup anymore. And there is no way to disable the feature.
>
> Both change_list and change_view (for read-only fields) use
> `display_for_field()`, that's why both are affected by the change. That might
> not have been on purpose...

I usually don't break software on purpose :)

> Imho, the automatically added link to the file should *not* have precedence over
> `list_display_links` and definitely should *not* break the automatically added
> link to the change_view, when the `FileField` happens to be first in `list_display`.
>
> What do you think?

Since you seem to care about this, any chance to open a pull request? The pull
request should contain updated tests to verify that the double <a> link is fixed
please. If not i'll give a try of fixing it later this week.

thanks

--
Riccardo Magliocchetti
@rmistaken

http://menodizero.it

[Florian Demmer]

On 2015-11-22 21:31, Riccardo Magliocchetti wrote:
> Hi Florian,
>
> Il 22/11/2015 21:14, Florian Demmer ha scritto:
>> Hi guys,
>>
>> i am confused/surprised by this ticket and already merged feature. it
>> took me a
>> while to find out what was happening in my admin:
>>
>> https://code.djangoproject.com/ticket/14497
>>
>> - it was merged into 1.8, right? the ticket says 1.2.
>
> That was set at the time of filing the issue supposedly
> It's pretty clear it has been applied to master and then backported:
> https://code.djangoproject.com/ticket/14497#comment:32

yeah, i know. i found the ticket by finding the code and finding the commit.

i was just wondering if the version should be updated on merge. it would
be easier to track down changes, when the merge-version is set on the
ticket, especially when the change is minor and not in the releasenotes.

But i see now, that i misunderstood the field:
https://docs.djangoproject.com/en/1.8/internals/contributing/triaging-tickets/#version

>
>> Both change_list and change_view (for read-only fields) use
>> `display_for_field()`, that's why both are affected by the change.
>> That might
>> not have been on purpose...
>
> I usually don't break software on purpose :)

did not mean the "breaking" part, just that changing it in one place had
an effect on both admin views... sorry if it sounded like that!

>
>> Imho, the automatically added link to the file should *not* have
>> precedence over
>> `list_display_links` and definitely should *not* break the
>> automatically added
>> link to the change_view, when the `FileField` happens to be first in
>> `list_display`.
>>
>> What do you think?
>
> Since you seem to care about this, any chance to open a pull request?
> The pull request should contain updated tests to verify that the
> double <a> link is fixed please. If not i'll give a try of fixing it
> later this week.
>
>

since both admin views use that same function, i am not sure how easy it
will be to fix without adding a whole bunch of more code just to detect
the field type and checking list_display_links etc... i'll probably not
find the time to look into that until next weekend either.

as a workaround i have added the 'id' field in first place to my
list_display.

br
Florian

[Riccardo Magliocchetti]

Il 22/11/2015 22:02, Florian Demmer ha scritto:
>
>
> On 2015-11-22 21:31, Riccardo Magliocchetti wrote:
>> Hi Florian,
>>
>> Il 22/11/2015 21:14, Florian Demmer ha scritto:
>>> Hi guys,
>>>
>>> i am confused/surprised by this ticket and already merged feature. it took me a
>>> while to find out what was happening in my admin:
>>>
>>> https://code.djangoproject.com/ticket/14497
>>>

>>> Imho, the automatically added link to the file should *not* have precedence over
>>> `list_display_links` and definitely should *not* break the automatically added
>>> link to the change_view, when the `FileField` happens to be first in
>>> `list_display`.
>>>
>>> What do you think?
>>
>> Since you seem to care about this, any chance to open a pull request? The pull
>> request should contain updated tests to verify that the double <a> link is
>> fixed please. If not i'll give a try of fixing it later this week.
>>
> since both admin views use that same function, i am not sure how easy it will be
> to fix without adding a whole bunch of more code just to detect the field type
> and checking list_display_links etc... i'll probably not find the time to look
> into that until next weekend either.
>
> as a workaround i have added the 'id' field in first place to my list_display.

I finally found time to look at this and on master it does not break at least,
am i missing something?

diff --git a/tests/admin_widgets/tests.py b/tests/admin_widgets/tests.py
index 46f2d2a..469a068 100644
--- a/tests/admin_widgets/tests.py
+++ b/tests/admin_widgets/tests.py
@@ -444,6 +444,12 @@ class AdminFileWidgetTests(TestDataMixin, TestCase):
File widgets should render as a link when they're marked "read only."
"""
self.client.login(username="super", password="secret")
+ response = self.client.get(reverse('admin:admin_widgets_album_changelist'))
+ self.assertContains(
+ response,
+ '<th class="field-__str__"><a
href="/admin_widgets/album/1/change/">Hybrid Theory</a></th>',
+ html=True,
+ )
response = self.client.get(reverse('admin:admin_widgets_album_change',
args=(self.album.id,)))
self.assertContains(
response,
diff --git a/tests/admin_widgets/widgetadmin.py b/tests/admin_widgets/widgetadmin.py
index 6b205b7..6287266 100644
--- a/tests/admin_widgets/widgetadmin.py
+++ b/tests/admin_widgets/widgetadmin.py
@@ -25,7 +25,7 @@ class EventAdmin(admin.ModelAdmin):


class AlbumAdmin(admin.ModelAdmin):
- fields = ('name', 'cover_art',)
+ fields = ('cover_art',)
readonly_fields = ('cover_art',)