Opened 5 years ago

Closed 4 months ago

#13165 closed New feature (fixed)

Display edit link beside add button for ForeignKey fields in admin

Reported by: DrMeers Owned by: charettes
Component: contrib.admin Version: master
Severity: Normal Keywords: admin foreign key edit link 1.8-alpha
Cc: stefanw, dev@…, hv@…, ua_django_bugzilla@…, danielsamuels, cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

The reciprocal of #13163 -- when a ForeignKey field is rendered in admin as a select widget with an add (green +) button next to it, also provide an 'edit' button that takes you to the change form for the selected object.

This will require some JavaScript for extracting the selected object.

I'll get onto the code for this shortly; poke me if it doesn't appear soon.

Attachments (7)

change_links.diff (5.6 KB) - added by DrMeers 5 years ago.
Draft patch. No tests yet.
combined_13163_13165.diff (24.0 KB) - added by DrMeers 5 years ago.
Combined patch for #113163 and #13165
combined_13165_13165.diff (24.0 KB) - added by DrMeers 5 years ago.
Added unicode fix
combined_13165_13165.2.diff (24.0 KB) - added by DrMeers 5 years ago.
Translation fix
patch_13165_20110923.diff (28.0 KB) - added by stefanw 4 years ago.
Latest Patch ported to trunk
ticket13165_20111004.diff (29.2 KB) - added by stefanw 4 years ago.
Rewrote patch, removed cruft, added js
ticket13165_20111005.diff (36.4 KB) - added by stefanw 4 years ago.
Added links to raw_id and radio_field widgets

Download all attachments as: .zip

Change History (60)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 5 years ago by agabel

Is there any update to this ticket? If not, I'd like to take a stab at it if no one objects.

comment:3 in reply to: ↑ 2 Changed 5 years ago by DrMeers

Replying to agabel:

Is there any update to this ticket? If not, I'd like to take a stab at it if no one objects.

The code is almost ready, it just hasn't been high on my priority list since I know it won't be committed until after 1.2 lands.
Now that others are expressing interest in the feature I'll try to knock it off ASAP.

comment:4 Changed 5 years ago by DrMeers

  • Has patch set
  • Needs tests set

