Opened 14 years ago

Closed 5 months ago

Last modified 5 months ago

#12241 closed Bug (fixed)

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

Reported by: Odin Hørthe Omdal Owned by: Mariana
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Salva Pinyol, Batiste Bieler, michelts@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 14 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 14 years ago.
Preserve query string, with tests
preserve_query_string_with_tests_2.diff (7.7 KB ) - added by Batiste Bieler 14 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 14 years ago.
Simplification of the tests
preserve_query_string_with_tests_4.diff (6.9 KB ) - added by Batiste Bieler 14 years ago.
Simplification of the tests
preserve_query_string_with_tests_5.diff (6.9 KB ) - added by Batiste Bieler 13 years ago.
Remove the urlparse call and use request.GET.copy instead
preserve_query_string7.patch (11.1 KB ) - added by Batiste Bieler 13 years ago.
Last patch, tests, doc, no parse_qs
preserve_query_string8.patch (10.9 KB ) - added by Batiste Bieler 13 years ago.
Same as last one, but removing adding the proxy thing in add_view

Download all attachments as: .zip

Change History (52)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

by Batiste Bieler, 14 years ago

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

comment:2 by Ramiro Morales, 14 years ago

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

by Batiste Bieler, 14 years ago

Preserve query string, with tests

comment:3 by Batiste Bieler, 14 years ago

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 9 years ago by johnhomes (previous) (diff)

comment:4 by Batiste Bieler, 14 years ago

milestone: 1.2
Version: 1.1SVN

comment:5 by Karen Tracey, 14 years ago

milestone: 1.2

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

comment:6 by anonymous, 14 years ago

milestone: 1.3

comment:7 by anonymous, 14 years ago

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 by Chris Beaven, 14 years ago

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

by Batiste Bieler, 14 years ago

Better tests that goes trough all the use case

comment:9 by Batiste Bieler, 14 years ago

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

by Batiste Bieler, 14 years ago

Simplification of the tests

by Batiste Bieler, 14 years ago

Simplification of the tests

comment:10 by Salva Pinyol, 14 years ago

Cc: Salva Pinyol added

comment:11 by Luke Plant, 14 years ago

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.

in reply to:  11 comment:12 by Batiste Bieler, 13 years ago

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.

by Batiste Bieler, 13 years ago

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

comment:13 by Batiste Bieler, 13 years ago

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 by Batiste Bieler, 13 years ago

Owner: changed from nobody to Batiste Bieler

comment:15 by Batiste Bieler, 13 years ago

Cc: Batiste Bieler added

comment:16 by Chris Beaven, 13 years ago

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 by Batiste Bieler, 13 years ago

Thanks Chris. Keep me updated with the progress.

comment:18 by Chris Beaven, 13 years ago

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

comment:19 by Batiste Bieler, 13 years ago

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

comment:20 by Luke Plant, 13 years ago

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 by anonymous, 13 years ago

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

comment:22 by Batiste Bieler, 13 years ago

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

comment:23 by Chris Beaven, 13 years ago

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 by Batiste Bieler, 13 years ago

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 by Julien Phalip, 13 years ago

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 by Julien Phalip, 13 years ago

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 by Batiste Bieler, 13 years ago

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?

by Batiste Bieler, 13 years ago

Last patch, tests, doc, no parse_qs

by Batiste Bieler, 13 years ago

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

comment:28 by Matt McClanahan, 13 years ago

Severity: Normal
Type: Bug

comment:29 by Michel Sabchuk, 12 years ago

Cc: michelts@… added
Easy pickings: unset
UI/UX: unset

comment:30 by matthewn, 17 months ago

Owner: changed from Batiste Bieler to matthewn
Status: newassigned

comment:31 by matthewn, 17 months ago

Patch needs improvement: unset

Greetings from DjangoCon US! First time contributor here with a pull request for this: https://github.com/django/django/pull/16206

Note: Earlier discussion in this ticket considered altering the behavior for 'save and continue' as well, but I am not sure that actually makes sense from a use case perspective. My proposed fix is more targeted, addressing only the 'save and add another' issue mentioned in the ticket title and description.

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

comment:32 by Mariusz Felisiak, 17 months ago

Patch needs improvement: set

comment:33 by matthewn, 17 months ago

Patch needs improvement: unset

As requested in the PR, I've expanded my fix to cover save-and-add and save-and-continue on both add and change forms. Four tests cover the four cases. https://github.com/django/django/pull/16206

comment:34 by Mariusz Felisiak, 17 months ago

Patch needs improvement: set

comment:35 by Mariusz Felisiak, 14 months ago

Needs tests: set

comment:36 by Mariana, 7 months ago

Owner: changed from matthewn to Mariana

comment:37 by Mariana, 7 months ago

Needs tests: unset
Patch needs improvement: unset
Last edited 7 months ago by Mariusz Felisiak (previous) (diff)

comment:38 by Lily Foote, 6 months ago

The patch looks good to me, but would probably benefit from a review from someone more familiar with the admin. There's also some code we think is dead that we're not sure whether to delete as part of this ticket or as a separate PR.

comment:39 by Mariusz Felisiak, 6 months ago

Needs tests: set

comment:40 by Mariana, 6 months ago

Needs tests: unset

comment:41 by Mariusz Felisiak, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:42 by Mariusz Felisiak <felisiak.mariusz@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In fc62e177:

Fixed #12241 -- Preserved query strings when using "Save and continue/add another" in admin.

Co-authored-by: Grady Yu <gradyy@…>
Co-authored-by: David Sanders <shang.xiao.sanders@…>
Co-authored-by: Matthew Newton <matthewn@…>

comment:43 by Mariusz Felisiak <felisiak.mariusz@…>, 5 months ago

In 0bbe6ca2:

[5.0.x] Fixed #12241 -- Preserved query strings when using "Save and continue/add another" in admin.

Co-authored-by: Grady Yu <gradyy@…>
Co-authored-by: David Sanders <shang.xiao.sanders@…>
Co-authored-by: Matthew Newton <matthewn@…>

Backport of fc62e17778dad9eab9e507d90d85a33d415f64a7 from main

comment:44 by Odin Hørthe-Omdal Urdland, 5 months ago

This app I was working on when filing this bug 14 years ago is not used anymore. But I have a more recent one made 12 years ago which is still in production and could benefit from this in the next release. ;)

Thank you all! :)))

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