Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33893 closed Bug (fixed)

Admin add model page incorrectly redirects

Reported by: Fabian Jarrett Owned by: Fabian Jarrett
Component: contrib.admin Version: 4.1
Severity: Release blocker Keywords: admin
Cc: Carlton Gibson, Marcelo Galigniana 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

In the admin when clicking "add model +" I fill out the fields to create my item then when I click "Save and add another" or "Save and continue editing" it redirects back to the model list view even though the item was added successfully. If I click back on to the item to edit it the buttons work how they should.

Looks like "_addanother" isn't in the request.POST: https://github.com/django/django/blob/main/django/contrib/admin/options.py#L1388

The name on the input isn't being passed with the form submit: https://github.com/django/django/blob/main/django/contrib/admin/templates/admin/submit_line.html#L6

If I comment out overriding the submit it works as expected: https://github.com/django/django/blob/main/django/contrib/admin/static/admin/js/change_form.js#L11:L19

Also when editing an item that has already been created "modelName" is undefined so the submit buttons don't get overridden: https://github.com/django/django/blob/main/django/contrib/admin/static/admin/js/change_form.js#L8
So works when editing, but not when adding a new item.

I think this change is causing the issue? https://github.com/django/django/commit/fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa

Change History (22)

comment:1 by Fabian Jarrett, 2 years ago

Owner: changed from nobody to Fabian Jarrett
Status: newassigned

comment:2 by Mariusz Felisiak, 2 years ago

Cc: Carlton Gibson Marcelo Galigniana added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the report! Regression in fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa.

comment:3 by Claude Paroz, 2 years ago

Maybe we should simply revert the regression commit, as it seems the remedy is worse than the disease.

comment:4 by Carlton Gibson, 2 years ago

HTMLFormElement.submit() (MDN):

<input> with attribute type="submit" will not be submitted with the form when using HTMLFormElement.submit(), but it would be submitted when you do it with original HTML form submit.

So I guess we'd need to add a hidden element for the button clicked... gah... — leaning too overly complex (as per Claude's take.)

Marcelo do you have a thought here?

comment:5 by Marcelo Galigniana, 2 years ago

I can add the hidden input, but I'm agree with Claude: It would add complexity+.

I am wondering if there is another strategy like we did in https://github.com/django/django/pull/15229 (disable buttons, but it was no good for accessibility).

Anyway, when I was working on this ticket, if I'm not wrong, after do a double submission the user will see two notifications (the green toasts), right? So it could handle the 'bug' deleting the second copy.

Last edited 2 years ago by Marcelo Galigniana (previous) (diff)

comment:6 by Mariusz Felisiak, 2 years ago

Agreed, let's revert it.

comment:7 by Mariusz Felisiak, 2 years ago

Fabian, Would you like to prepare a patch? You can use git revert fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa and add release notes to the 4.1.1.txt.

in reply to:  7 comment:8 by Fabian Jarrett, 2 years ago

Replying to Mariusz Felisiak:

Fabian, Would you like to prepare a patch? You can use git revert fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa and add release notes to the 4.1.1.txt.

Yea, I'll look into it.

comment:9 by Fabian Jarrett, 2 years ago

Version 1, edited 2 years ago by Fabian Jarrett (previous) (next) (diff)

comment:10 by Fabian Jarrett, 2 years ago

I messed up a few PRs. Hopefully this one is all good!

comment:11 by Mariusz Felisiak, 2 years ago

Has patch: set
Needs tests: set

comment:12 by Mariusz Felisiak, 2 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 0756c61f:

Fixed #33893 -- Reverted "Fixed #28889 -- Prevented double submission of admin forms."

Regression in fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa.

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 82e9e19:

[4.1.x] Fixed #33893 -- Reverted "Fixed #28889 -- Prevented double submission of admin forms."

Regression in fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa.

Backport of 0756c61f2ada56e4ae625589099c0141a77737eb from main

comment:15 by Mariusz Felisiak, 2 years ago

#33904 was a related issue for many-to-many fields with filters.

comment:16 by ADHD is DEV, 2 years ago

I don't get it, so was it "fixed" then or not? I still have this problem and my ticket was closed bc it's a "duplicate" of this one.

comment:17 by Tim Graham, 2 years ago

The fix will be released in Django 4.1.1.

comment:18 by Emanuel Angelo, 2 years ago

I've been testing version 4.1.1 and it still has the same problem.

comment:19 by Claude Paroz, 2 years ago

Did you try after emptying the browser cache?

in reply to:  19 comment:20 by Emanuel Angelo, 2 years ago

Replying to Claude Paroz:

Did you try after emptying the browser cache?

I did not... and it worked flawlessly after I flushed it. I'm sorry for writing the comment without first trying such a basic thing. What fooled me was that I wasn't expecting content to be cached when running the development server.

comment:21 by Claude Paroz, 2 years ago

If I thought about this, this means I was bitten by this, too (I reported such a wrong issue not long ago!).

comment:22 by Carlton Gibson, 2 years ago

...What fooled me was that I wasn't expecting content to be cached when running the development server.

If I thought about this, this means I was bitten by this, too (I reported such a wrong issue not long ago!).

(Reading as those two clauses are directly related.)

Telling the browser not to cache in development (presumably by setting an explicit max age of 0) was discussed in #32981 (see also #33148 and #27572).
A trip via the DevelopersMailingList to discuss re-opening would be available. (Generally I want the browser to cache even in development, because otherwise it's hard to test the real behaviour: developer tools have commands to empty the cache, and toggles for disabling that cache, with or without the web tools console open, so I'm not sure myself there's a need to add it, but it may be worth a discussion.)

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