Opened 8 years ago

Closed 7 years ago

#26357 closed Cleanup/optimization (fixed)

'related-lookup' and 'add-another' popups don't work for admin fields added via ajax

Reported by: Julian Andrews Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin javascript ajax
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

If a field with a 'related-lookup' or 'add-another' link is added after page load (for instance via ajax), the javascript event handler which causes these links to open a pop-up doesn't get attached to the new links. Instead of opening a pop-up window to allow selection of the related object, the links simply send the user to the list page for the object.

As a result ajax generated admin fields don't work correctly. While this doesn't break the Django admin itself, it does make extending the admin with Ajax difficult.

An easy fix is to attach the 'related-lookup' and 'add-another' click events to document instead of to the links themselves. This would come at a small performance cost since the events will be triggered on every click instead of just on link click, but with modern javascript engines I think the cost would be unnoticeable, and given how unobtrusive this change is I think the added flexibility justifies the cost.

I'll attach a PR for the proposed fix after I submit this report.

Change History (10)

comment:1 by Julian Andrews, 8 years ago

I've created a patch here: https://github.com/fusionbox/django/tree/ticket_26357

It's a fairly simple fix, but since I can see arguments against this approach, I've held off on making a PR directly. If I get the go ahead, I'll make a PR on this patch.

comment:2 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

The main concern I have is how to ensure the admin continues to support such use cases in the future. I guess we can say "no tests = no official support" and you can continue sending PRs in the future if things inadvertently break? At least you might want to add a comment about it so someone doesn't "cleanup" the code to the old version.

comment:3 by Julian Andrews, 8 years ago

@timgraham: I've gone ahead and added comments.

I agree that maintaining future support for the use case would be nice, but I'm not sure exactly what that would look like. Given how common ajax and dynamic page content is now, it's probably a good practice to avoid writing javascript that assumes a static page structure. Maybe a line to that effect could be added to https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/javascript/?

In any case, Fusionbox relies on ajax in the admin somewhat heavily so we're likely to catch inadvertent breakage down the line and send PRs.

comment:4 by Tim Graham, 8 years ago

Sure, some documented guidelines wouldn't hurt.

comment:5 by Julian Andrews, 8 years ago

@timgraham I've added my best go at a documentation improvement.

I'm not 100% happy with it just because I'm not sure it's the best idea to encourage binding events to the document, so take a look and I'll either submit this as a PR as is, or rollback the documentation change and just submit the first commit.

comment:6 by Julian Andrews, 7 years ago

@timgraham I completely lost track of this issue, and just discovered it again. I could rebase my changes, make sure it all still works, and submit a PR. Would that be welcome?

comment:7 by Tim Graham, 7 years ago

Yes, creating a pull request for review seems to be the next step.

comment:8 by Julian Andrews, 7 years ago

comment:9 by Tim Graham, 7 years ago

Has patch: set

comment:10 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In adc93e85:

Fixed #26357 -- Allowed admin popups to work on links added after page load.

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