I have a patch which is working nicely. Confirmation of a few design decisions is required:

  1. Does the link popup a new window? [I think not, users can control-click for this behaviour]
  2. Does the link work for the selected object, or just the previously saved one? [I think the latter; otherwise clicking on it will cause changes in the form to be lost. It just needs to be clear that the link is to the saved selection (I've included the name of the object in the link text beside an edit icon).]
  3. Do we provide edit links for multiple-select widgets? [I've left these blank for now; if you've got 30 objects selected, links will be a bit unwieldy]
  4. Should it work for readonly_fields? [I think it should, so have included this functionality]

Based on my decision for point 2 above, the current patch requires no JavaScript. This also saves us having to do anything ugly with fudging reverse url resolution in JavaScript.

I'll get onto the tests shortly.

Changed 5 years ago by DrMeers

Draft patch. No tests yet.

comment:5 Changed 5 years ago by DrMeers

A further design decision: do we need to check whether the user has permission to edit the related model in question? Currently users will simply see a 'permission denied' message if they click the link and don't have permission. Which is probably OK. Hiding the link for users without permissions requires significant restructuring with the use of custom template tags or similar.

Changed 5 years ago by DrMeers

Combined patch for #113163 and #13165

comment:6 Changed 5 years ago by DrMeers

  • Needs tests unset

comment:7 Changed 5 years ago by DrMeers

  • Version changed from 1.1 to SVN

comment:8 follow-up: Changed 5 years ago by anonymous

It has some issues with the hungarian accented strings: If there is accent in the object's name, it does not render either the select widget, nor the add/edit buttons.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by DrMeers

Replying to anonymous:

It has some issues with the hungarian accented strings: If there is accent in the object's name, it does not render either the select widget, nor the add/edit buttons.

I cannot reproduce this problem; please provide an example problematic accented string.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by gk@…

Replying to DrMeers:

Replying to anonymous:

It has some issues with the hungarian accented strings: If there is accent in the object's name, it does not render either the select widget, nor the add/edit buttons.

I cannot reproduce this problem; please provide an example problematic accented string.

Let's say that the model, which is referred through foreign key, has this method:

def unicode(self):

return u"árvíztűrő tükörfúrógép"

If you edit some object of the model, which has the FK field on the admin interface, the source code will not include the select widget. When you press Save, you get an error message that the value is missing (if required). The add page seems like working, because there the field has no value, so the edit button (with the result of unicode() of the linked object) is not present.

comment:11 in reply to: ↑ 10 ; follow-ups: Changed 5 years ago by DrMeers

Replying to gk@lka.hu:

Let's say that the model, which is referred through foreign key, has this method:

def unicode(self):

return u"árvíztűrő tükörfúrógép"

If you edit some object of the model, which has the FK field on the admin interface, the source code will not include the select widget. When you press Save, you get an error message that the value is missing (if required). The add page seems like working, because there the field has no value, so the edit button (with the result of unicode() of the linked object) is not present.

Still can't reproduce; see herehttp://share.simonmeers.com/accented_string_foreign_key.png -- are you using trunk/1.2 with this patch applied?

comment:12 in reply to: ↑ 11 Changed 5 years ago by gklka

Replying to DrMeers:

Still can't reproduce; see herehttp://share.simonmeers.com/accented_string_foreign_key.png -- are you using trunk/1.2 with this patch applied?

I have Django 1.2.1 (stable) with Python 2.6 on Mac OS X.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by gklka

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by DrMeers

Replying to gklka:

Please see this video: http://www.youtube.com/watch?v=BIMRoBbH8ws

Thank you for taking the time to make this.

Could you please try changing line 374 of django/contrib/admin/util.py from

            '%(text)s</a>' % {'url': url, 'text': text}) 

to

            '%(text)s</a>' % {'url': url, 'text': force_unicode(text)}) 

and let me know if this solves the problem?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by gklka

It didn't help. I have tried to replace text with some constant string like 'text', but it does not work that way either. I think the bug is elsewhere.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by DrMeers

Replying to gklka:

It didn't help. I have tried to replace text with some constant string like 'text', but it does not work that way either. I think the bug is elsewhere.

Very strange; that seems to be the only thing that should be affected by the unicode representation of the object. Can you try something like this in a shell?

>>> from AppName import admin                 # ensure models get registered
>>> from AppName.models import *              # get models
>>> from django.contrib.admin import site     # get admin site
>>> from django.contrib.admin.util import *   # get get_changelink_ methods
>>> obj = ModelName.objects.get(pk=ChooseAnID)
>>> get_changelink_url(obj, site)
'/admin/AppName/ModelName/ChooseAnID/'
>>> get_changelink_html(obj, site)
u' <a href="/admin/AppName/ModelName/ChooseAnID/" class="related_field_changelink changelink">edit</a>'
>>> get_changelink_html(obj, site, show_name=True)
u' <a href="/admin/AppName/ModelName/ChooseAnID/" class="related_field_changelink changelink">edit \xe1rv\xedzt\u0171r\u0151 T\xdcK\xd6RF\xdaR\xd3G\xc9P</a>'

Hopefully something will blow up noisily so we can find where the problem is...

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by gklka

That's it:

