Code

Opened 6 years ago

Closed 10 months ago

Last modified 3 weeks ago

#6903 closed New feature (fixed)

Go back to old change_list view after hitting save

Reported by: jarrow Owned by: oinopion
Component: contrib.admin Version: master
Severity: Normal Keywords: admin
Cc: sixthgear, mremolt, ramusus@…, spinyol, marcoberi@…, joh, hr.bjarni+django@…, rob@…, empty, riccardo.magliocchetti@…, michelts@…, odin.omdal@…, danny.adair@…, revolunet, andybak, kraken, tomek@…, charettes, 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)

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 slonopotamus 6 years ago.
6903-r9781.diff (4.5 KB) - added by ramiro 5 years ago.
Patch updated to r9781
6903-changelist-return-to.diff (8.2 KB) - added by jacob 5 years ago.
A few updates to ramiro's patch, and updated to r9888
6903-r9904.diff (14.7 KB) - added by ramiro 5 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 5 years ago.
Patch fixed for r11616
6903.r12351.current.diff (13.3 KB) - added by ramusus 4 years ago.
Without new changes, just for working with 12351 revision in development trunk
6903.r16617.diff (2.6 KB) - added by graveyboat 3 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 16 months ago.
Accidentally uploaded. Please ignore.
6903.diff (3.1 KB) - added by julien 16 months ago.
6903.2.diff (7.3 KB) - added by julien 14 months ago.
6903.3.diff (10.6 KB) - added by julien 13 months ago.
6903.4.diff (13.0 KB) - added by oinopion 10 months ago.
Updated earlier patch by julien

Download all attachments as: .zip

Change History (111)

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

  • Keywords nfa-someday added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 6 years ago by jarrow

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

  • Description modified (diff)

comment:5 Changed 6 years ago by slonopotamus

  • Has patch set
  • Owner changed from nobody to slonopotamus

Changed 6 years ago by slonopotamus

comment:6 Changed 6 years ago by slonopotamus

  • Owner changed from slonopotamus to nobody

comment:7 Changed 6 years ago by slonopotamus

  • Keywords yandex-sprint added

comment:8 Changed 6 years ago by ericholscher

  • milestone set to post-1.0

comment:9 Changed 5 years ago by niels

  • Cc niels.busch@… added

comment:10 Changed 5 years ago by ramiro

  • Needs tests set
  • Owner changed from nobody to ramiro
  • Patch needs improvement set

Changed 5 years ago by ramiro

Patch updated to r9781

comment:11 Changed 5 years ago by niels

  • Cc niels.busch@… removed

Changed 5 years ago by jacob

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

comment:12 follow-up: Changed 5 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 5 years ago by ramiro

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

comment:13 in reply to: ↑ 12 Changed 5 years ago by ramiro

  • 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 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:15 Changed 5 years ago by sixthgear

  • Cc sixthgear added

comment:16 Changed 5 years ago by ramiro

  • Keywords nfa-someday yandex-sprint removed
  • milestone set to 1.1
  • Version changed from newforms-admin to SVN

comment:17 follow-up: Changed 5 years ago by mawimawi

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 5 years ago by gonz

  • Cc gonz added

comment:19 in reply to: ↑ 17 Changed 5 years ago by ramiro

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

Version 0, edited 5 years ago by ramiro (next)

comment:20 Changed 5 years ago by ramiro

  • milestone changed from 1.1 to 1.1 beta

comment:21 follow-up: Changed 5 years ago by 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.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 5 years ago by mremolt

  • Cc mremolt 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 ; follow-up: Changed 5 years ago by 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.

comment:24 in reply to: ↑ 23 Changed 5 years ago by mremolt

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 5 years ago by jacob

  • milestone changed from 1.1 beta to 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 Changed 5 years ago by margieroginski

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

  • Cc ramusus@… added

comment:28 Changed 5 years ago by spinyol

  • Cc spinyol added

