Opened 15 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@…> 15 years ago.
immediate raw-id-fields selection feedback
improved-raw-id-admin-feedback-fixed-insertion.diff (3.4 KB) - added by Marcob <marcoberi@…> 15 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@…> 15 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@…> 15 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 14 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 13 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 13 years ago.
Removed a leftover from a merge conflict, added Marco to authors.
ticket7028-5.patch (19.3 KB) - added by mrts 13 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 13 years ago.
Add field clearing with red x.
ticket7028-7.patch (22.9 KB) - added by mrts 13 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@… 13 years ago.
Patch for Django 1.2
patch7028-1.2.4.patch (15.7 KB) - added by marcob 12 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> 12 years ago.
Previous patch adapted to 1.3
patch7028-1.4.rev16590.patch (14.6 KB) - added by Stanislas Guerra <stan__at__slashdev.me> 12 years ago.
Patch against 1.4 Alpha
patch7028-1.4.rev16669.patch (13.6 KB) - added by Stan <stan__at__slashdev.me> 12 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> 12 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> 12 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@… 10 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 10 years ago.
First look for the suggested token_fields implementation

Download all attachments as: .zip

Change History (112)

Changed 15 years ago by Marcob <marcoberi@…>

immediate raw-id-fields selection feedback

comment:2 Changed 15 years ago by Marcob <marcoberi@…>

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

Changed 15 years ago by Marcob <marcoberi@…>

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

comment:3 Changed 15 years ago by Marcob <marcoberi@…>

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

Changed 15 years ago by Marcob <marcoberi@…>

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

Changed 15 years ago by Marcob <marcoberi@…>

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

comment:4 Changed 15 years ago by Marcob <marcoberi@…>

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 Changed 15 years ago by Karen Tracey <kmtracey@…>

Keywords: nfa-someday added

comment:6 Changed 15 years ago by Simon Greenhill

Triage Stage: UnreviewedReady for checkin

comment:7 Changed 15 years ago by Julian Bez

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

comment:8 Changed 15 years ago by Karen Tracey

#4134 was a dup, also with a patch.

comment:9 Changed 15 years ago by marcoberi@…

Version: newforms-admin1.0

Patch for 1.0

Changed 15 years ago by marcoberi@…

Changed 15 years ago by marcob

patch for django 1.0.1

Changed 14 years ago by marcob

patch for django 1.1.0 alpha

comment:10 Changed 14 years ago by anonymous

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 Changed 14 years ago by Karen Tracey

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 Changed 14 years ago by paul.hide@…

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 Changed 14 years ago by paul.hide@…

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

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

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 Changed 14 years ago by Russell Keith-Magee

milestone: 1.2

comment:16 Changed 14 years ago by mrts

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.

Changed 14 years ago by mrts

Attachment: ticket7028.patch added

comment:17 Changed 14 years ago by mrts

Patch needs improvement: set

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

Cc: hv@… added

Changed 14 years ago by mrts

Attachment: ticket7028-2.patch added

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

comment:19 Changed 14 years ago by mrts

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 Changed 14 years ago by mrts

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.

Changed 14 years ago by mrts

Attachment: ticket7028-with_popup.patch added

Related object links open in popups.

comment:21 Changed 14 years ago by mrts

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

comment:22 Changed 14 years ago by mrts

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

comment:23 Changed 14 years ago by marcob

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 Changed 14 years ago by mrts

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 Changed 14 years ago by marcob

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 Changed 14 years ago by 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.

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.

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

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.

Changed 14 years ago by mrts

Fixes and improvements as described in the comment.

comment:28 Changed 14 years ago by mrts

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 Changed 14 years ago by mrts

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 Changed 14 years ago by marcob

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 Changed 14 years ago by marcob

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 Changed 14 years ago by marcob

fanny = funny :) (Italian common error)

comment:33 Changed 14 years ago by mrts

:)

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 Changed 14 years ago by marcob

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 Changed 13 years ago by mrts

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

comment:36 Changed 13 years ago by marcob

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

comment:37 Changed 13 years ago by mrts

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.

Changed 13 years ago by mrts

Attachment: ticket7028-3.patch added

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

Changed 13 years ago by mrts

Attachment: ticket7028-4.patch added

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

Changed 13 years ago by mrts

Attachment: ticket7028-5.patch added

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

