r16051 broke editing a filtered changelist

[Karen Tracey]

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.

[Karen Tracey]

In [16328]:

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

[Karen Tracey]

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.

[Luke Plant]

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.

[Luke Plant]

In [16330]:

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

Refs #16154

[Karen Tracey]

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

[Karen Tracey]

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

[Luke Plant]

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.

[Karen Tracey]

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.

[Luke Plant]

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