comment:29 Changed 5 years ago by marcob

  • Cc marcoberi@… added

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

Patch fixed for r11616

comment:31 Changed 4 years ago by ramiro

  • Owner ramiro deleted

Changed 4 years ago by ramusus

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

comment:32 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:33 Changed 4 years ago by joh

  • Cc joh added

comment:34 Changed 4 years ago by ramiro

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

comment:35 Changed 4 years ago by hejsan

  • Cc hr.bjarni+django@… added

comment:36 Changed 3 years ago by robhudson

  • Cc rob@… added

comment:37 Changed 3 years ago by empty

  • Cc empty added

comment:38 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:39 Changed 3 years ago by julien

  • UI/UX set

Changed 3 years ago by graveyboat

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

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted

comment:41 Changed 3 years ago by ptone

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

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

comment:43 Changed 2 years ago by rm_

  • Cc riccardo.magliocchetti@… added

comment:44 Changed 2 years ago by michelts

  • Cc michelts@… added

comment:45 Changed 2 years ago by velmont

  • Cc odin.omdal@… added

comment:46 Changed 2 years ago by gonz

  • Cc gonz removed

comment:47 Changed 18 months ago by danny.adair@…

  • Cc danny.adair@… added

comment:48 Changed 17 months ago by revolunet

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 17 months ago by revolunet (previous) (diff)

comment:49 Changed 17 months ago by richard@…

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

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

comment:50 Changed 17 months ago by revolunet

  • Cc revolunet 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 17 months ago by revolunet

  • 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 17 months ago by akaariai

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 17 months ago by revolunet

Adressed the two issues in a new patch improvement.

Last edited 17 months ago by revolunet (previous) (diff)

comment:54 Changed 17 months ago by akaariai

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 17 months ago by revolunet

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 17 months ago by julien

  • 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 follow-up: Changed 17 months ago by hejsan

Why not cookies?

comment:58 Changed 17 months 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 17 months ago by aaugustin

Replying to hejsan:

Why not cookies?

cookies == session

comment:60 Changed 17 months ago by jarrow

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 17 months ago by revolunet

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 17 months ago by revolunet

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 16 months ago by julien

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

Changed 16 months ago by julien

Accidentally uploaded. Please ignore.

Changed 16 months ago by julien

comment:64 Changed 16 months ago by julien

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 16 months ago by andybak

  • Cc andybak added

comment:66 Changed 15 months ago by revolunet

any news on this ?
needs more improvments ?

comment:67 Changed 15 months ago by julien

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 follow-up: Changed 14 months ago by gabbork

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

comment:69 in reply to: ↑ 68 Changed 14 months ago by julien

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 14 months ago by julien

comment:70 Changed 14 months ago by julien

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 14 months 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 14 months ago by kraken

  • Cc kraken added

comment:73 Changed 14 months ago by julien

@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 14 months ago by revolunet

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 ?

Last edited 14 months ago by revolunet (previous) (diff)

comment:75 Changed 13 months ago by julien

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 13 months 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 13 months ago by julien

comment:77 Changed 13 months ago by julien

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 13 months 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 10 months ago by oinopion

Updated earlier patch by julien

comment:79 Changed 10 months ago by oinopion

  • Cc tomek@… added
  • Owner set to oinopion
  • Status changed from new to 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 Changed 10 months ago by oinopion

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

comment:81 Changed 10 months ago by oinopion

  • Patch needs improvement unset

comment:82 Changed 10 months ago by oinopion

  • 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 10 months 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 10 months 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 10 months 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 10 months 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 10 months ago by charettes

  • Cc charettes 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 10 months 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 10 months ago by revolunet

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 10 months 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 10 months ago by charettes

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

comment:92 Changed 10 months 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 10 months ago by loic84 (previous) (diff)

comment:94 Changed 10 months ago by revolunet

woot ! thanks guys

comment:95 Changed 10 months ago by timo

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:96 Changed 5 months 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 weeks 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 weeks 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 weeks 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.