Opened 7 years ago

Last modified 19 months ago

#12241 new Bug

Admin forgets URL used for prefilling forms when hitting Save and add another

Reported by: Odin Hørthe Omdal Owned by: Batiste Bieler
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Salva Pinyol, Batiste Bieler, michelts@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When I go to /admin/framlegg/patch/add/?document=5 everything works fine, the 5 is prefilled. However, when I hit «save and add another» the GET request variable isn't in the URL any more.

Attachments (8)

keep_query_string_after_continue.diff (2.3 KB) - added by Batiste Bieler 7 years ago.
Keep the request's query string when the button "Save and continue" or "Add and continue" are used.
preserve_query_string_with_tests.diff (4.1 KB) - added by Batiste Bieler 7 years ago.
Preserve query string, with tests
preserve_query_string_with_tests_2.diff (7.7 KB) - added by Batiste Bieler 6 years ago.
Better tests that goes trough all the use case
preserve_query_string_with_tests_3.diff (7.0 KB) - added by Batiste Bieler 6 years ago.
Simplification of the tests
preserve_query_string_with_tests_4.diff (6.9 KB) - added by Batiste Bieler 6 years ago.
Simplification of the tests
preserve_query_string_with_tests_5.diff (6.9 KB) - added by Batiste Bieler 6 years ago.
Remove the urlparse call and use request.GET.copy instead
preserve_query_string7.patch (11.1 KB) - added by Batiste Bieler 6 years ago.
Last patch, tests, doc, no parse_qs
preserve_query_string8.patch (10.9 KB) - added by Batiste Bieler 6 years ago.
Same as last one, but removing adding the proxy thing in add_view

Download all attachments as: .zip

Change History (37)

comment:1 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

Changed 7 years ago by Batiste Bieler

Keep the request's query string when the button "Save and continue" or "Add and continue" are used.

comment:2 Changed 7 years ago by Ramiro Morales

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

Changed 7 years ago by Batiste Bieler

Preserve query string, with tests

comment:3 Changed 7 years ago by Batiste Bieler

Has patch: set
Triage Stage: AcceptedReady for checkin

The bug #6903 is quite a different beast. This one just keep the query string when you push the "Save and continue" button in the admin. As simple as that. No session needed.

Last edited 19 months ago by johnhomes (previous) (diff)

comment:4 Changed 7 years ago by Batiste Bieler

milestone: 1.2
Version: 1.1SVN

comment:5 Changed 7 years ago by Karen Tracey

milestone: 1.2

Sorry, but 1.2 milestone is only for release-critical bugs at this point and this does not qualify.

comment:6 Changed 7 years ago by anonymous

milestone: 1.3

comment:7 Changed 7 years ago by anonymous

milestone: 1.3

Reading my last patch, I spotted what could be a small error:

?_popup=1" + request.META.get('QUERY_STRING')

Should be

?_popup=1&" + request.META.get('QUERY_STRING')

comment:8 Changed 7 years ago by Chris Beaven

Needs tests: set
Patch needs improvement: set

This isn't the neatest code:
?_popup=1" + request.META.get('QUERY_STRING')

I'd use request.GET.copy(), add _popup if necessary, and then only append the querystring if necessary.

And you should write a test for the case where there's a querystring already so that we're not talking about whether it "could be" an error ;)

Changed 6 years ago by Batiste Bieler

Better tests that goes trough all the use case

comment:9 Changed 6 years ago by Batiste Bieler

There you go. The tests are more complete. I used the urllib and urlencode modules. Is that ok to use them within Django?

Cheers,
Batiste

Changed 6 years ago by Batiste Bieler

Simplification of the tests

Changed 6 years ago by Batiste Bieler

Simplification of the tests

comment:10 Changed 6 years ago by Salva Pinyol

Cc: Salva Pinyol added

comment:11 Changed 6 years ago by Luke Plant

Triage Stage: Ready for checkinAccepted

Changed back to 'Accepted' to reflect SmileyChris's criticisms. There doesn't seem to be any need to parse META['QUERY_STRING'] when the request object has done that already.

