Opened 16 years ago

Closed 7 years ago

#7028 closed New feature (wontfix)

Better admin raw_id_fields feedback

Reported by: Marcob <marcoberi@…> Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: raw-id-fields nfa-someday design_ux raw_id_fields
Cc: hv@…, davenaff@…, carlos.palol@…, stan@…, Stephane "Twidi" Angel, depaolim@…, cmawebsite@…, hugo@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

When you select an item for a raw-id-fields, the description of the id showes up only after saving current record. This is a major drawback for the user when id isn't a slug or a "self speaking" id. This little patch solve the problem showing the description immediately after user chooses the item in the raw id popup window.

Attachments (28)

improved-raw-id-admin-feedback.diff (3.1 KB ) - added by Marcob <marcoberi@…> 16 years ago.
immediate raw-id-fields selection feedback
improved-raw-id-admin-feedback-fixed-insertion.diff (3.4 KB ) - added by Marcob <marcoberi@…> 16 years ago.
Fixed two minor problem with unicode representation and create id description placeholder also during insertion of a new record
improved-raw-id-admin-feedback-fixed-regression-test.diff (3.7 KB ) - added by Marcob <marcoberi@…> 16 years ago.
This patch leaves only one (trivial) failure running django tests
tests-fix-for-improved-raw-id-admin-patch.patch (1.2 KB ) - added by Marcob <marcoberi@…> 16 years ago.
Little fix (one liner): now django tests run fine
improved-raw-id-admin-django-1.0.diff (3.7 KB ) - added by marcoberi@… 15 years ago.
improved-raw-id-admin-django-1.0.1.diff (3.7 KB ) - added by marcob 15 years ago.
patch for django 1.0.1
improved-raw-id-admin-django-1.0.1.2.diff (3.8 KB ) - added by marcob 15 years ago.
patch for django 1.1.0 alpha
ticket7028.patch (12.7 KB ) - added by mrts 14 years ago.
ticket7028-2.patch (12.3 KB ) - added by mrts 14 years ago.
Fixed the mistake that caused False to appear in output when is_popup was False.
ticket7028-with_popup.patch (14.1 KB ) - added by mrts 14 years ago.
Related object links open in popups.
ticket7028-with_popup_2.patch (17.4 KB ) - added by mrts 14 years ago.
Fixes and improvements as described in the comment.
ticket7028-3.patch (18.5 KB ) - added by mrts 14 years ago.
Merge trunk changes, hadle 'Add another'. Needs a bit more work, but is usable as-is.
ticket7028-4.patch (18.8 KB ) - added by mrts 14 years ago.
Removed a leftover from a merge conflict, added Marco to authors.
ticket7028-5.patch (19.3 KB ) - added by mrts 14 years ago.
Change field label on page when object is changed via popup. All functionality done.
ticket7028-6.patch (21.7 KB ) - added by mrts 14 years ago.
Add field clearing with red x.
ticket7028-7.patch (22.9 KB ) - added by mrts 14 years ago.
The clearing button only appears if an object is present, next to it. Commit: http://github.com/mrts/django/commit/267b1fea2518b2a39c35708b21b39ebeb069b0ee
patch7028-8.patch (21.4 KB ) - added by stanislas.guerra@… 14 years ago.
Patch for Django 1.2
patch7028-1.2.4.patch (15.7 KB ) - added by marcob 13 years ago.
Patch against 1.2.4 (no patch for tests, save file admin_list.py with unix fileformat)
patch7028-1.3.patch (15.0 KB ) - added by stan <stan__at__slashdev.me> 13 years ago.
Previous patch adapted to 1.3
patch7028-1.4.rev16590.patch (14.6 KB ) - added by Stanislas Guerra <stan__at__slashdev.me> 13 years ago.
Patch against 1.4 Alpha
patch7028-1.4.rev16669.patch (13.6 KB ) - added by Stan <stan__at__slashdev.me> 13 years ago.
Patch against latest trunk. Get rid of useless `get_related_url()' function.
patch7028-1.4.rev16669.2.patch (13.6 KB ) - added by Stan <stan__at__slashdev.me> 13 years ago.
Oups. Uploaded the wrong file (this one replace the previous upload).
patch7028-1.4.rev16669.3.patch (13.7 KB ) - added by Stan <stan__at__slashdev.me> 13 years ago.
Fix the `ValueError' raised before validation when non-integer value is enter.
patch7028-1.4.rev16961.patch (18.7 KB ) - added by Stanislas <stan@…> 12 years ago.
Patch to handle unregistred FK. +Regression tests.
patch7028-1.4.rev16961.2.patch (22.1 KB ) - added by Stanislas <stan@…> 12 years ago.
ManyToMany raw_id_field label added.
patch7028-1.4.1.patch (23.3 KB ) - added by Stanislas <stanislas.guerra@…> 11 years ago.
Patch against Django-1.4.1.
patch7028-1.4.5.patch (23.3 KB ) - added by Stan@… 11 years ago.
Fix a bug in M2M label when empty.
Add_token_field_event___Django_site_admin.png (20.0 KB ) - added by Julien Phalip 11 years ago.
First look for the suggested token_fields implementation

