Opened 9 years ago

Closed 3 years ago

Last modified 3 years ago

#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: master
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 Ramiro Morales)

When editing something via the admin I could observe a recurring usage pattern on myself:

  1. 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
  2. Edit
  3. Hit save
  4. Hit the back button two times to get my former list
  5. 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)

return_to_old_change_list.diff (4.8 KB) - added by Marat Radchenko 8 years ago.
6903-r9781.diff (4.5 KB) - added by Ramiro Morales 8 years ago.
Patch updated to r9781
6903-changelist-return-to.diff (8.2 KB) - added by Jacob 8 years ago.
A few updates to ramiro's patch, and updated to r9888
6903-r9904.diff (14.7 KB) - added by Ramiro Morales 8 years ago.
New patch, including Jacob's changes and suggestions. Thanks Alex Gaynor for his help with the tests
6903-r11616.diff (14.6 KB) - added by marcob 7 years ago.
Patch fixed for r11616
6903.r12351.current.diff (13.3 KB) - added by ramusus 7 years ago.
Without new changes, just for working with 12351 revision in development trunk
6903.r16617.diff (2.6 KB) - added by graveyboat 5 years ago.
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.
19505.admin-post-save-redirects.diff (12 bytes) - added by Julien Phalip 4 years ago.
Accidentally uploaded. Please ignore.
6903.diff (3.1 KB) - added by Julien Phalip 4 years ago.
6903.2.diff (7.3 KB) - added by Julien Phalip 4 years ago.
6903.3.diff (10.6 KB) - added by Julien Phalip 4 years ago.
6903.4.diff (13.0 KB) - added by Tomek Paczkowski 3 years ago.
Updated earlier patch by julien

Download all attachments as: .zip

Change History (111)

comment:1 Changed 9 years ago by Karen Tracey <kmtracey@…>

Keywords: nfa-someday added
Triage Stage: UnreviewedDesign decision needed

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.

comment:2 Changed 9 years ago by

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

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 Changed 8 years ago by Ramiro Morales

Description: modified (diff)

comment:5 Changed 8 years ago by Marat Radchenko

Has patch: set
Owner: changed from nobody to Marat Radchenko

Changed 8 years ago by Marat Radchenko

comment:6 Changed 8 years ago by Marat Radchenko

Owner: changed from Marat Radchenko to nobody

comment:7 Changed 8 years ago by Marat Radchenko

Keywords: yandex-sprint added

comment:8 Changed 8 years ago by Eric Holscher

milestone: post-1.0

comment:9 Changed 8 years ago by Niels Sandholt Busch

Cc: niels.busch@… added

comment:10 Changed 8 years ago by Ramiro Morales

Needs tests: set
Owner: changed from nobody to Ramiro Morales
Patch needs improvement: set

Changed 8 years ago by Ramiro Morales

Attachment: 6903-r9781.diff added

Patch updated to r9781

comment:11 Changed 8 years ago by Niels Sandholt Busch

Cc: niels.busch@… removed

Changed 8 years ago by Jacob

A few updates to ramiro's patch, and updated to r9888

comment:12 Changed 8 years ago by Jacob

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.

Changed 8 years ago by Ramiro Morales

Attachment: 6903-r9904.diff added

New patch, including Jacob's changes and suggestions. Thanks Alex Gaynor for his help with the tests

comment:13 in reply to:  12 Changed 8 years ago by Ramiro Morales

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 a GET 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:14 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:15 Changed 8 years ago by Joshua Cender

Cc: Joshua Cender added

comment:16 Changed 8 years ago by Ramiro Morales

Keywords: nfa-someday yandex-sprint removed
milestone: 1.1
Version: newforms-adminSVN

comment:17 Changed 8 years ago by Martin Winkler

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 Changed 8 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added

comment:19 in reply to:  17 Changed 8 years ago by Ramiro Morales

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.

Last edited 4 years ago by Ramiro Morales (previous) (diff)

comment:20 Changed 8 years ago by Ramiro Morales