comment:38 Changed 13 years ago by mrts

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 Changed 13 years ago by mrts

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 Changed 13 years ago by mrts

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.

Changed 13 years ago by mrts

Attachment: ticket7028-6.patch added

Add field clearing with red x.

Changed 13 years ago by mrts

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 Changed 13 years ago by mrts

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

comment:42 Changed 13 years ago by mrts

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

comment:43 Changed 13 years ago by mrts

A holistic approach would also take care of #11700.

comment:44 Changed 13 years ago by marcob

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

comment:45 Changed 13 years ago by mrts

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

comment:46 Changed 13 years ago by Russell Keith-Magee

Keywords: design_ux added

comment:47 Changed 13 years ago by mrts

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

comment:48 Changed 13 years ago by marcob

Mart, one word: GREAT!

comment:49 Changed 13 years ago by James Bennett

milestone: 1.2

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

comment:50 Changed 13 years ago by dschruth

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 Changed 13 years ago by Russell Keith-Magee

#13221 was submitted with an alternate patch.

comment:52 Changed 13 years ago by carwyn

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 Changed 13 years ago by mrts

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.

Changed 13 years ago by stanislas.guerra@…

Attachment: patch7028-8.patch added

Patch for Django 1.2

comment:54 Changed 13 years ago by mrts

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

comment:55 Changed 13 years ago by EmilStenstrom

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/

Changed 12 years ago by marcob

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 Changed 12 years ago by Luke Plant

Severity: Normal
Type: New feature

Changed 12 years ago by stan <stan__at__slashdev.me>

Attachment: patch7028-1.3.patch added

Previous patch adapted to 1.3

comment:57 Changed 12 years ago by Dave Naffziger

Cc: davenaff@… added
Easy pickings: unset

comment:58 Changed 12 years ago by Dave Naffziger

Easy pickings: set

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

comment:59 Changed 12 years ago by Luke Plant

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 Changed 12 years ago by Julien Phalip

UI/UX: set

Changed 12 years ago by Stanislas Guerra <stan__at__slashdev.me>

Patch against 1.4 Alpha

comment:61 Changed 12 years ago by Stanislas Guerra <stan__at__slashdev.me>

Patch against trunk added.

comment:62 Changed 12 years ago by Ramiro Morales

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)

Changed 12 years ago by Stan <stan__at__slashdev.me>

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

Changed 12 years ago by Stan <stan__at__slashdev.me>

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

Changed 12 years ago by Stan <stan__at__slashdev.me>

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

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

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 Changed 12 years ago by Julien Phalip

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 Changed 12 years ago by Julien Phalip

See also #10293 re: ManyToMany raw_id_fields.

comment:66 Changed 12 years ago by Carlos Palol

Cc: carlos.palol@… added

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

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

Changed 12 years ago by Stanislas <stan@…>

Patch to handle unregistred FK. +Regression tests.

Changed 12 years ago by Stanislas <stan@…>

ManyToMany raw_id_field label added.

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

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 Changed 12 years ago by Stephane "Twidi" Angel

Cc: Stephane "Twidi" Angel added

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

Attachment: patch7028-1.4.1.patch added

Patch against Django-1.4.1.

comment:70 Changed 10 years ago by anonymous

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 Changed 10 years ago by Jacob

#10293 was a dup.

comment:72 Changed 10 years ago by Stan@…

Add patch against 1.4.5.

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

Changed 10 years ago by Stan@…

Attachment: patch7028-1.4.5.patch added

Fix a bug in M2M label when empty.

comment:73 Changed 10 years ago by Stanislas

Fixed `Clear' button malfunction in admin raw_id fields.

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

comment:74 Changed 10 years ago by Julien Phalip

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

Changed 10 years ago by Julien Phalip

First look for the suggested token_fields implementation

comment:75 Changed 10 years ago by stanislas.guerra@…

@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 !

comment:77 in reply to:  75 Changed 10 years ago by stanislas.guerra@…

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 Changed 10 years ago by stanislas.guerra@…

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 Changed 9 years ago by Marco De Paoli

Cc: depaolim@… added

comment:80 Changed 9 years ago by Collin Anderson

Keywords: raw_id_fields added

comment:81 Changed 9 years ago by Collin Anderson

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

comment:82 Changed 8 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:83 Changed 7 years ago by Hugo Osvaldo Barrera

Cc: hugo@… added

comment:84 Changed 7 years ago by Tim Graham

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