Opened 9 years ago
Closed 8 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 , 9 years ago
comment:2 by , 9 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Type: | Uncategorized → Cleanup/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 , 9 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:5 by , 9 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 , 8 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:9 by , 8 years ago
| Has patch: | set |
|---|
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.