>>> get_changelink_html(obj, site, show_name = True)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Library/Python/2.6/site-packages/django/contrib/admin/util.py", line 370, in get_changelink_html
    text = _('edit %s' % obj)
  File "/Library/Python/2.6/site-packages/django/utils/translation/__init__.py", line 55, in ugettext
    return real_ugettext(message)
  File "/Library/Python/2.6/site-packages/django/utils/functional.py", line 55, in _curried
    return _curried_func(*(args+moreargs), **dict(kwargs, **morekwargs))
  File "/Library/Python/2.6/site-packages/django/utils/translation/__init__.py", line 36, in delayed_loader
    return getattr(trans, real_name)(*args, **kwargs)
  File "/Library/Python/2.6/site-packages/django/utils/translation/trans_real.py", line 276, in ugettext
    return do_translate(message, 'ugettext')
  File "/Library/Python/2.6/site-packages/django/utils/translation/trans_real.py", line 262, in do_translate
    result = getattr(t, translation_function)(eol_message)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/gettext.py", line 404, in ugettext
    return unicode(message)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 26: ordinal not in range(128)

comment:18 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by DrMeers

Replying to gklka:

That's it:

Try replacing

  text = _('edit %s' % obj)

with

  text = _(u'edit %s' % force_unicode(obj))

and let me know how if this solves it; I'll then update the patch.

comment:19 in reply to: ↑ 18 Changed 5 years ago by gklka

OK, that works fine. Thanks for your efforts!

Changed 5 years ago by DrMeers

Added unicode fix

comment:20 follow-up: Changed 5 years ago by gk@…

I think the line 370 should have some more fix:

Instead of this:

        text = _('edit %s' % force_unicode(obj))

This:

        text = _('edit %s') % force_unicode(obj)

So this way it could be translated correctly.

Changed 5 years ago by DrMeers

Translation fix

comment:21 in reply to: ↑ 20 Changed 5 years ago by DrMeers

  • Status changed from new to assigned

Replying to gk@lka.hu:

So this way it could be translated correctly.

I believe you are correct, thank you. Patch updated.

comment:22 follow-up: Changed 4 years ago by JackLeo

Applying patch does not show any changes at all... what may be the case?

comment:23 in reply to: ↑ 22 Changed 4 years ago by JackLeo

Replying to JackLeo:

Applying patch does not show any changes at all... what may be the case?

My bad, patcher couldn't properly read .diff file.

comment:24 Changed 4 years ago by anonymous

  • Patch needs improvement set

it dont work after update

comment:25 Changed 4 years ago by julien

  • Resolution set to duplicate
  • Severity set to Normal
  • Status changed from assigned to closed
  • Type set to Uncategorized

Just for the reference, #6723 was closed as dupe.

comment:26 Changed 4 years ago by DrMeers

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Umm; if #6723 was closed as a duplicate of this, why was this one closed also?

comment:27 Changed 4 years ago by DrMeers

  • Type changed from Uncategorized to New feature

comment:28 Changed 4 years ago by julien

Sorry, didn't mean to close this one!

comment:29 Changed 4 years ago by julien

See #11397 for another dupe.

comment:30 Changed 4 years ago by julien

  • UI/UX set

comment:31 Changed 4 years ago by julien

  • Component changed from User Experience to contrib.admin

comment:32 follow-up: Changed 4 years ago by anonymous

  • Easy pickings unset

Hi - I'm just about to start hacking something like this together... any idea when it might make it into trunk? If there is anything useful I can do...

comment:33 in reply to: ↑ 32 Changed 4 years ago by julien

Replying to anonymous:

Hi - I'm just about to start hacking something like this together... any idea when it might make it into trunk? If there is anything useful I can do...

We're approaching the alpha/beta release stages so the earlier one starts to work on this and the closer one gets to a fully functional and fully tested code, the higher the chance it has to be included in core. Any contribution is appreciated ;-)

Changed 4 years ago by stefanw

Latest Patch ported to trunk

comment:34 Changed 4 years ago by stefanw

  • Owner changed from DrMeers to stefanw
  • Patch needs improvement unset
  • Status changed from reopened to new

I ported the latest patch to trunk including the following additional changes:

  • CSS names now contain - (dashes) instead of _ (underscores)
  • link generation in admin.util.get_changelink_html includes escaping