comment:12 in reply to:  11 Changed 6 years ago by Batiste Bieler

Replying to lukeplant:

Changed back to 'Accepted' to reflect SmileyChris's criticisms. There doesn't seem to be any need to parse META['QUERY_STRING'] when the request object has done that already.

I am not sure if you are satisfied by the state of the patch? Do I need to make some more change and not parse METAQUERY_STRING?? I would really like to see this issue fixed and I am ready to improve the patch if needed.

Changed 6 years ago by Batiste Bieler

Remove the urlparse call and use request.GET.copy instead

comment:13 Changed 6 years ago by Batiste Bieler

The latest patch (preserve_query_string_with_tests_5.diff) removed the urlparse and use request.GET.copy() instead. The tests still pass. To test the 2 tests:

python runtests.py admin_views.AdminResponseChangeTest --settings test_sqlite

I am awaiting review of the new patch.

comment:14 Changed 6 years ago by Batiste Bieler

Owner: changed from nobody to Batiste Bieler

comment:15 Changed 6 years ago by Batiste Bieler

Cc: Batiste Bieler added

comment:16 Changed 6 years ago by Chris Beaven

batiste, I've got a local branch with some more changes than your patch which I started yesterday. I'll get it pushed somewhere for you to look at, or at least attach it as a patch here.

There's an issue actually with the current logic of _ispopup which I want to address.

comment:17 Changed 6 years ago by Batiste Bieler

Thanks Chris. Keep me updated with the progress.

comment:18 Changed 6 years ago by Chris Beaven

Sorry for the lack of progress. Here's the branch, anyway: http://github.com/SmileyChris/django/compare/master...12241-preserve-querystring

comment:19 Changed 6 years ago by Batiste Bieler

The commit looks good to me. Did you commit in the trunk already?

comment:20 Changed 6 years ago by Luke Plant

Looks good to me. There seems to be a stray comment in one of the tests — "Technically, only the response_change" — that I can't make sense of.

comment:21 Changed 6 years ago by anonymous

This looks like an unfinished comment. The test being exactly the same as the one above, I think it should be removed.

comment:22 Changed 6 years ago by Batiste Bieler

Last comment was from me. Any news on this issue? I would really like to see it in 1.3.

comment:23 Changed 6 years ago by Chris Beaven

It was me committing in the middle of a thought, then getting distracted and moving on to other stuff :P

The test looks the same, but the behaviour of the _saveasnew forces a response_add instead of a response_change from memory. I wanted to look at it a bit more, but still haven't got around to it yet.

comment:24 Changed 6 years ago by Batiste Bieler

Isn't it there that you wanted to add the query string?:

http://github.com/SmileyChris/django/blob/master/django/contrib/admin/options.py#L873

What can I do to help to push this on 1.3?

comment:25 Changed 6 years ago by Julien Phalip

Note that urlparse.parse_qs() was introduced in Python 2.6 [1], so the patch needs to be modified to work with Python >= 2.4.

[1] http://docs.python.org/library/urlparse.html#urlparse.parse_qs

comment:26 Changed 6 years ago by Julien Phalip

Needs tests: unset

#10824 was closed as a dupe. Note that Malcolm and Chris had triaged the ticket differently than this one (i.e. considered as a new feature with DDN).

comment:27 Changed 6 years ago by Batiste Bieler

Merged latest Django with chris changes, fix tests, added documentation on the test that was missing, removed parse_qs

https://github.com/batiste/django/commit/af3285ca12604ddde1b7f2af65fed1f8634e340c
https://github.com/batiste/django/commit/90708a9b162d7c6c2f48afe8a32a7e5e42b91e54

What else can I do?

Changed 6 years ago by Batiste Bieler

Last patch, tests, doc, no parse_qs

Changed 6 years ago by Batiste Bieler

Same as last one, but removing adding the proxy thing in add_view

comment:28 Changed 6 years ago by Matt McClanahan

Severity: Normal
Type: Bug

comment:29 Changed 5 years ago by Michel Sabchuk

Cc: michelts@… added
Easy pickings: unset
UI/UX: unset
Note: See TracTickets for help on using tickets.
Back to Top