#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)
Change History (52)
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 15 years ago
Attachment: | keep_query_string_after_continue.diff added |
---|
comment:2 by , 15 years ago
See also #6903 that also proposes preserving the query string in another admin use case.
by , 15 years ago
Attachment: | preserve_query_string_with_tests.diff added |
---|
Preserve query string, with tests
comment:3 by , 15 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:4 by , 15 years ago
milestone: | → 1.2 |
---|---|
Version: | 1.1 → SVN |
comment:5 by , 15 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 , 14 years ago
milestone: | → 1.3 |
---|
comment:7 by , 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 , 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 , 14 years ago
Attachment: | preserve_query_string_with_tests_2.diff added |
---|
Better tests that goes trough all the use case
comment:9 by , 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 , 14 years ago
Attachment: | preserve_query_string_with_tests_3.diff added |
---|
Simplification of the tests
by , 14 years ago
Attachment: | preserve_query_string_with_tests_4.diff added |
---|
Simplification of the tests
comment:10 by , 14 years ago
Cc: | added |
---|
follow-up: 12 comment:11 by , 14 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
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 by , 14 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 , 14 years ago
Attachment: | preserve_query_string_with_tests_5.diff added |
---|
Remove the urlparse call and use request.GET.copy instead
comment:13 by , 14 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 , 14 years ago
Owner: | changed from | to
---|
comment:15 by , 14 years ago
Cc: | added |
---|
comment:16 by , 14 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:18 by , 14 years ago
Sorry for the lack of progress. Here's the branch, anyway: http://github.com/SmileyChris/django/compare/master...12241-preserve-querystring
comment:20 by , 14 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 , 14 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 , 14 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 , 14 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 , 14 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 , 14 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 , 14 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 , 14 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 , 14 years ago
Attachment: | preserve_query_string7.patch added |
---|
Last patch, tests, doc, no parse_qs
by , 14 years ago
Attachment: | preserve_query_string8.patch added |
---|
Same as last one, but removing adding the proxy thing in add_view
comment:28 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:29 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:30 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:31 by , 2 years 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.
comment:32 by , 2 years ago
Patch needs improvement: | set |
---|
comment:33 by , 2 years 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 , 2 years ago
Patch needs improvement: | set |
---|
comment:35 by , 22 months ago
Needs tests: | set |
---|
comment:36 by , 15 months ago
Owner: | changed from | to
---|
comment:37 by , 15 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:38 by , 14 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 , 14 months ago
Needs tests: | set |
---|
comment:40 by , 13 months ago
Needs tests: | unset |
---|
comment:41 by , 13 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:44 by , 13 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! :)))
Keep the request's query string when the button "Save and continue" or "Add and continue" are used.