Download all attachments as: .zip

Change History (112)

by Marcob <marcoberi@…>, 16 years ago

immediate raw-id-fields selection feedback

comment:2 by Marcob <marcoberi@…>, 16 years ago

Fixed two minor problem with unicode representation and create id description placeholder also during insertion of a new record

by Marcob <marcoberi@…>, 16 years ago

Fixed two minor problem with unicode representation and create id description placeholder also during insertion of a new record

comment:3 by Marcob <marcoberi@…>, 16 years ago

Running tests there are two failure.

The first one is obvious (we added an id to description label):

Expected:
    <input type="text" name="test" value="1" class="vForeignKeyRawIdAdminField" /><a href="../../../admin_widgets/band/" class="related-lookup" id="lookup_id_test" onclick="return showRelatedObjectLookupPopup(this);"> <img src="/media/img/admin/selector-search.gif" width="16" height="16" alt="Lookup"></a>&nbsp;<strong>Linkin Park</strong>
Got:
    <input type="text" name="test" value="1" class="vForeignKeyRawIdAdminField" /><a href="../../../admin_widgets/band/" class="related-lookup" id="lookup_id_test" onclick="return showRelatedObjectLookupPopup(this);"> <img src="/media/img/admin/selector-search.gif" width="16" height="16" alt="Lookup"></a>&nbsp;<strong id="view_lookup_id_test">Linkin Park</strong>

The second one less obvious when you have two primary keys identifying foreign record:

Failed example:
    print conditional_escape(w.render('test', [m1.pk, m2.pk], attrs={}))
Exception raised:
    Traceback (most recent call last):
      File "/home/marcob/test/lib/django/test/_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest regressiontests.admin_widgets.models.__test__.WIDGETS_TESTS[21]>", line 1, in ?
        print conditional_escape(w.render('test', [m1.pk, m2.pk], attrs={}))
      File "/home/marcob/mytech/lib/django/contrib/admin/widgets.py", line 132, in render
        return super(ManyToManyRawIdWidget, self).render(name, value, attrs)
      File "/home/marcob/mytech/lib/django/contrib/admin/widgets.py", line 106, in render
        output.append(self.label_for_value(value, "id_%s" % name))
    TypeError: label_for_value() takes exactly 2 arguments (3 given)

We need a better patch for the second problem

by Marcob <marcoberi@…>, 16 years ago

This patch leaves only one (trivial) failure running django tests

by Marcob <marcoberi@…>, 16 years ago

Little fix (one liner): now django tests run fine

comment:4 by Marcob <marcoberi@…>, 16 years ago

With these two patches improved-raw-id-admin-feedback-fixed-regression-test.diff and
tests-fix-for-improved-raw-id-admin-patch.patch (1.2 kB) all django tests run fine:

----------------------------------------------------------------------
Ran 252 tests in 87.861s

OK

comment:5 by Karen Tracey <kmtracey@…>, 16 years ago

Keywords: nfa-someday added

comment:6 by Simon Greenhill, 16 years ago

Triage Stage: UnreviewedReady for checkin

comment:7 by Julian Bez, 16 years ago

Can somebody have a look at #7923, which is related to that?

comment:8 by Karen Tracey, 15 years ago

#4134 was a dup, also with a patch.

comment:9 by marcoberi@…, 15 years ago

Version: newforms-admin1.0

Patch for 1.0

by marcoberi@…, 15 years ago

by marcob, 15 years ago

patch for django 1.0.1

by marcob, 15 years ago

patch for django 1.1.0 alpha

comment:10 by anonymous, 15 years ago

There is an error, this line in admin_list.py:

result_name = repr(escape(force_unicode(result_id).replace("'", '&prime')))[1:]

Should be:

