Opened 7 years ago

Closed 21 months 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: master
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 6 years ago.
Draft patch. No tests yet.
combined_13163_13165.diff (24.0 KB) - added by Simon Meers 6 years ago.
Combined patch for #113163 and #13165
combined_13165_13165.diff (24.0 KB) - added by Simon Meers 6 years ago.
Added unicode fix
combined_13165_13165.2.diff (24.0 KB) - added by Simon Meers 6 years ago.
Translation fix
patch_13165_20110923.diff (28.0 KB) - added by Stefan Wehrmeyer 5 years ago.
Latest Patch ported to trunk
ticket13165_20111004.diff (29.2 KB) - added by Stefan Wehrmeyer 5 years ago.
Rewrote patch, removed cruft, added js
ticket13165_20111005.diff (36.4 KB) - added by Stefan Wehrmeyer 5 years ago.
Added links to raw_id and radio_field widgets

Download all attachments as: .zip

Change History (60)

comment:1 Changed 7 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Austin Gabel

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

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

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

Attachment: change_links.diff added

Draft patch. No tests yet.

comment:5 Changed 6 years ago by Simon Meers

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

Attachment: combined_13163_13165.diff added

Combined patch for #113163 and #13165

comment:6 Changed 6 years ago by Simon Meers

Needs tests: unset

comment:7 Changed 6 years ago by Simon Meers

Version: 1.1SVN

comment:8 Changed 6 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 ; Changed 6 years ago by Simon Meers

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 ; Changed 6 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 ; Changed 6 years ago by Simon Meers

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 6 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 ; Changed 6 years ago by gklka

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

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 ; Changed 6 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 ; Changed 6 years ago by Simon Meers

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 ; Changed 6 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 ; Changed 6 years ago by Simon Meers

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 6 years ago by gklka

OK, that works fine. Thanks for your efforts!

Changed 6 years ago by Simon Meers

Attachment: combined_13165_13165.diff added

Added unicode fix

comment:20 Changed 6 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 6 years ago by Simon Meers

Attachment: combined_13165_13165.2.diff added

Translation fix

comment:21 in reply to:  20 Changed 6 years ago by Simon Meers

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 Changed 6 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 6 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 6 years ago by anonymous

Patch needs improvement: set

it dont work after update

comment:25 Changed 6 years ago by Julien Phalip

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

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

comment:26 Changed 6 years ago by Simon Meers

Resolution: duplicate
Status: closedreopened

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

comment:27 Changed 6 years ago by Simon Meers

Type: UncategorizedNew feature

comment:28 Changed 6 years ago by Julien Phalip

Sorry, didn't mean to close this one!

comment:29 Changed 6 years ago by Julien Phalip

See #11397 for another dupe.

comment:30 Changed 5 years ago by Julien Phalip

UI/UX: set

comment:31 Changed 5 years ago by Julien Phalip

Component: User Experiencecontrib.admin

comment:32 Changed 5 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 5 years ago by Julien Phalip

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

Attachment: patch_13165_20110923.diff added

Latest Patch ported to trunk

comment:34 Changed 5 years ago by Stefan Wehrmeyer

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 Changed 5 years ago by Stefan Wehrmeyer

Cc: Stefan Wehrmeyer added
Status: newassigned

comment:36 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

Changed 5 years ago by Stefan Wehrmeyer

Attachment: ticket13165_20111004.diff added

Rewrote patch, removed cruft, added js

comment:37 Changed 5 years ago by Stefan Wehrmeyer

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

Attachment: ticket13165_20111005.diff added

Added links to raw_id and radio_field widgets

comment:38 Changed 5 years ago by Stefan Wehrmeyer

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

comment:39 Changed 5 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 4 years ago by Brillgen Developers

Cc: dev@… added

comment:41 Changed 4 years ago by Simon Charette

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

Cc: hv@… added

comment:43 Changed 4 years ago by Matthias Dahl

Cc: ua_django_bugzilla@… added

comment:44 Changed 3 years ago by Simon Charette

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 Changed 3 years ago by Daniel Samuels

Cc: Daniel Samuels added

comment:46 Changed 3 years ago by Simon Charette

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

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

I added the from_field restriction.

comment:49 Changed 2 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:50 Changed 23 months ago by Simon Charette

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

Last edited 23 months ago by Simon Charette (previous) (diff)

comment:51 Changed 21 months ago by Simon Charette

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

comment:52 Changed 21 months ago by Tim Graham

Keywords: 1.8-alpha added

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

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