Opened 7 years ago

Last modified 7 months ago

#7028 new New feature

Better admin raw_id_fields feedback

Reported by: Marcob <marcoberi@…> Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: raw-id-fields nfa-someday design_ux raw_id_fields
Cc: hv@…, davenaff@…, carlos.palol@…, stan@…, Twidi, depaolim@…, cmawebsite@… 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@…> 7 years ago.
immediate raw-id-fields selection feedback
improved-raw-id-admin-feedback-fixed-insertion.diff (3.4 KB) - added by Marcob <marcoberi@…> 7 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@…> 7 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@…> 7 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@… 7 years ago.
improved-raw-id-admin-django-1.0.1.diff (3.7 KB) - added by marcob 7 years ago.
patch for django 1.0.1
improved-raw-id-admin-django-1.0.1.2.diff (3.8 KB) - added by marcob 7 years ago.
patch for django 1.1.0 alpha
ticket7028.patch (12.7 KB) - added by mrts 6 years ago.
ticket7028-2.patch (12.3 KB) - added by mrts 6 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 6 years ago.
Related object links open in popups.
ticket7028-with_popup_2.patch (17.4 KB) - added by mrts 6 years ago.
Fixes and improvements as described in the comment.
ticket7028-3.patch (18.5 KB) - added by mrts 6 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 6 years ago.
Removed a leftover from a merge conflict, added Marco to authors.
ticket7028-5.patch (19.3 KB) - added by mrts 6 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 6 years ago.
Add field clearing with red x.
ticket7028-7.patch (22.9 KB) - added by mrts 6 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@… 5 years ago.
Patch for Django 1.2
patch7028-1.2.4.patch (15.7 KB) - added by marcob 5 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> 4 years ago.
Previous patch adapted to 1.3
patch7028-1.4.rev16590.patch (14.6 KB) - added by Stanislas Guerra <stan__at__slashdev.me> 4 years ago.
Patch against 1.4 Alpha
patch7028-1.4.rev16669.patch (13.6 KB) - added by Stan <stan__at__slashdev.me> 4 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> 4 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> 4 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@…> 4 years ago.
Patch to handle unregistred FK. +Regression tests.
patch7028-1.4.rev16961.2.patch (22.1 KB) - added by Stanislas <stan@…> 4 years ago.
ManyToMany raw_id_field label added.
patch7028-1.4.1.patch (23.3 KB) - added by Stanislas <stanislas.guerra@…> 3 years ago.
Patch against Django-1.4.1.
patch7028-1.4.5.patch (23.3 KB) - added by Stan@… 2 years ago.
Fix a bug in M2M label when empty.
Add_token_field_event___Django_site_admin.png (20.0 KB) - added by julien 2 years ago.
First look for the suggested token_fields implementation

Download all attachments as: .zip

Change History (110)

Changed 7 years ago by Marcob <marcoberi@…>

immediate raw-id-fields selection feedback

comment:1 Changed 7 years ago by Marcob <marcoberi@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 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 7 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 7 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 7 years ago by Marcob <marcoberi@…>

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

Changed 7 years ago by Marcob <marcoberi@…>

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

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

  • Keywords nfa-someday added

comment:6 Changed 7 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:7 Changed 7 years ago by julianb

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

comment:8 Changed 7 years ago by kmtracey

#4134 was a dup, also with a patch.

comment:9 Changed 7 years ago by marcoberi@…

  • Version changed from newforms-admin to 1.0

Patch for 1.0

Changed 7 years ago by marcoberi@…

Changed 7 years ago by marcob

patch for django 1.0.1

Changed 7 years ago by marcob

patch for django 1.1.0 alpha

comment:10 Changed 7 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 6 years ago by kmtracey

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

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 6 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 follow-up: Changed 6 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 6 years ago by kmtracey

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

  • milestone set to 1.2

comment:16 Changed 6 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 6 years ago by mrts