result_name = repr(escape(force_unicode(result).replace("'", '&prime')))[1:]

comment:11 by Karen Tracey, 15 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

The last comment makes this look not quite ready for checkin. Also there are no tests in the latest patch, not even the earlier posted test changes required to fix test breakage, so it isn't clear what exactly is supposed to be applied. This ticket needs a consolidated patch with tests for the new function before it can be called ready for checkin.

comment:12 by paul.hide@…, 15 years ago

Line 182 of the patch to django/contrib/admin/widgets.py currently reads:
def label_for_value(self, value, nome=):
but this is probably meant be:
def label_for_value(self, value, name=
):
This would not cause any tests to fail since the arg is not (currently) used.

comment:13 by paul.hide@…, 15 years ago

I just noticed that the empty string quotes after nome= and name= did not get through to the last comment that I posted.

in reply to:  13 comment:14 by Karen Tracey, 15 years ago

Replying to paul.hide@gmail.com:

I just noticed that the empty string quotes after nome= and name= did not get through to the last comment that I posted.

They were interpreted as Wiki formatting characters which turned on, and then off, italics. To avoid this you should mark code blocks as preformatted text: http://code.djangoproject.com/wiki/WikiFormatting#Preformattedtext. Also checking how the comment will look via Preview before hitting Submit is always a good idea.

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

milestone: 1.2

comment:16 by mrts, 14 years ago

I've implemented this and also integrated #7923 in my GitHub branch: http://github.com/mrts/django/commits/ticket7028

Admin widget tests pass, but there's a regression in admin views tests -- a spurious False appears into the output, but I haven't yet got time to figure out from where (probably a trivial problem somewhere).

Attaching a patch.

IMHO #7923 can be closed as duplicate.

by mrts, 14 years ago

Attachment: ticket7028.patch added

comment:17 by mrts, 14 years ago

Patch needs improvement: set

comment:18 by Thomas Güttler, 14 years ago

Cc: hv@… added

by mrts, 14 years ago

Attachment: ticket7028-2.patch added

Fixed the mistake that caused False to appear in output when is_popup was False.

comment:19 by mrts, 14 years ago

Tests pass, looks more or less OK to me.

One thing that needs to be resolved is whether the object link should open in a pop-up. That would be more consistent, but would make the corresponding code horrible -- imagine embedding yet another showPopup into the \'-quoted part:

' onclick="opener.dismissRelatedLookupPopup(window, %s, \'<a href=&quot;%s&quot;>%s</a>\'); return false;"'

I'll probably implement this nevertheless.

comment:20 by mrts, 14 years ago

Needs tests: unset
Patch needs improvement: unset
Version: 1.0SVN

Tried my best to create a clean representation of the intent:

            yield mark_safe(u'<%s%s><a href="%s"%s>%s</a></%s>' %
                (table_tag, row_class, url,
                    (cl.is_popup
                        and ' onclick='
                            '"opener.dismissRelatedLookupPopup(window, %s, '
                            "'<a href=&quot;%s&quot; "
                            'onclick=&quot;return showRelatedObjectPopup(this);&quot;'
                            '>%s</a>\'); return false;"' %
                            (result_id, result_url, result_name)
                        or ''),
                    conditional_escape(result_repr), table_tag))

Commit: http://github.com/mrts/django/commit/e95f27ad4b74eef3a9d4e9553e5cfb58f159c10a

Tests pass, also tested to be working in a real site.

Patch upcoming.

by mrts, 14 years ago

Attachment: ticket7028-with_popup.patch added

Related object links open in popups.

comment:21 by mrts, 14 years ago

I'd say my work is completed. Any comments welcome.

comment:22 by mrts, 14 years ago

I'd be much obliged if someone cared to test the ticket7028-with_popup.patch in IE.

comment:23 by marcob, 14 years ago

mrts, I tried it with IE7 but I got dreaded "Error on page". Can't find where is the problem (I tried for at least on hour, sorry :-( IE is a pita to debug). Hope there is someone smarter than me.

comment:24 by mrts, 14 years ago

There are instructions for JavaScript debugging in IE at http://blogs.msdn.com/ie/archive/2004/10/26/247912.aspx .

I'll get to it eventually myself as well, not sure when though.

comment:25 by marcob, 14 years ago

mrts I followed your link and I discovered the "glich". Thank you!

I've data in my database that contain a ' so this line in the popup window link raises a javascript error:

