Opened 4 months ago

Closed 4 months ago

Last modified 2 months 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 Changed 4 months ago by Fabian Jarrett

Owner: changed from nobody to Fabian Jarrett
Status: newassigned

comment:2 Changed 4 months ago by Mariusz Felisiak

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

Thanks for the report! Regression in fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa.

comment:3 Changed 4 months ago by Claude Paroz

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

comment:4 Changed 4 months ago by Carlton Gibson

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 Changed 4 months ago by Marcelo Galigniana

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 4 months ago by Marcelo Galigniana (previous) (diff)

comment:6 Changed 4 months ago by Mariusz Felisiak

Agreed, let's revert it.

comment:7 Changed 4 months ago by 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.

comment:8 in reply to:  7 Changed 4 months ago by Fabian Jarrett

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 Changed 4 months ago by Fabian Jarrett

Last edited 4 months ago by Fabian Jarrett (previous) (diff)

comment:10 Changed 4 months ago by Fabian Jarrett

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

comment:11 Changed 4 months ago by Mariusz Felisiak

Has patch: set
Needs tests: set

comment:12 Changed 4 months ago by Mariusz Felisiak

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 0756c61f:

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

Regression in fe7dbef5867c577995f0fc849d8dfdb8f2e6bbfa.

comment:14 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 4 months ago by Mariusz Felisiak

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

comment:16 Changed 3 months ago by ADHD is DEV

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 Changed 3 months ago by Tim Graham

The fix will be released in Django 4.1.1.

comment:18 Changed 2 months ago by Emanuel Angelo

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

comment:19 Changed 2 months ago by Claude Paroz

Did you try after emptying the browser cache?

comment:20 in reply to:  19 Changed 2 months ago by Emanuel Angelo

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 Changed 2 months ago by Claude Paroz

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

comment:22 Changed 2 months ago by Carlton Gibson

...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