comment:17 Changed 6 years ago by mrts

  • Patch needs improvement set

comment:18 Changed 6 years ago by guettli

  • Cc hv@… added

Changed 6 years ago by mrts

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

comment:19 Changed 6 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 6 years ago by mrts

  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.0 to SVN

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

Related object links open in popups.

comment:21 Changed 6 years ago by mrts

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

comment:22 Changed 6 years ago by mrts

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

comment:23 Changed 6 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 6 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 6 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 follow-up: Changed 6 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 6 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 6 years ago by mrts

Fixes and improvements as described in the comment.

comment:28 Changed 6 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 6 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 6 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 6 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 6 years ago by marcob

fanny = funny :) (Italian common error)

comment:33 Changed 6 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 6 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 6 years ago by mrts

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

comment:36 Changed 6 years ago by marcob

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

comment:37 Changed 6 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 6 years ago by mrts

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

Changed 6 years ago by mrts

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

Changed 6 years ago by mrts

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

comment:38 Changed 6 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 6 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 6 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 6 years ago by mrts

Add field clearing with red x.

Changed 6 years ago by mrts

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

comment:41 Changed 6 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 6 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 6 years ago by mrts

A holistic approach would also take care of #11700.

comment:44 Changed 6 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 6 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 6 years ago by russellm

  • Keywords design_ux added

comment:47 Changed 6 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 6 years ago by marcob

Mart, one word: GREAT!

comment:49 Changed 6 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:50 Changed 5 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 5 years ago by russellm

#13221 was submitted with an alternate patch.

comment:52 Changed 5 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 5 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 5 years ago by stanislas.guerra@…

Patch for Django 1.2

comment:54 Changed 5 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 5 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 5 years ago by marcob

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

comment:56 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

Changed 4 years ago by stan <stan__at__slashdev.me>

Previous patch adapted to 1.3

comment:57 Changed 4 years ago by davenaff

  • Cc davenaff@… added
  • Easy pickings unset

comment:58 Changed 4 years ago by davenaff

  • 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 4 years ago by lukeplant

  • 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 4 years ago by julien

  • UI/UX set

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

Patch against 1.4 Alpha

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

Patch against trunk added.

comment:62 follow-up: Changed 4 years ago by 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)

Changed 4 years ago by Stan <stan__at__slashdev.me>

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

Changed 4 years ago by Stan <stan__at__slashdev.me>

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

Changed 4 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 4 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 follow-up: Changed 4 years ago by 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

comment:65 Changed 4 years ago by julien

See also #10293 re: ManyToMany raw_id_fields.

comment:66 Changed 4 years ago by carlospalol

  • Cc carlos.palol@… added

comment:67 in reply to: ↑ 64 Changed 4 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 4 years ago by Stanislas <stan@…>

Patch to handle unregistred FK. +Regression tests.

Changed 4 years ago by Stanislas <stan@…>

ManyToMany raw_id_field label added.

comment:68 Changed 4 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 4 years ago by Twidi

  • Cc Twidi added

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

Patch against Django-1.4.1.

comment:70 Changed 2 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 2 years ago by jacob

#10293 was a dup.

comment:72 Changed 2 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 2 years ago by Stan@…

Fix a bug in M2M label when empty.

comment:73 Changed 2 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 2 years ago by julien

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 2 years ago by julien

First look for the suggested token_fields implementation

comment:75 follow-up: Changed 2 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 2 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 2 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 15 months ago by depaolim

  • Cc depaolim@… added

comment:80 Changed 11 months ago by collinanderson

  • Keywords raw_id_fields added

comment:81 Changed 11 months ago by collinanderson

  • Summary changed from Better raw_id_fields feedback in newform-admins branch to Better admin raw_id_fields feedback

comment:82 Changed 7 months ago by collinanderson

  • Cc cmawebsite@… added
Note: See TracTickets for help on using tickets.
Back to Top