milestone: 1.11.1 beta

comment:21 Changed 8 years ago by

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.

comment:22 in reply to:  21 ; Changed 8 years ago by Marc Remolt

Cc: Marc Remolt 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.

comment:23 in reply to:  22 ; Changed 8 years ago by

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 in reply to:  23 Changed 8 years ago by Marc Remolt

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

milestone: 1.1 beta1.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 Changed 8 years ago by Margie Roginski

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

Cc: ramusus@… added

comment:28 Changed 7 years ago by Salva Pinyol

Cc: Salva Pinyol added

comment:29 Changed 7 years ago by marcob

Cc: marcoberi@… added

comment:30 Changed 7 years ago by marcob

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

Changed 7 years ago by marcob

Attachment: 6903-r11616.diff added

Patch fixed for r11616

comment:31 Changed 7 years ago by Ramiro Morales

Owner: Ramiro Morales deleted

Changed 7 years ago by ramusus

Attachment: 6903.r12351.current.diff added

Without new changes, just for working with 12351 revision in development trunk

comment:32 Changed 7 years ago by James Bennett

milestone: 1.2

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

comment:33 Changed 7 years ago by Johannes Bornhold

Cc: Johannes Bornhold added

comment:34 Changed 7 years ago by Ramiro Morales

See also #12241 that also proposes preserving the query string in another admin use case.

comment:35 Changed 6 years ago by hejsan

Cc: hr.bjarni+django@… added

comment:36 Changed 6 years ago by Rob Hudson

Cc: rob@… added

comment:37 Changed 6 years ago by empty

Cc: empty added

comment:38 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: New feature

comment:39 Changed 5 years ago by Julien Phalip

UI/UX: set

Changed 5 years ago by graveyboat

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 Changed 5 years ago by Alex Gaynor

Easy pickings: unset
Triage Stage: Design decision neededAccepted

comment:41 Changed 5 years ago by Preston Holmes

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

#3777 was closed as a duplicate. It contains extensive discussions on this topic.

comment:43 Changed 5 years ago by rm_

Cc: riccardo.magliocchetti@… added

comment:44 Changed 5 years ago by Michel Sabchuk

Cc: michelts@… added

comment:45 Changed 5 years ago by Odin Hørthe Omdal

Cc: odin.omdal@… added

comment:46 Changed 5 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra removed

comment:47 Changed 4 years ago by danny.adair@…

Cc: danny.adair@… added

comment:48 Changed 4 years ago by Julien Bouquillon

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

Last edited 4 years ago by Julien Bouquillon (previous) (diff)

comment:49 Changed 4 years ago by richard@…

An alternative approach for saving and restoring the filters after editing a model:

http://djangosnippets.org/snippets/2849/

comment:50 Changed 4 years ago by Julien Bouquillon

Cc: Julien Bouquillon 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 Changed 4 years ago by Julien Bouquillon

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 Changed 4 years ago by Anssi Kääriäinen

Two comments:

  1. 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
  2. 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:53 Changed 4 years ago by Julien Bouquillon

Adressed the two issues in a new patch improvement.

Last edited 4 years ago by Julien Bouquillon (previous) (diff)

comment:54 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Julien Bouquillon

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

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:57 Changed 4 years ago by hejsan

Why not cookies?

comment:58 Changed 4 years ago by Velmont

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:59 in reply to:  57 Changed 4 years ago by Aymeric Augustin

Replying to hejsan:

Why not cookies?

cookies == session

comment:60 Changed 4 years ago by

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 Changed 4 years ago by Julien Bouquillon

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 Changed 4 years ago by Julien Bouquillon

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

Thanks revolunet for updating the patch. I've started reviewing it. I'll try to provide some detailed feedback soon.

Changed 4 years ago by Julien Phalip

Accidentally uploaded. Please ignore.

Changed 4 years ago by Julien Phalip

Attachment: 6903.diff added

comment:64 Changed 4 years ago by Julien Phalip

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 Changed 4 years ago by Andy Baker

