Opened 14 years ago

Closed 9 years ago

#13165 closed New feature (fixed)

Display edit link beside add button for ForeignKey fields in admin

Reported by: Simon Meers Owned by: Simon Charette
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin foreign key edit link 1.8-alpha
Cc: Stefan Wehrmeyer, dev@…, hv@…, ua_django_bugzilla@…, Daniel Samuels, 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 Simon Meers 14 years ago.
Draft patch. No tests yet.
combined_13163_13165.diff (24.0 KB ) - added by Simon Meers 14 years ago.
Combined patch for #113163 and #13165
combined_13165_13165.diff (24.0 KB ) - added by Simon Meers 14 years ago.
Added unicode fix
combined_13165_13165.2.diff (24.0 KB ) - added by Simon Meers 14 years ago.
Translation fix
patch_13165_20110923.diff (28.0 KB ) - added by Stefan Wehrmeyer 12 years ago.
Latest Patch ported to trunk
ticket13165_20111004.diff (29.2 KB ) - added by Stefan Wehrmeyer 12 years ago.
Rewrote patch, removed cruft, added js
ticket13165_20111005.diff (36.4 KB ) - added by Stefan Wehrmeyer 12 years ago.
Added links to raw_id and radio_field widgets

Download all attachments as: .zip

Change History (60)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Austin Gabel, 14 years ago

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

in reply to:  2 comment:3 by Simon Meers, 14 years ago

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 by Simon Meers, 14 years ago

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.

by Simon Meers, 14 years ago

Attachment: change_links.diff added

Draft patch. No tests yet.

comment:5 by Simon Meers, 14 years ago

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.

by Simon Meers, 14 years ago

Attachment: combined_13163_13165.diff added

Combined patch for #113163 and #13165

comment:6 by Simon Meers, 14 years ago

Needs tests: unset

comment:7 by Simon Meers, 14 years ago

Version: 1.1SVN

comment:8 by anonymous, 14 years ago

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.

in reply to:  8 ; comment:9 by Simon Meers, 14 years ago

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.

in reply to:  9 ; comment:10 by gk@…, 14 years ago

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.

in reply to:  10 ; comment:11 by Simon Meers, 14 years ago

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?

in reply to:  11 comment:12 by gklka, 14 years ago

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.

in reply to:  11 ; comment:13 by gklka, 14 years ago

in reply to:  13 ; comment:14 by Simon Meers, 14 years ago

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?

in reply to:  14 ; comment:15 by gklka, 14 years ago

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.

in reply to:  15 ; comment:16 by Simon Meers, 14 years ago

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...

in reply to:  16 ; comment:17 by gklka, 14 years ago

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)

in reply to:  17 ; comment:18 by Simon Meers, 14 years ago

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.

in reply to:  18 comment:19 by gklka, 14 years ago

OK, that works fine. Thanks for your efforts!

by Simon Meers, 14 years ago

Attachment: combined_13165_13165.diff added

Added unicode fix

comment:20 by gk@…, 14 years ago

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.

by Simon Meers, 14 years ago

Attachment: combined_13165_13165.2.diff added

Translation fix

in reply to:  20 comment:21 by Simon Meers, 14 years ago

Status: newassigned

Replying to gk@lka.hu:

So this way it could be translated correctly.

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

comment:22 by JackLeo, 13 years ago

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

in reply to:  22 comment:23 by JackLeo, 13 years ago

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 by anonymous, 13 years ago

Patch needs improvement: set

it dont work after update

comment:25 by Julien Phalip, 13 years ago

Resolution: duplicate
Severity: Normal
Status: assignedclosed
Type: Uncategorized

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

comment:26 by Simon Meers, 13 years ago

Resolution: duplicate
Status: closedreopened

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

comment:27 by Simon Meers, 13 years ago

Type: UncategorizedNew feature

comment:28 by Julien Phalip, 13 years ago

Sorry, didn't mean to close this one!

comment:29 by Julien Phalip, 13 years ago

See #11397 for another dupe.

comment:30 by Julien Phalip, 13 years ago

UI/UX: set

comment:31 by Julien Phalip, 13 years ago

Component: User Experiencecontrib.admin

comment:32 by anonymous, 13 years ago

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...

in reply to:  32 comment:33 by Julien Phalip, 13 years ago

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 ;-)

by Stefan Wehrmeyer, 12 years ago

Attachment: patch_13165_20110923.diff added

Latest Patch ported to trunk

comment:34 by Stefan Wehrmeyer, 12 years ago

Owner: changed from Simon Meers to Stefan Wehrmeyer
Patch needs improvement: unset
Status: reopenednew

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 by Stefan Wehrmeyer, 12 years ago

Cc: Stefan Wehrmeyer added
Status: newassigned

comment:36 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

by Stefan Wehrmeyer, 12 years ago

Attachment: ticket13165_20111004.diff added

Rewrote patch, removed cruft, added js

comment:37 by Stefan Wehrmeyer, 12 years ago

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

by Stefan Wehrmeyer, 12 years ago

Attachment: ticket13165_20111005.diff added

Added links to raw_id and radio_field widgets

comment:38 by Stefan Wehrmeyer, 12 years ago

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

comment:39 by anonymous, 12 years ago

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 by Brillgen Developers, 12 years ago

Cc: dev@… added

comment:41 by Simon Charette, 12 years ago

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 by Thomas Güttler, 11 years ago

Cc: hv@… added

comment:43 by Matthias Dahl, 11 years ago

Cc: ua_django_bugzilla@… added

comment:44 by Simon Charette, 10 years ago

Owner: changed from Stefan Wehrmeyer to Simon Charette

@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 by Daniel Samuels, 10 years ago

Cc: Daniel Samuels added

comment:46 by Simon Charette, 10 years ago

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 by Simon Charette, 10 years ago

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 by Simon Charette, 10 years ago

I added the from_field restriction.

comment:49 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:50 by Simon Charette, 9 years ago

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

Last edited 9 years ago by Simon Charette (previous) (diff)

comment:51 by Simon Charette, 9 years ago

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 9 years ago by Simon Charette (previous) (diff)

comment:52 by Tim Graham, 9 years ago

Keywords: 1.8-alpha added

comment:53 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

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