Code

Opened 4 years ago

Last modified 2 years 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 4 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 4 years ago.
Preserve query string, with tests
preserve_query_string_with_tests_2.diff (7.7 KB) - added by batiste 4 years ago.
Better tests that goes trough all the use case
preserve_query_string_with_tests_3.diff (7.0 KB) - added by batiste 4 years ago.
Simplification of the tests
preserve_query_string_with_tests_4.diff (6.9 KB) - added by batiste 4 years ago.
Simplification of the tests
preserve_query_string_with_tests_5.diff (6.9 KB) - added by batiste 4 years ago.
Remove the urlparse call and use request.GET.copy instead
preserve_query_string7.patch (11.1 KB) - added by batiste 3 years ago.
Last patch, tests, doc, no parse_qs
preserve_query_string8.patch (10.9 KB) - added by batiste 3 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 4 years ago by russellm

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

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

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

Changed 4 years ago by batiste

Preserve query string, with tests

comment:3 Changed 4 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.

comment:4 Changed 4 years ago by batiste

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

comment:5 Changed 4 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 4 years ago by anonymous

  • milestone set to 1.3

comment:7 Changed 4 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 4 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 4 years ago by batiste

Better tests that goes trough all the use case

comment:9 Changed 4 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 4 years ago by batiste

Simplification of the tests

Changed 4 years ago by batiste

Simplification of the tests

comment:10 Changed 4 years ago by spinyol

  • Cc spinyol added

comment:11 follow-up: Changed 4 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 4 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 4 years ago by batiste

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

comment:13 Changed 4 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 4 years ago by batiste

  • Owner changed from nobody to batiste

comment:15 Changed 4 years ago by batiste

  • Cc batiste added

comment:16 Changed 4 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 4 years ago by batiste

Thanks Chris. Keep me updated with the progress.

comment:18 Changed 4 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 4 years ago by batiste

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

comment:20 Changed 4 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 4 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 3 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 3 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 3 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 3 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 3 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 3 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 3 years ago by batiste

Last patch, tests, doc, no parse_qs

Changed 3 years ago by batiste

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

comment:28 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to Bug

comment:29 Changed 2 years ago by michelts

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from batiste to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.