onclick="opener.dismissRelatedLookupPopup(window, 'A014', 'Servizi connessi all&#39;Agricoltura'); return false;"

The problem is that &#39;

With IE it's enough to open the popup window to get the error, with Firefox you need to click that line. For this I never got it with Firefox, at least once IE helped me :-).

comment:26 by mrts, 14 years ago

Fixed with .replace("'", "`") as was originally intended (but incorrectly implemented). I wonder if this is causes problems for Romanic languages that use ` for accents -- Marco, is it OK for Italian? I.e. you would see Servizi connessi all`Agricoltura instead of Servizi connessi all'Agricoltura in the object link. Also, I wonder if English speakers would be annoyed by seeing Aesop`s fables instead of Aesop's fables.

Other fixes:

  • more than 7 words are now truncated instead of the somewhat arbitrary 14 in the original patch. Even retaining 7 may be a few too many,
  • proper XSS protection with nameElem.innerHTML = ... html_escape(chosenName) ...;, also get rid of the ugliness mentioned a few comments above
  • add ?_popup=1 to the related object popup and dismiss on save
  • consistent label usage.

Will upload the patch tomorrow.

in reply to:  26 comment:27 by marcob, 14 years ago

Replying to mrts:

Fixed with .replace("'", "`") as was originally intended (but incorrectly implemented). I wonder if this is causes problems for Romanic languages that use ` for accents -- Marco, is it OK for Italian? I.e. you would see Servizi connessi all`Agricoltura instead of Servizi connessi all'Agricoltura in the object link. Also, I wonder if English speakers would be annoyed by seeing Aesop`s fables instead of Aesop's fables.

Unfortunately this isn't correct in italian. We use ' (apostrophe) between truncated words ("all'agricoltura") and we sometimes use ` (back-prime? back-apex? how is it called?) with accented word ("menu`" instead of "menù").

Would it be possible to fix it with .replace("'", r"\'") ?

I tried this manually and the error vanished:

onclick="opener.dismissRelatedLookupPopup(window, 'A014', 'Servizi connessi all\'Agricoltura'); return false;"

Will upload the patch tomorrow.

I'll surely try it. Thanks.

by mrts, 14 years ago

Fixes and improvements as described in the comment.

comment:28 by mrts, 14 years ago

Marco, please review and test when you have time (checking with that same failing case in IE is much appreciated). Using ` was both a hack and a bad idea, thanks for rectifying this (my only excuse, albeit a feeble one, is that it doesn't matter in my language, ` and ' are treated more or less equivalently).

comment:29 by mrts, 14 years ago

Needs tests: set
Patch needs improvement: set

Although the patch is quite usable, it needs

  • more view tests, specifically for the template tag and options.py/ModelAdmin.response_change when the next item is implemented
  • when the object's label changes via the popup, it makes sense to update it with JavaScript in the DOM as well (similarly to dismissAddAnotherPopup), there are TODOs in the patch for the places that need updating.

comment:30 by marcob, 14 years ago

Tested and it works! Both in FF than IE. Thanks!

I was to write about update the label if changed when I saw you already write it in the "it needs" part of you comment.
Just to know, are you willing to do it or you left it as an exercise for the reader? :)

Another minor glitch is that I have a table with a FK on a table of hierarchical codes that has a "self" FK. When I open the first popup window and I click on the inside label for the "self" FK I don't get another popup but it opens in the same window. I'm not sure if it's better in this way or to open a new window but to obtain this behaviour it's enough to name the window with the id of the object.

Btw this is nitpicking. You did a wonderful job.

comment:31 by marcob, 14 years ago

mtrs, after looking at your patch I remembered this comment of mine: http://code.djangoproject.com/ticket/6903#comment:30

Fanny, isn't it? I would like to say "great minds think alike" but perhaps is more proper "great asses fart alike" :-)

comment:32 by marcob, 14 years ago

fanny = funny :) (Italian common error)

comment:33 by mrts, 14 years ago

:)

Your nitpicking is most welcome, thanks!

I plan continue taking care of the patch and I'll think about the popup cascading case as well as implement the things mentioned in the TODO comments. However, it is unclear when I have time for it. But I'd say the patch is usable as-is for those who need it.

comment:34 by marcob, 14 years ago

Mrts, I think that we need also to update the label during an add-another:

diff -r be3cfbfcb85b lib/django/contrib/admin/media/js/admin/RelatedObjectLookups.js
--- a/lib/django/contrib/admin/media/js/admin/RelatedObjectLookups.js   Sat Dec 05 03:10:26 2009 +0100
+++ b/lib/django/contrib/admin/media/js/admin/RelatedObjectLookups.js   Sun Dec 06 06:00:58 2009 +0100
@@ -100,6 +100,13 @@
         SelectBox.add_to_cache(toId, o);
         SelectBox.redisplay(toId);
     }
+    var nameElem = document.getElementById("view_lookup_" + name);
+    if (nameElem) {
+      var chosedIdHref = String(win.location.href).split(/\/add\//)[0] + '/' + newId + '/';
+      nameElem.innerHTML = '<a href="' + chosedIdHref + '" ' +
+       'onclick="return showRelatedObjectPopup(this);">' +
+        html_escape(newRepr) + '</a>';
+    }
     win.close();
 }

Do you agree?

comment:35 by mrts, 14 years ago

Marco, I've been ill and away, sorry for that. Will get back to this soon.

comment:36 by marcob, 14 years ago

Nothing is due and you did most of the job on this patch :-)

comment:37 by mrts, 14 years ago

Marco, good point, but here come some of my notes:

  1. the added block should be within the } else if (elem.nodeName == 'INPUT') { branch,
  2. as win.location.href already is a string, there is no need to cast it,
  3. we should not split() as theoretically it's not impossible that the path already contains an add, e.g. a contrived but not illegal '/add/admin/add/add/ (admin is mounted under /add/, there's an app labeled add), so I propose the following instead: var chosenIdHref = win.location.href.replace(/\/add\/[\/]*$/, '/' + newId + '/'); (that gets rid of the redundant ?_popup=1` as well),
  4. it makes sense to store the escaped copy of newRepr in the beginning and not escape it again.

Thanks! I've updated the patch accordingly, re-merged master and will upload the resulting patch shortly.

by mrts, 14 years ago

Attachment: ticket7028-3.patch added

Merge trunk changes, hadle 'Add another'. Needs a bit more work, but is usable as-is.

by mrts, 14 years ago

Attachment: ticket7028-4.patch added

Removed a leftover from a merge conflict, added Marco to authors.

by mrts, 14 years ago

Attachment: ticket7028-5.patch added

Change field label on page when object is changed via popup. All functionality done.

comment:38 by mrts, 14 years ago

Patch needs improvement: unset

The linked label should properly update now both when objects are added and changed via the popup, so all the required functionality should be there.

Marco and others who are interested in this, please test and report any problems you notice (e.g. I haven't tried in IE).

I'll leave it into "more tests needed" state as of now -- the options.py:response_change():elif request.POST.has_key("_popup") path needs testing as do quirky PKs and other corner cases.

Another minor glitch is that I have a table with a FK on a table of hierarchical codes that has a "self" FK. When I open the first popup window and I click on the inside label for
the "self" FK I don't get another popup but it opens in the same window. I'm not sure if it's better in this way or to open a new window but to obtain this behaviour it's enough
to name the window with the id of the object.

I'd say handling this is not worth the effort. But you are most welcome to tinker with the case.

comment:39 by mrts, 14 years ago

I've added selection clearing via javascript with the red x button. The ID input field (and the confusing numeric ID) can be entirely hidden from end-users with CSS display: none now with no loss of functionality.

comment:40 by mrts, 14 years ago

Patch needs improvement: set

Note that I haven't even thought about ManyToManyFields, but the patch has to handle them as well eventually. Will deal with it later, left a failing test as a remainder.

by mrts, 14 years ago

Attachment: ticket7028-6.patch added

Add field clearing with red x.

by mrts, 14 years ago

Attachment: ticket7028-7.patch added

The clearing button only appears if an object is present, next to it. Commit: http://github.com/mrts/django/commit/267b1fea2518b2a39c35708b21b39ebeb069b0ee

comment:41 by mrts, 14 years ago

I've been asked for a 1.1 version by email. See http://github.com/mrts/django/tree/ticket7028-1.1.X .

comment:42 by mrts, 14 years ago

While working at this, it would make sense also to fix #5704, #7955 and perhaps #11683, #11684 as well.

comment:43 by mrts, 14 years ago

A holistic approach would also take care of #11700.

comment:44 by marcob, 14 years ago

mrts, I do agree, but who is in charge to close and link those tickets to this one?

comment:45 by mrts, 14 years ago

A core committer should do this I think. I will presently leave the patch as-is unless instructed otherwise.

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

Keywords: design_ux added

comment:47 by mrts, 14 years ago

I created a 3-minute screencast of the new functionality, to make high-level reviewing this easier: http://www.vimeo.com/9322083

comment:48 by marcob, 14 years ago

Mart, one word: GREAT!

comment:49 by James Bennett, 14 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:50 by dschruth, 14 years ago

I hate to do this, but the functionality is merely only close to what I need but not quite there yet. I happen to like (or tolerate rather) the dropdown box for the ForeignKeyField (probably because I make extensive use of def unicode to make my listed names more verbose/descriptive)... and have no need for the pop-up functionality.

Consequently, all I need would be just the linking functionality to the selected Foreign Object. I envision something similar to the way in which the ImageField and the FileField link to a URL of the file/image: with the link *just* *above* the "select" HTML form element.

I've reopened this ticket's 'duplicate' here http://code.djangoproject.com/ticket/12085 for the first issue.

Additionally: It seems like there could be a similarly nice way to link to the foreign key's class' change_list too. Perhaps we could just turn the field's label into a simple link to the class' list table? This second idea should probably be opened up as a separate ticket though...

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

#13221 was submitted with an alternate patch.

comment:52 by carwyn, 14 years ago

What's blocking progress on one or other of these patches? There seems to be a lot of good work here on a worthy improvement to the admin interface.

In absence of accepted patches does it make sense to re-craft these patches as widgets that could be used with ModelAdmin.formfield_overrides?

comment:53 by mrts, 14 years ago

My patch is complete, but doesn't touch ManyToManyFields. I can not drive this further though for various reasons, so someone else should step up and continue the work.

by stanislas.guerra@…, 14 years ago

Attachment: patch7028-8.patch added

Patch for Django 1.2

comment:54 by mrts, 14 years ago

getAdminMediaPrefix() is unneccessary in 1.2. See #11967 and [13002]. Use if (window.__admin_media_prefix__ != undefined) { ... etc.

comment:55 by EmilStenstrom, 13 years ago

While you wait for this ticket to be resolved, I've added a snippet that shows the values for both ForeignKeys and ManyToManyFields, and links each value to its change page in the admin. If this is something that can be used in this patch, feed free to pick whatever you want from it: http://djangosnippets.org/snippets/2217/

by marcob, 13 years ago

Attachment: patch7028-1.2.4.patch added

Patch against 1.2.4 (no patch for tests, save file admin_list.py with unix fileformat)

comment:56 by Luke Plant, 13 years ago

Severity: Normal
Type: New feature

by stan <stan__at__slashdev.me>, 13 years ago

Attachment: patch7028-1.3.patch added

Previous patch adapted to 1.3

comment:57 by Dave Naffziger, 13 years ago

Cc: davenaff@… added
Easy pickings: unset

comment:58 by Dave Naffziger, 13 years ago

Easy pickings: set

Hmm, just meant to add me to cc list - didn't mean to change easy pickings. Setting easy pickings.

comment:59 by Luke Plant, 13 years ago

Easy pickings: unset

Hmm, that message is confusing lots of people. (it was technically 'null' before, which counts as 'false', which was correct, but then your update forced it to be explicitly 'false' i.e. unset, which was still correct. Until you toggled it :-)

comment:60 by Julien Phalip, 13 years ago

UI/UX: set

by Stanislas Guerra <stan__at__slashdev.me>, 13 years ago

Patch against 1.4 Alpha

comment:61 by Stanislas Guerra <stan__at__slashdev.me>, 13 years ago

Patch against trunk added.

comment:62 by Ramiro Morales, 13 years ago

Latest path is using AdminSite's root_path that doesn't exit anymore (see r16575). Also, I don't like it is reintroducing using '../../..'-style paths we are trying to move away from (see r16578, #15294)

by Stan <stan__at__slashdev.me>, 13 years ago

Patch against latest trunk. Get rid of useless `get_related_url()' function.

by Stan <stan__at__slashdev.me>, 13 years ago

Oups. Uploaded the wrong file (this one replace the previous upload).

by Stan <stan__at__slashdev.me>, 13 years ago

Fix the `ValueError' raised before validation when non-integer value is enter.

in reply to:  62 comment:63 by Stan <stan__at__slashdev.me>, 13 years ago

Replying to ramiro:

Latest path is using AdminSite's root_path that doesn't exit anymore (see r16575). Also, I don't like it is reintroducing using '../../..'-style paths we are trying to move away from (see r16578, #15294)

The latest patch is root_path and ../.. free.

comment:64 by Julien Phalip, 13 years ago

This looks really good! However, it really needs to support ManyToManyFields before it can be included in core. A list builder widget, as specified in [1], could be a nice solution.

[1] http://wiki.jqueryui.com/w/page/12137993/ListBuilder

comment:65 by Julien Phalip, 12 years ago

See also #10293 re: ManyToMany raw_id_fields.

comment:66 by Carlos Palol, 12 years ago

Cc: carlos.palol@… added

in reply to:  64 comment:67 by Stanislas <stan@…>, 12 years ago

Cc: stan@… added

Replying to julien:

This looks really good! However, it really needs to support ManyToManyFields before it can be included in core. A list builder widget, as specified in [1], could be a nice solution.

[1] http://wiki.jqueryui.com/w/page/12137993/ListBuilder

That's nice indeed but for the sake of consistency the same interface should also be ported to the ForeignKey's raw_id. This mean to completely rethink this patch.

And what about the non-javascript users ?

Merging #10293 looks more reasonable and simpler to me.

But I like the idea of an auto-completion (and so my users) !

by Stanislas <stan@…>, 12 years ago

Patch to handle unregistred FK. +Regression tests.

by Stanislas <stan@…>, 12 years ago

ManyToMany raw_id_field label added.

comment:68 by Stanislas <stan@…>, 12 years ago

ManyToMany raw_id_field: Maybe we could add a "Clear" button for each labeled object in order to javascriptly remove its Id in the input ?

comment:69 by Stephane "Twidi" Angel, 12 years ago

Cc: Stephane "Twidi" Angel added

by Stanislas <stanislas.guerra@…>, 11 years ago

Attachment: patch7028-1.4.1.patch added

Patch against Django-1.4.1.

comment:70 by anonymous, 11 years ago

This patch looks great!

What's the status of this patch? Is any work being done to include it in core?

Thanks a lot!

comment:71 by Jacob, 11 years ago

#10293 was a dup.

comment:72 by Stan@…, 11 years ago

Add patch against 1.4.5.

Fix a bug contrib.admin.ManyToManyRawIdWidget#label_for_value(self, value, name) when value was empty.

by Stan@…, 11 years ago

Attachment: patch7028-1.4.5.patch added

Fix a bug in M2M label when empty.

comment:73 by Stanislas, 11 years ago

Fixed `Clear' button malfunction in admin raw_id fields.

Github branch : https://github.com/Starou/django/tree/ticket_7028_1_4

comment:74 by Julien Phalip, 11 years ago

I've worked on a reasonably straightforward implementation (1) for this inspired from DrMeers' "cooked_ids" (2). I've also written a number of selenium tests. It has the advantage of preserving raw_id_fields if you still want them (for whatever reason), while allowing you to use the more user-friendly version (here called "token_fields").

The main piece that's still missing from this implementation is the ability to work with non-integer primary keys.

Any feedback?

(1) https://bitbucket.org/drmeers/django-generic/src/bd20113dce83a5ba6db30a96f0493b93e3aeea17/generic/admin/mixins/cooking.py
(2) https://github.com/jphalip/django/compare/django:04489c7dbf8f69de84ca272a0a1710e7b6067e9d...ticket-7028-token-fields

by Julien Phalip, 11 years ago

First look for the suggested token_fields implementation

comment:75 by stanislas.guerra@…, 11 years ago

@Julien : I have backported your patch to 1.5.x and tested it. The concern I have is about the lack of the hyperlink to *admin_change* (but maybe it is safer to not provide it rather provide it badly (i.e. permission check)) and the clean button for the ForeignKey.

Anyway, looks good to me !

in reply to:  75 comment:77 by stanislas.guerra@…, 11 years ago

Replying to stanislas.guerra@…:

[...] and the clean button for the ForeignKey.

My mistake, the clean button is there.

Anyway, looks good to me !

comment:78 by stanislas.guerra@…, 11 years ago

But this does not works with inlines. Using token_fields into an InlineModelAdmin gives 404 in ajax call to render the FK unicode().

For example, I have added in the test the following models:

class TicketDealer(models.Model):
    name = models.CharField(blank=False, max_length=20)
    website = models.URLField(blank=True)

    def __unicode__(self):
        return self.name

class TokenFieldTicketDealerEvent(models.Model):
    """
    A model that has token fields in the admin inline.
    """
    dealer = models.ForeignKey(TicketDealer)
    event = models.ForeignKey(TokenFieldEvent)

And the admin:

class TokenFieldTicketDealerEventAdminInline(admin.StackedInline):
    model = models.TokenFieldTicketDealerEvent
    token_fields = ('dealer',)

class TokenFieldEventAdmin(admin.ModelAdmin):
    token_fields = ['main_band', 'supporting_bands']
    inlines = (TokenFieldTicketDealerEventAdminInline,)

With the TestCase:

@override_settings(PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',))
class AdminInlineTokenWidgetSeleniumFirefoxTests(AdminSeleniumWebDriverTestCase):
    available_apps = ['admin_widgets'] + AdminSeleniumWebDriverTestCase.available_apps
    fixtures = ['admin-widgets-users.xml']
    urls = "admin_widgets.urls"
    webdriver_class = 'selenium.webdriver.firefox.webdriver.WebDriver'

    def setUp(self):
        band = models.Band.objects.create(id=42, name='Bogey Blues')
        event = models.TokenFieldEvent.objects.create(id=11, main_band=band)
        dealer1 = models.TicketDealer.objects.create(id=12, name="Cheap Horse", website="www.ch.com")
        dealer2 = models.TicketDealer.objects.create(id=21, name="Decent Buy", website="www.db.com")
        models.TokenFieldTicketDealerEvent.objects.create(event=event, dealer=dealer1)
        super(AdminInlineTokenWidgetSeleniumFirefoxTests, self).setUp()

    def test_foreignkey(self):
        self.admin_login(username='super', password='secret', login_url='/')
        self.selenium.get(
            '%s%s' % (self.live_server_url, '/admin_widgets/tokenfieldevent/11/'))
        main_window = self.selenium.current_window_handle

It fires http://localhost:8081/admin_widgets/tokenfieldevent/objects-by-ids/tokenfieldticketdealerevent_set-0-dealer/12/ -> 404

I though it was just a bug in retrieving the field name into RelatedObjectLookups.js#updateTokenField() and had it patched like that:

diff --git a/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js b/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
index 5d8a19c..b333cfd 100644
--- a/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
+++ b/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
@@ -108,7 +108,7 @@ var updateTokenField = function(field) {
     var $field = $(field);
     $field.hide();
     var container = $field.closest('div');
-    var field_name = $field.attr('name');
+    var field_name = getTokenFieldName(field);
     var pks = encodeURI($field.val());
     if (pks) {
         var objects_url = '../objects-by-ids/' + field_name + '/' + pks + '/'; // FIXME: hard-coded url
@@ -128,6 +128,18 @@ var updateTokenField = function(field) {
     }
 };
 
+var getTokenFieldName = function(field) {
+    // inline fields are prefixed.
+    var $ = django.jQuery;
+    var $field = $(field);
+    var rx_prefix = new RegExp("\\\w+-\\\d{1,2}-\(\\\w+\)");
+    var field_name = $field.attr('name');
+    if (field_name.match(rx_prefix)) {
+        field_name = field_name.match(rx_prefix)[1];
+    }
+    return field_name;
+}
+
 var removeToken = function(removeLink, field) {
     var $ = django.jQuery;
     var li = $(removeLink).parent();

Now It fires an Ajax call to http://localhost:8081/admin_widgets/tokenfieldevent/objects-by-ids/dealer/12/ -> 404, dealer not in TokenFieldEventAdmin.token_fields.
This is because in RelatedOjectLookup.js (l.158) the ajax call is on ../objects-by-ids/et/caetera/ which is in the "path" of the parent ModelAdmin.
In fact, there is not any url to call at this point because InlineModelAdmin can't register any url.

Any idea on the direction to take ? allow InlineModelAdmin to register url ? Where (under its ModelAdmin or not) ?

Thanks.

comment:79 by Marco De Paoli, 10 years ago

Cc: depaolim@… added

comment:80 by Collin Anderson, 9 years ago

Keywords: raw_id_fields added

comment:81 by Collin Anderson, 9 years ago

Summary: Better raw_id_fields feedback in newform-admins branchBetter admin raw_id_fields feedback

comment:82 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:83 by Hugo Osvaldo Barrera, 8 years ago

Cc: hugo@… added

comment:84 by Tim Graham, 7 years ago

Resolution: wontfix
Status: newclosed

I think it doesn't make sense to add this enhancement while also adding a new autocomplete widget (#14370). After the autocomplete widget is added, we might deprecate raw_id_fields in some later release.

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