Code

Opened 3 years ago

Closed 3 years ago

#16154 closed Bug (fixed)

r16051 broke editing a filtered changelist

Reported by: kmtracey Owned by: nobody
Component: contrib.admin Version: 1.3
Severity: Release blocker Keywords: regression dataloss
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

r16051 changed a bunch of form actions in the admin from action="" to action=".". I have noticed a side-effect of this change is that editing a filtered changelist is now broken: the edit can affect rows other then the ones intended. Looking at the dev server logging for the change list edit POST, with r16050 I see:

[04/Jun/2011 12:24:25] "POST /admin/ctrac/cat/?avail__exact=1 HTTP/1.1" 302 0

whereas with r16051 I see:

[04/Jun/2011 12:20:50] "POST /admin/ctrac/cat/ HTTP/1.1" 302 0

The change list filtering information is no longer being sent with the POST, and as a result the queryset used to process the POSTed edit doesn't match the original queryset that was used to construct the filtered change list for display. The effect that I have seen, without completely tracking down how it is happening, is that rows other than the filtered ones are being changed by the change list edit. For affected objects that should not have been affected I see messages in their history like:

Changed avail and id.

Although id was not actually changed -- I think that's an indication that the hidden id field for the object didn't match up with the value in the queryset used by POST...but the admin went ahead and changed the other data (avail) anyway.

Attachments (0)

Change History (10)

comment:1 Changed 3 years ago by kmtracey

In [16328]:

Undo part of r16051 to avoid dataloss bug. Refs #16154.

comment:2 Changed 3 years ago by kmtracey

For now I have undone part of r16051 -- the change list form -- in order to get a fix in for the dataloss problem. I'm not sure what the right fix is long-term and I don't know if any of the other forms changed are subject to the same problem, but while we investigate that I'd like to make sure that continued data corruption of the type I've observed is prevented.

comment:3 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by lukeplant

Gulp, my bad, sorry. I had no idea that action="" and action="." meant different things, but I ought to have. Looking at RFC 1808 (see sections 5.1 and 5.2, and 4.2.a), they are indeed different, and the browser is doing what it is supposed to do. It is possible the validator was just incorrect, and it should be ignored anyway, given that an empty action is the only way to post back to exactly the same URL.

So we should be reverting much more of [16051] - almost all apart from the obsolete 'cellspacing' attribute.

There is, of course, another bug here - it shouldn't be possible for the edit to affect rows that were not specifically selected, even if the queryset returns more objects than it did before, which is entirely possible is someone else has changed the data in the database in the mean time. I'm sure there is a separate ticket about this, but I can't find it.

comment:5 Changed 3 years ago by lukeplant

In [16330]:

Reverted most of [16051], because it was thoroughly incorrect (whatever some validator says)

Refs #16154

comment:6 Changed 3 years ago by kmtracey

The validator may be right, per: http://dev.w3.org/html5/spec/Overview.html#attr-fs-action which says if action is specified it must be non-empty. I guess maybe with HTML5 it's not supposed to be specified at all if it's empty. Of course it used to be "required"...so it seems leaving it entirely unspecified might be troublesome for older browsers, though in a quick test with FF 3.6.17, Chromium 12, and IE8 leaving action out entirely looks to behave the same as action="". Oh, fun with evolving "standards".

comment:7 Changed 3 years ago by kmtracey

(And yes, there is another bug here, I think the closest match is described by #11313. Admin is too trusting that posted data "lines up" the same when POSTED as it did when the page was originally retrieved via GET. If we had a fix for that issue this change likely would have produced some error message rather than unexpected data changes.)

comment:8 follow-up: Changed 3 years ago by lukeplant

Regarding the HTML5 draft spec, there is this bug filed: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12561

I added some comments. I propose on that front we just say that spec is wrong, at least for now.

comment:9 in reply to: ↑ 8 Changed 3 years ago by kmtracey

Replying to lukeplant:

I added some comments. I propose on that front we just say that spec is wrong, at least for now.

Works for me. Given that, we could close this now, right? It sounds like going back to using the empty string for action is the right fix, and that change has been made as noted above. We do still have the admin fragility in this area as another underlying bug, but that's a different ticket.

comment:10 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

Yep, let's close this, and use #11313 for ongoing issues.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.