#6903 closed New feature (fixed)
Go back to old change_list view after hitting save
Reported by: | Owned by: | Tomek Paczkowski | |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | admin |
Cc: | Joshua Cender, Marc Remolt, ramusus@…, Salva Pinyol, marcoberi@…, Johannes Bornhold, hr.bjarni+django@…, rob@…, empty, riccardo.magliocchetti@…, michelts@…, odin.omdal@…, danny.adair@…, Julien Bouquillon, Andy Baker, kraken, tomek@…, Simon Charette, honyczek@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | yes |
Description (last modified by )
When editing something via the admin I could observe a recurring usage pattern on myself:
- Search for something (e.g. by time, filter, phrase) wanting to edit them all or just go through the pages till I found a certain timespan
- Edit
- Hit save
- Hit the back button two times to get my former list
- Repeat with 2. :)
While this may be ok for myself my client is protesting ;) So I thought I'll hack it. But then again, isn't the desired behaviour to go back to the exact collection of stuff in the change list that was there before anyway?
So I'd like to propose to change that. I'm happy to try and write a patch for it, if this get's accepted.
Cheers!
Jonas
Attachments (12)
Change History (114)
comment:1 by , 17 years ago
Keywords: | nfa-someday added |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 17 years ago
Ah, I'll look into the patch. Thx! I still think this should be the default behaviour. The question ist: Why wouldn't I want the filters to be persistent? IMHO the user expects this and not the automatic filter reset. Any thoughts?
comment:3 by , 17 years ago
I agree that preserving the filters is more user-friendly. Personally it doesn't bother me too much in Django admin but that's because I don't happen to use filters in admin that much. However when I run into it in other interfaces it tends to annoy me when some sort of item-edit page's "doit" button returns me to a list that differs from the one I came from to edit the object. So I'd be in favor of the change, but don't have much personal interest in it and think newforms-admin has bigger fish to fry at the moment (specifically anything blocking a merge to trunk).
comment:4 by , 17 years ago
Description: | modified (diff) |
---|
comment:5 by , 16 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
by , 16 years ago
Attachment: | return_to_old_change_list.diff added |
---|
comment:6 by , 16 years ago
Owner: | changed from | to
---|
comment:7 by , 16 years ago
Keywords: | yandex-sprint added |
---|
comment:8 by , 16 years ago
milestone: | → post-1.0 |
---|
comment:9 by , 16 years ago
Cc: | added |
---|
comment:10 by , 16 years ago
Needs tests: | set |
---|---|
Owner: | changed from | to
Patch needs improvement: | set |
comment:11 by , 16 years ago
Cc: | removed |
---|
by , 16 years ago
Attachment: | 6903-changelist-return-to.diff added |
---|
A few updates to ramiro's patch, and updated to r9888
follow-up: 13 comment:12 by , 16 years ago
I've made a few stylistic improvements to the patch, but it still needs some fundamental improvement.
The main problem is that "guessing" the return URL from HTTP_REFERER leads to strange behavior. For example:
- Click "add" from the main admin index, then save the object. Result: you're returned to the index page. No, you should go to the changelist to inspect the object you just added.
- Click "add" from anywhere. Click "save and add another". Click "save". Result: you're taken back to the last object you created. This is really confusing: it appears identical to clicking "save and continue editing", except you've got different values.
This needs to be fixed. I think probably the best approach is to *only* read the return URL from GET
, and to modify the changelist to pass the return URL through as a GET
variable. That is, rewrite the links from the changelist to include the return URL in the link.
(This means you'll want to choose a different URL keyword to avoid conflicting with a field named return_to
. Probably _return_to
, I'd say.)
Second, this patch needs some tests. Especially ones to make sure that the redirects stay sane in cases like above.
by , 16 years ago
Attachment: | 6903-r9904.diff added |
---|
New patch, including Jacob's changes and suggestions. Thanks Alex Gaynor for his help with the tests
comment:13 by , 16 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Replying to jacob:
it still needs some fundamental improvement.
The main problem is that "guessing" the return URL from HTTP_REFERER leads to strange behavior.
This needs to be fixed. I think probably the best approach is to *only* read the return URL from
GET
, and to modify the changelist to pass the return URL through as aGET
variable. That is, rewrite the links from the changelist to include the return URL in the link.
I've implemented this change in the new 6903-r9904.diff
patch.
(This means you'll want to choose a different URL keyword to avoid conflicting with a field named
return_to
. Probably_return_to
, I'd say.)
done
Second, this patch needs some tests. Especially ones to make sure that the redirects stay sane in cases like above.
done
Jacob, your comment above cleared three of the four doubts I had posted to django-dev
after attaching the first revised version of the patch. The only one remaining is this one (I'm reposting because it's a fundamental one and the answer will tell if the approach taken is valid or if we should consider others options discussed like storing things in the session instead):
- It passes that URL around from view to view by using an query string value until the time to use it comes, it does so even when a POST request is sent (when editing or creating a model instance). Are we ok with this?
comment:15 by , 16 years ago
Cc: | added |
---|
comment:16 by , 16 years ago
Keywords: | nfa-someday yandex-sprint removed |
---|---|
milestone: | → 1.1 |
Version: | newforms-admin → SVN |
follow-up: 19 comment:17 by , 16 years ago
Isn't such a "sticky" behaviour exactly what sessions are for? Why would you not use a session key like
request.session+ <myapp> + '_' + <mymodel> + '_filter' = {'p':14, 'my_fieldexact':True}
and checking for this key when displaying the list of objects of a model?
I'm also offering to try and implement that if this is desired.
comment:18 by , 16 years ago
Cc: | added |
---|
comment:19 by , 16 years ago
Replying to mawimawi:
Isn't such a "sticky" behaviour exactly what sessions are for? Why would you not use a session key like
request.session['admin_' + <myapp> + '_' + <mymodel> + '_filter'] = {'p':14, 'my_field__exact':True}
and checking for this key when displaying the list of objects of a model?
I'm also offering to try and implement that if this is desired.
I'd say go for it and attach it to the ticket so both implementations are available for review and the chances of this feature being available in 1.1 are better.
comment:20 by , 16 years ago
milestone: | 1.1 → 1.1 beta |
---|
follow-up: 22 comment:21 by , 16 years ago
The idea was that you get an unfiltered list when you go to the changelist URL directly (not coming from an edit form of an entry). mawimawi's example code looks like he wants the filters to stay forever. To cite Malcolm "Yes, from the admin home page it should be a clean view."
I proposed to use the session to store the filters so that you don't pass your filters and search queries around whenever you pass the URL of an edit form to someone else. So I would use a hash of the data and use it as a key in the session. This key would then be passed around instead of the info. But this could be changed later, I don't think it would be backwards incompatible.
follow-up: 23 comment:22 by , 16 years ago
Cc: | added |
---|
Replying to jarrow:
The idea was that you get an unfiltered list when you go to the changelist URL directly (not coming from an edit form of an entry). mawimawi's example code looks like he wants the filters to stay forever. To cite Malcolm "Yes, from the admin home page it should be a clean view."
I proposed to use the session to store the filters so that you don't pass your filters and search queries around whenever you pass the URL of an edit form to someone else. So I would use a hash of the data and use it as a key in the session. This key would then be passed around instead of the info. But this could be changed later, I don't think it would be backwards incompatible.
I don't see, why it is a bad thing to be able to post the filter settings around with the url. I consider it a feature, to be able to copy a filtered admin changelist view URL into an email and post it to a colleague to show him exacly what I want him to notice. Using the session would also go against REST (no ambiguous URLs).
Using URLs also automatically solves Malcolms request you mentioned. The admin start page just uses URLs without filter parameters as it always has.
Just my 5 cents.
follow-up: 24 comment:23 by , 16 years ago
I proposed to use the session to store the filters so that you don't pass your filters and search queries around whenever you pass the URL of an edit form to someone else. So I would use a hash of the data and use it as a key in the session. This key would then be passed around instead of the info. But this could be changed later, I don't think it would be backwards incompatible.
I don't see, why it is a bad thing to be able to post the filter settings around with the url. I consider it a feature, to be able to copy a filtered admin changelist view URL into an email and post it to a colleague to show him exacly what I want him to notice. Using the session would also go against REST (no ambiguous URLs).
Using URLs also automatically solves Malcolms request you mentioned. The admin start page just uses URLs without filter parameters as it always has.
Sure. I totally agree :) What I was talking about was not the changelist view itself. Of course that stays like it is. With an URL that you can copy including the filters. What I meant was: While editing an item - coming from a filtered changelist - it might not be so obvious that if you copy the URL from the edit view and pass it to a fried he will be going back to your custom-filtered change list.
comment:24 by , 16 years ago
Replying to jarrow:
I proposed to use the session to store the filters so that you don't pass your filters and search queries around whenever you pass the URL of an edit form to someone else. So I would use a hash of the data and use it as a key in the session. This key would then be passed around instead of the info. But this could be changed later, I don't think it would be backwards incompatible.
I don't see, why it is a bad thing to be able to post the filter settings around with the url. I consider it a feature, to be able to copy a filtered admin changelist view URL into an email and post it to a colleague to show him exacly what I want him to notice. Using the session would also go against REST (no ambiguous URLs).
Using URLs also automatically solves Malcolms request you mentioned. The admin start page just uses URLs without filter parameters as it always has.
Sure. I totally agree :) What I was talking about was not the changelist view itself. Of course that stays like it is. With an URL that you can copy including the filters. What I meant was: While editing an item - coming from a filtered changelist - it might not be so obvious that if you copy the URL from the edit view and pass it to a fried he will be going back to your custom-filtered change list.
Then I got your proposal wrong. I really don't think anyone wants to copy an URL from an edit page with filters enabled. Forget everything I said. (Note to self: READ, then post)
comment:25 by , 16 years ago
milestone: | 1.1 beta → 1.2 |
---|
This has gone around quite a bit, but it's still not quite perfect. It's really a hard problem, unfortunately, and we're out of time to put it in 1.1.
comment:26 by , 16 years ago
Has anyone thought about this issue of "sticky filters" with respect to the breadcrumbs in the changelist? For example, say I filter my objects in the changelist view and then click on one of the objects to go to its change form. Now my breadcrumbs contain something like:
Home > ObjectManager > Objects > Object_I_Clicked_On
It seems to me that if I click on "Objects" in the breadcrumbs, that I would want to go back to my filtered change list, not the unfiltered change list.
I'm mentioning this because it seems like it would be nice to have a solution that could be extended to solve this as well (assuming others agree that the breadcrumbs should return you to the filtered change list).
Margie
comment:27 by , 15 years ago
Cc: | added |
---|
comment:28 by , 15 years ago
Cc: | added |
---|
comment:29 by , 15 years ago
Cc: | added |
---|
comment:30 by , 15 years ago
I use a fancy snippet that open related object in change mode (using few lines of jQuery), so I would like to close the window after saving if in popup mode.
A call to a dismissChangePopup (similar to dismissAddAnotherPopup) would be great, but for now I add this to response_change function after msg:
# Close if editing in a popup if request.POST.has_key("_popup"): return HttpResponse('<script type="text/javascript">window.close();</script>')
comment:31 by , 15 years ago
Owner: | removed |
---|
by , 15 years ago
Attachment: | 6903.r12351.current.diff added |
---|
Without new changes, just for working with 12351 revision in development trunk
comment:32 by , 15 years ago
milestone: | 1.2 |
---|
1.2 is feature-frozen, moving this feature request off the milestone.
comment:33 by , 15 years ago
Cc: | added |
---|
comment:34 by , 15 years ago
See also #12241 that also proposes preserving the query string in another admin use case.
comment:35 by , 14 years ago
Cc: | added |
---|
comment:36 by , 14 years ago
Cc: | added |
---|
comment:37 by , 14 years ago
Cc: | added |
---|
comment:38 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:39 by , 14 years ago
UI/UX: | set |
---|
by , 13 years ago
Attachment: | 6903.r16617.diff added |
---|
Proposed patch. This stores the HTTP_REFERER as a hidden input in the form (if REQUEST.METAHTTP_REFERER exists). On change, the user is redirected back from whence they came.
comment:40 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Accepted |
comment:41 by , 13 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Latest patch needs tests and probably brief docs note on the behavior, not sure if tests from earlier patch apply but they make a good starting place.
comment:42 by , 13 years ago
#3777 was closed as a duplicate. It contains extensive discussions on this topic.
comment:43 by , 13 years ago
Cc: | added |
---|
comment:44 by , 13 years ago
Cc: | added |
---|
comment:45 by , 13 years ago
Cc: | added |
---|
comment:46 by , 13 years ago
Cc: | removed |
---|
comment:47 by , 12 years ago
Cc: | added |
---|
comment:48 by , 12 years ago
Any update on @graveyboat patch ?
Does it work as expected ?
What happens when user click "save and continue" ? The referer is gone so we may need to keep the initial one from POST if specified ?
-> It cannot work as is, working on a update
comment:49 by , 12 years ago
An alternative approach for saving and restoring the filters after editing a model:
comment:50 by , 12 years ago
Cc: | added |
---|---|
Easy pickings: | set |
Patch needs improvement: | set |
Hi
This "bug" is quite annoying for my customers :) (lazy dudes)
I compiled as much info as possible on this thread and started a new patch with current propositions.
I used the _return_to URL parameter approach to pass the base change list url along in the different views and redirect accordingly (change/add/delete/history...).
Works nicely.
I updated some regressiontests that matched a fixed url to support an optional trailing querystring with a assertRegexpMatches.
As i'm not a TDD expert i'd appreciate recommandations on how to make it better and which tests i should add to validate this feature.
Any comment appreciated.
Patched branch here : https://github.com/revolunet/django/compare/bug-6903 (add .patch to the url for the patch)
comment:51 by , 12 years ago
Keywords: | admin added |
---|---|
Needs tests: | unset |
I just added a full selenium test that covers the whole scenario : filter, edit, add, addanother, continue, history, delete...
please review :)
comment:52 by , 12 years ago
Two comments:
- Is there possibility of malicious use of the RETURN_GET_PARAM, that is you could send a link of edit_something?_return_to=evil.com
- There is some repeating of this:
if RETURN_GET_PARAM in request.GET: url += '?%s=%s' % (RETURN_GET_PARAM, urlquote(request.GET.get(RETURN_GET_PARAM, '')))
seems like a little helper method could make this a little more DRY.
This is just after quick skimming, no full review done.
comment:54 by , 12 years ago
I added some comments to the compare view.
For the committer of this: I am not sure if the check that the return url must start with '/' is the right one - so this check should be verified. (Not saying it is the wrong one either - just want to make sure this will be double checked).
I don't think I am the right committer for this, there are committers who know this area of Django way better than me.
comment:55 by , 12 years ago
Due to popular request, i finally replaced the wonderful Selenium Test with a more standard but more quick standard TestCase. (Duration is now 1.5s versus 5s with selenium)
Any others improvements advises here ?
comment:56 by , 12 years ago
Easy pickings: | unset |
---|
Thanks for all your work on the patch so far. I think that overall the redirections work pretty well and quite intuitively.
However, I have some concerns regarding the continuous passing-around of the querystring parameter. Firstly, it makes the implementation more complex as we have to check for the existence of that parameter in many places in the code — not only does this already require a lot of new code in the current implementation, but as we continue to extend the admin's functionality we'll have to continue to add lots of similar code just to ensure that this querystring parameter is correctly taken care of. Secondly, it makes the admin urls really long and ugly — this would have a negative impact on the user experience. Thirdly, while introducing a querystring parameter could be seen as a nice new feature, I fear it might be abused and exploited for too many different things outside of the original use case.
Besides, while I agree that using explicit urls is generally a good thing on the web, in this case I think it is purely a convenient way of navigating the site by one given user. How the changelist was previously filtered by user A before accessing an object is most likely not going to be interesting or useful to any other user B. So there's no reason for user A to provide this information when sharing an object's url with user B.
For all these reasons, my strong preference would be to use sessions instead of a querystring parameter. 1) Each time the changelist is rendered: if any filters are used then store them in the session otherwise clear the session; and 2) When comes the time to redirect to the changelist (i.e. after an object gets saved), then use the filters saved in the session, if any.
The session would be used to save the state of the changelist as it was last seen by the user. If the user opens multiple browser tabs for the changelist and operates multiple different filterings in each tab, then only the last filtering that is operated would be kept in the session.
Also, to be clear, we do not want to have situations where the changelist's filtering doesn't match what is displayed in the browser's address bar. So the filtering saved in the session should only be retrieved when there is a redirection, and the redirection's url should include all the corresponding querystring parameters. This means for example that if you directly click on model name on the admin index page, then you should be sent to the changelist's "clean" url (i.e. without any querystring parameters) and the session's filtering should be cleared.
I hope this is clear. While I personally would prefer using sessions, I don't want my opinion to be too authoritative at this point and I'm happy to hear counter-opinions from other core-devs and any one else.
comment:58 by , 12 years ago
Here's someone else. It makes much sense not to clutter the urls with something that should be an implementation detail. Rather than doing it as a session, it might also be possible just to do it client-side with localStorage. If I'd hacked a quickfix patch myself, I'd probably do that. Minimal changes. But it wouldn't fix the problem when not using javascript ofc.
I'll take the patch as it is now into a membership management system, hopefully I won't have to do it for next release. The workflow is hampered without it :-)
comment:60 by , 12 years ago
To cite my four years younger self ;)
I proposed to use the session to store the filters so that you don't pass your filters and search queries around whenever you pass the URL of an edit form to someone else. So I would use a hash of the data and use it as a key in the session. This key would then be passed around instead of the info.
So yes, I'm all for using the session, but I think it would be great to still have a querystring parameter that holds a hash/key of the data that it is in the session (i.e. filter settings). This way one could work with multiple browser windows and it would still work as expected.
Great to see this is moving again! Go, Django! :)
comment:61 by , 12 years ago
Hi guys, thanks for reviewing/commenting again... this has definitely to be fixed.
After have read all the comments, i first went with the URL solution. Works well but makes urls a little ugly, true. if this is a problem for most of you, we can go with the session.
The session is another approach that could also work well and i can rewrite my patch and tests to make this happen :)
The only little issue is multi-windows but this could be resolved in a 2nd pass?
I understand and agree with the fact that this session filters is just one-time use and should not affect the direct links to the change list.
Waiting for your approval to kill this one...
comment:62 by , 12 years ago
Hi :)
So here's my session based patch : https://github.com/revolunet/django/compare/bug-6903-2
Its much more simpler than the querystring based approach :
- records the current changelist querystring in an
admin_[APP]_[MODEL]_filters
session variable - if any, redirect with that querystring on add/change/delete object
I added some standard unittests
comments ?
comment:63 by , 12 years ago
Thanks revolunet for updating the patch. I've started reviewing it. I'll try to provide some detailed feedback soon.
by , 12 years ago
Attachment: | 19505.admin-post-save-redirects.diff added |
---|
Accidentally uploaded. Please ignore.
by , 12 years ago
comment:64 by , 12 years ago
Ok, so the review took a bit longer than anticipated as I realized we first needed to clean up the ModelAdmin
API a little in #19505.
This allows a much more succinct and cleaner approach as in the patch above.
Tests are still needed. The tests from the previous patch could probably be reused. However, they could be simplified a bit by testing a model simpler than auth.User
.
As always, feedback is appreciated. Thanks!
comment:65 by , 12 years ago
Cc: | added |
---|
comment:67 by , 12 years ago
I think the latest patch above looks good. Only tests should be added and slightly improved as noted in my previous comment. I'll try to get to that soon if no one beats me up to it.
follow-up: 69 comment:68 by , 12 years ago
hi, will you include this new feature in next 1.5 django release?
comment:69 by , 12 years ago
Replying to gabbork:
hi, will you include this new feature in next 1.5 django release?
1.5 has been in feature-freeze mode since the first release candidate came out some weeks ago. I'm really hoping to have it included in 1.6 though. I'm planning to wrap up the patch this weekend during the worldwide sprint, and then I'll be seeking feedback from users to make sure we get the user experience right.
by , 12 years ago
Attachment: | 6903.2.diff added |
---|
comment:70 by , 12 years ago
Ok, so I have wrapped up the implementation and tests. There is just one minor test failure:
====================================================================== FAIL: test_changelist_view_count_queries (regressiontests.admin_views.tests.AdminCustomQuerysetTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/django/tests/regressiontests/admin_views/tests.py", line 2550, in test_changelist_view_count_queries self.assertEqual(resp.context['selection_note_all'], 'All 2 selected') File "/django/django/test/testcases.py", line 195, in __exit__ executed, self.num AssertionError: 6 != 4 : 6 queries executed, 4 expected
I haven't looked into this yet but it's most likely because the session lookups require extra database queries (which is normal).
comment:71 by , 12 years ago
I'd like to voice my concern regarding this approach. We typically deal with the admin using multiple tabs and with this patch the redirect becomes rather unpredictable.
I'd love a solution for persistent ChangeList
filters since the current behavior forces us to set all our filters repeatedly but at least what is happening is obvious. It's extremely confusing to land on an empty ChangeList
upon saving because in parallel you checked something in a different tab and set a restrictive bunch of filters.
There ought to be a direct connection between the ChangeList
that led to a add/change form and the ChangeList
it redirects to.
Maybe a hidden form field and a querystring parameter for the delete link? Actually semantically it would even make sense that the delete link be changed to a delete form with a submit button.
BTW, if you still plan to go ahead with the current patch, module_name
should be amended to model_name
.
comment:72 by , 12 years ago
Cc: | added |
---|
comment:73 by , 12 years ago
@loic84: I agree with these concerns. I'd like to avoid cluttering the urls so a hidden field sounds like a sane approach.
This means the match needs to be reworked further. The tests should also be extended to simulate a multi-tab use case. If someone can propose a patch that would be great, otherwise I'll try to work on it at Pycon.
comment:74 by , 12 years ago
Wow. There are 12 patches in this page, made by various people, and the desired implementation is still not defined.
This feature may not be a priority, even if more than 5 years old, but would be cool if the core team could agree on the implementation so we can try again and kill that one ;)
Anyone here going at djangocon.eu and interested in fixing that with me ?
comment:75 by , 12 years ago
While I like the idea of a hidden field in the change form, we still need a way of passing the filters from the changelist to the change form when an object is clicked in the changelist. Passing the filters via the querystring would look a bit ugly and would kind of defeat the purpose of using a hidden field.
Ideally, I'd like a way that doesn't clutter the urls and also satisfies the principle of least surprise :)
Sorry I haven't had much time to think all of this through yet. Please don't let this deter you from working on this patch. I'll continue to provide feedback whenever needed. Thanks!
comment:76 by , 12 years ago
@julien: when the user goes from changelist_view
to change_view
you can retrieve the querystring from HTTP_REFERER.
The only time where you might need an ugly URL is for the delete link, but I think we should POST (instead of GET) to the delete_view
anyway, just like we do for the delete action.
by , 12 years ago
Attachment: | 6903.3.diff added |
---|
comment:77 by , 12 years ago
Using the http referer us a good idea. I've just posted a quick proof of concept (I haven't tested this manually yet).
comment:78 by , 12 years ago
The original querystring approach has worked fine for us for years. Are we over thinking the approach on this? I'd really like to see this get in. It's sort of a non starter for a lot of our clients.
comment:79 by , 12 years ago
Cc: | added |
---|---|
Owner: | set to |
Status: | new → assigned |
I've updated older patch by julien. It was missing passing filters to delete view. It is still missing some is_safe_url
checks.
comment:80 by , 12 years ago
I've submitted pull request from previous ticket with some small corrections.
comment:81 by , 12 years ago
Patch needs improvement: | unset |
---|
comment:82 by , 12 years ago
Needs documentation: | unset |
---|
I'm not really sure what kind of documentation this would need: Functional details of admin are not documented. Release notes might be a good place, but should I put it in 1.6?
comment:83 by , 12 years ago
Somehow I missed the updates for that ticket and just saw oinopion's PR on github.
I really like the patch, except that I would have taken this opportunity to turn the delete link into a delete button.
- It's more in line with the HTTP spec where GET should be free of side effects. (We do have a confirmation page, but that's an implementation detail.)
- It's consistant with the
change_list
delete action, and the rest of thechange_view
actions. - It avoids special casing the delete link in this patch.
- Less importantly, it avoids an ugly URL for the delete confirmation page.
That said, this could be changed later in a different patch/ticket but I doubt such a minor cleanup would get much attention on its own.
comment:84 by , 12 years ago
Can we please not bikeshed about delete link? It does not bring us any closer to closing this 5 years old ticket.
comment:85 by , 12 years ago
Patch needs improvement: | set |
---|
I looked further into it, I pass on the delete link as there is no easy way to POST to the delete URL because submit_line.html
is included within a form, and you can't have a form within a form. Fixing it requires a refactor too big to be within the scope of this ticket.
That said, the patch and the tests only cover the basic scenarios:
change_list > add_view > save
change_list > change_view > save
change_list > delete_view > save
You are out of luck if you do:
change_list > change_view (Save and continue editing) > change_view > save
change_list > change_view (Save and add another) > add_view > save
The reason we lose track of filters is that _continue
, _saveasnew
and _addanother
rely on HttpResponseRedirect
so we have to use the querystring for such cases.
At this point I agree with @empty, I'd vote to use querystring everywhere, the URL is not that ugly and if anything, it gives some context of where you came from. If you want to keep the pretty URLs where possible, just append the querystring to these HttpResponseRedirect
and allow _changelist_filters
to come from either POST or GET.
comment:86 by , 12 years ago
I've tried to sum up the different options that have been used so far in this ticket.
The problem:
We need the sticky filters to survive an arbitrarily long path made of GET, POST, and Redirects.
The options:
The only 2 *consistent* ways of doing it are the querystring and the session. There is a third option that involve reading the referrer and storing information in a hidden field, but at least for redirects and html links we need a fallback method.
There is no perfect solution:
- If we pick the querystring, we add a lot of things to the URLs.
- If we pick the session, we lose the direct link between the
change_list
that started the chain of events and the one to which we return. (as explained in comment:71) - If we pick an hybrid solution, we lack consistency and we still suffer from the defects of whatever we use as a fallback.
For the issue at hand I value predictably and reliability much more than the aesthetic of the URL, especially for an admin site. So I'd like to suggest using the querystring and offering an option to switch off the behavior: ModelAdmin.sticky_filters
. The developer gets to pick, he can have prestine URLs or a robust preservation of changelist filters, not both. If we go with that, we'll have to decide what's the default value of the sticky_filters
flag.
Proof of concept: https://github.com/loic/django/compare/ticket6903
I tested it manually and I have yet to fail it, I'll write the docs and tests tomorrow.
Unlike previous attempts at using the querystring, it relies on a templatetag that accounts for existing querystring parameters.
I've also left out the breadcrumbs, as I like that they point to a fresh change_list
, but that's debatable, I can easily add them.
Thoughts?
comment:87 by , 12 years ago
Cc: | added |
---|
I also think the querystring is the more appropriate approach, not sure about the sticky_filters
wording thought.
I'd say leave the breadcrumbs as they are.
comment:88 by , 12 years ago
Four years ago I started using the querystring approach to this problem and it has worked flawlessly. I'm not sure why there's such consternation over the query string parameters in the URL. It's actually quite useful (for sending to a friend) and the exact same behavior you would get with regular filtering of the change list.
I understand the desire to get the "perfect" implementation here but sometimes "great" is the enemy of "good enough". Perhaps we can get some resolution by going forward with the querystring approach. It's not as though anything is lost by implementing this.
comment:89 by , 12 years ago
The querystring approach has the advantages of being explicit and allow to have multi-tabs/windows in different states.
Having the breadcrumbs use the QS is optional but i thing it adds some comfort for the user (let's get back to where i were)
comment:90 by , 12 years ago
Alright, it seems we reached some consensus, hopefully we'll have a RFC patch by today.
I'm not particularly attached to the sticky_filters
wording. At first I went with changelist_filters
as spotted in a different patch, but then I thought that would create confusion with the existing ModelAdmin.list_filter
. If we find a better name, I'll query-replace the whole thing. Note that the user facing string (in the URL) is _changelist_filters
, the underscore is there as a precaution but I guess it's unlikely people would have a model field named "changelist_filters".
For the breadcrumbs, I'm leaving it as it is and I'll see after a couple more people have chipped in, when the patch is complete, I'll do whatever is more popular.
comment:92 by , 12 years ago
PR https://github.com/django/django/pull/1281
I eventually replaced "sticky" by "preserve/preserved".
Edit: Tests are heavily inspired by @revolunet's patch, all credits go to him.
comment:93 by , 12 years ago
comment:95 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:96 by , 11 years ago
Cc: | added |
---|
have your tried it on productional web server? It doesn't work for me at Apache/2.2.22 (Linux/SUSE) mod_ssl/2.2.22 OpenSSL/1.0.1e mod_wsgi/3.3 Python/2.7.3
If I try run the same code on the server on Django development web server (port 8000), filtering is preserved. If I run it on Apache and WSGI, it doesn't work.
This has been requested before, see for example #3777. That was closed by submitter after an alternate method was shown that didn't involve changing Django code. I haven't tried it so don't know if it's still directly applicable to newforms-admin but it might give you an idea of an approach.