Cc: Andy Baker added

comment:66 Changed 4 years ago by Julien Bouquillon

any news on this ?
needs more improvments ?

comment:67 Changed 4 years ago by Julien Phalip

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.

comment:68 Changed 4 years ago by Gabriele Giaccari

hi, will you include this new feature in next 1.5 django release?

comment:69 in reply to:  68 Changed 4 years ago by Julien Phalip

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.

Changed 4 years ago by Julien Phalip

Attachment: 6903.2.diff added

comment:70 Changed 4 years ago by Julien Phalip

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 Changed 4 years ago by loic84

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 Changed 4 years ago by kraken

Cc: kraken added

comment:73 Changed 4 years ago by Julien Phalip

@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 Changed 4 years ago by Julien Bouquillon

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 3 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 ?

Version 0, edited 4 years ago by Julien Bouquillon (next)

comment:75 Changed 4 years ago by Julien Phalip

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 Changed 4 years ago by loic84

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

Changed 4 years ago by Julien Phalip

Attachment: 6903.3.diff added

comment:77 Changed 4 years ago by Julien Phalip

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 Changed 4 years ago by empty

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.

Changed 3 years ago by Tomek Paczkowski

Attachment: 6903.4.diff added

Updated earlier patch by julien

comment:79 Changed 3 years ago by Tomek Paczkowski

Cc: tomek@… added
Owner: set to Tomek Paczkowski
Status: newassigned

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 Changed 3 years ago by Tomek Paczkowski

I've submitted pull request from previous ticket with some small corrections.

comment:81 Changed 3 years ago by Tomek Paczkowski

Patch needs improvement: unset

comment:82 Changed 3 years ago by Tomek Paczkowski

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 Changed 3 years ago by loic84

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.

  1. 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.)
  2. It's consistant with the change_list delete action, and the rest of the change_view actions.
  3. It avoids special casing the delete link in this patch.
  4. 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 Changed 3 years ago by anonymous

Can we please not bikeshed about delete link? It does not bring us any closer to closing this 5 years old ticket.

comment:85 Changed 3 years ago by loic84

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 Changed 3 years ago by loic84

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 Changed 3 years ago by Simon Charette

Cc: Simon Charette 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 Changed 3 years ago by empty

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 Changed 3 years ago by Julien Bouquillon

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 Changed 3 years ago by loic84

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:91 Changed 3 years ago by Simon Charette

@loic84 FWIW model fields can't start with an underscore anyway.

comment:92 Changed 3 years ago by loic84

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.

Last edited 3 years ago by loic84 (previous) (diff)

comment:93 Changed 3 years ago by Aymeric Augustin

Last edited 3 years ago by Aymeric Augustin (previous) (diff)

comment:94 Changed 3 years ago by Julien Bouquillon

woot ! thanks guys

comment:95 Changed 3 years ago by Tim Graham

Resolution: fixed
Status: assignedclosed

comment:96 Changed 3 years ago by honyczek

Cc: honyczek@… 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.

comment:97 Changed 3 years ago by Tim Graham <timograham@…>

In 4339e9a92d98371a50a6fe54ab7ea0c602ecee28:

Fixed #21795 -- Made add_preserved_filters account for url prefixes.

Thanks to trac username honyczek for the report. Refs #6903.

comment:99 Changed 3 years ago by Tim Graham <timograham@…>

In a5297c1ef4c4c010d1eb979177d4633beb5f9cad:

[1.6.x] Fixed #21795 -- Made add_preserved_filters account for url prefixes.

Thanks to trac username honyczek for the report. Refs #6903.

Backport of 4339e9a92d from master

comment:98 Changed 3 years ago by Tim Graham <timograham@…>

In 5268d71f18d12c362d74010210309c1cec8e8a1a:

[1.7.x] Fixed #21795 -- Made add_preserved_filters account for url prefixes.

Thanks to trac username honyczek for the report. Refs #6903.

Backport of 4339e9a92d from master

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