Opened 7 years ago

Last modified 15 months ago

#12241 new Bug

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

Reported by: velmont Owned by: batiste
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: spinyol, batiste, 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 6 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 6 years ago.
Preserve query string, with tests
preserve_query_string_with_tests_2.diff (7.7 KB) - added by batiste 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 6 years ago.
Simplification of the tests
preserve_query_string_with_tests_4.diff (6.9 KB) - added by batiste 6 years ago.
Simplification of the tests
preserve_query_string_with_tests_5.diff (6.9 KB) - added by batiste 6 years ago.
Remove the urlparse call and use request.GET.copy instead
preserve_query_string7.patch (11.1 KB) - added by batiste 5 years ago.
Last patch, tests, doc, no parse_qs
preserve_query_string8.patch (10.9 KB) - added by batiste 5 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 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 6 years ago by batiste

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

comment:2 Changed 6 years ago by ramiro

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

Changed 6 years ago by batiste

Preserve query string, with tests

comment:3 Changed 6 years ago by batiste

  • Has patch set
  • Triage Stage changed from Accepted to Ready 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 15 months ago by johnhomes (previous) (diff)

comment:4 Changed 6 years ago by batiste

  • milestone set to 1.2
  • Version changed from 1.1 to SVN

comment:5 Changed 6 years ago by kmtracey

  • milestone 1.2 deleted

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

comment:6 Changed 6 years ago by anonymous

  • milestone set to 1.3

comment:7 Changed 6 years ago by anonymous

  • milestone 1.3 deleted

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

  • 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

Better tests that goes trough all the use case

comment:9 Changed 6 years ago by batiste

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

Simplification of the tests

Changed 6 years ago by batiste

Simplification of the tests

comment:10 Changed 6 years ago by spinyol

  • Cc spinyol added

comment:11 follow-up: Changed 6 years ago by lukeplant

  • Triage Stage changed from Ready for checkin to 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 in reply to: ↑ 11 Changed 6 years ago by batiste

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

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

comment:13 Changed 6 years ago by batiste

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

  • Owner changed from nobody to batiste

comment:15 Changed 6 years ago by batiste

  • Cc batiste added

comment:16 Changed 6 years ago by SmileyChris

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

Thanks Chris. Keep me updated with the progress.

comment:18 Changed 6 years ago by SmileyChris

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

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

comment:20 Changed 6 years ago by lukeplant

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

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 SmileyChris

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

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

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

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

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

Last patch, tests, doc, no parse_qs

Changed 5 years ago by batiste

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

comment:28 Changed 5 years ago by mattmcc

  • Severity set to Normal
  • Type set to Bug

comment:29 Changed 4 years ago by michelts

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