The related-field-changelink CSS class needs some styling (like vertical-align: middle or something).

comment:35 Changed 4 years ago by stefanw

  • Cc stefanw added
  • Status changed from new to assigned

comment:36 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Changed 4 years ago by stefanw

Rewrote patch, removed cruft, added js

comment:37 Changed 4 years ago by stefanw

  • Patch needs improvement set

jezdez commented in irc and disliked the OPs approach of retrieving the object and wanted the change link to change when a different fk object is selected.

There is a pull request with some open questions: https://github.com/django/django/pull/57

Changed 4 years ago by stefanw

Added links to raw_id and radio_field widgets

comment:38 Changed 4 years ago by stefanw

I posted to the django-ux list about this ticket: https://groups.google.com/forum/#!topic/django-ux/dzkQogwfx4I

comment:39 Changed 4 years ago by anonymous

Here's a little something I implemented for a project. It adds the edit / delete link and enable / disable them when nothing is selected. Plus saving an object after deleting (via popup) updates the repr of the object in the select box.

The code can be found on django snippet at http://djangosnippets.org/snippets/2562/. I think we should consider adding the delete link also.

comment:40 Changed 3 years ago by brillgen

  • Cc dev@… added

comment:41 Changed 3 years ago by charettes

Comment comment:39 was me. I packaged a simple app providing admin mixins to add a the edition and deletion links described above. You can find the project on github and on pypi.

comment:42 Changed 2 years ago by guettli

  • Cc hv@… added

comment:43 Changed 2 years ago by BinaryKhaos

  • Cc ua_django_bugzilla@… added

comment:44 Changed 18 months ago by charettes

  • Owner changed from stefanw to charettes

@stefanw hope you don't mind I assign this ticket to myself but I'm working on adapting the django-admin-enhancer code to merge it into Django. You can follow the WIP here.

comment:45 Changed 18 months ago by danielsamuels

  • Cc danielsamuels added

comment:46 Changed 17 months ago by charettes

I've been working on this lately and almost everything is working correctly.

The last missing part concerns foreign keys with to_field since there's no way to retrieve the related object based on the selected to_field value.

We've got two options here:

  1. Provides views redirecting to the related object change and delete views based on the provided to_field value;
  2. Attach a data-pk attribute on all options (in the case of Select widget) or to the raw id input to retrieve.

The second option doesn't seems viable since it's requires a lot of special casing when dealing with different types of widgets.

The first one should be doable by allowing ModelAdmin.get_object to receive a third parameter specifying which field to retrieve it's model from.

comment:47 Changed 17 months ago by charettes

I opened a Pull Request with a working implementation of 1.

I'd appreciated feedback on the possible from_field attack vector those changes introduce. One solution would be to restrict allowed from_field values based on current admin site registered models pointing to the actual model being looked up.

comment:48 Changed 17 months ago by charettes

I added the from_field restriction.

comment:49 Changed 8 months ago by collinanderson

  • Cc cmawebsite@… added

comment:50 Changed 6 months ago by charettes

Created a PR with the changes. It's only missing documentation and a way to deal with the two broken public APIs.

Last edited 6 months ago by charettes (previous) (diff)

comment:51 Changed 5 months ago by charettes

  • Patch needs improvement unset

Added what was missing to ship with 1.8. Review would be greatly appreciated.

See https://github.com/django/django/pull/3542

Last edited 5 months ago by charettes (previous) (diff)

comment:52 Changed 4 months ago by timgraham

  • Keywords 1.8-alpha added

comment:53 Changed 4 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 07988744b347302925bc6cc66511e34224db55ab:

Fixed #13165 -- Added edit and delete links to admin foreign key widgets.

Thanks to Collin Anderson for the review and suggestions and Tim for the
final review.

Note: See TracTickets for help on using tickets.
Back to Top