Created on 2014-03-05 20:21 by r.david.murray, last changed 2014-03-12 16:49 by ber.
|linkhtmlproperty.patch||r.david.murray, 2014-03-06 20:06|
|msg4994||Author: [hidden] (r.david.murray)||Date: 2014-03-05 20:21|
I ran across this fixing the classic query.edit.html template. There is a snippet that does: query.private_for == None This is never true, even if the _value of private_for is None. The reason is that because the HTMLProperty classes are old style classes, python looks for an __eq__ method and, when it doesn't find one (because neither LinkHTMLProperty nor any of its superclasses define one) LinkHTMLProperty's __getattr__ is called. That method does: if not self._value: <construct MissingValue object and look up attr on it) This is obviously never going to equal None. I can fix the template by doing 'not query.private_for', since __nonzero__ is defined by a subclass, but it seems to me that this is a bug in LinkHTMLProperty. This could be fixed by making LinkHTMLProperty a new-style class, but I don't know if that would have any unexpected consequences.
|msg4995||Author: [hidden] (ber)||Date: 2014-03-06 08:15|
David, thanks for reporting the issue! Can you say more about the consequences this defect has? As for the consequences of changing the LinkHTMLProperty class: If it is easy, you could try doing the chance and ran the included testsuite. Best, Bernhard
|msg4996||Author: [hidden] (r.david.murray)||Date: 2014-03-06 13:49|
Well, in the specific case of the query.edit page, the consequence was that all queries were shown as 'private', whether they were or not, and then when the form was submitted for any reason, all queries were set to private, whether they were before or not, unless you had changed them back before submitting. In general the problem is that if a LinkHTMLProperty is None, and you try to *test* that it is None using == (you can't use 'is', obviously, which is a different but unavoidable problem), things don't work as you expect, and (the big problem) the failure is silent. I will try the test suite when I have time (later today, with any luck).
|msg4998||Author: [hidden] (r.david.murray)||Date: 2014-03-06 20:06|
With the attached patch applied, all unit tests pass. I looked at the unit tests a bit, but it isn't at all obvious how I'd write a test for this using the current infrastructure. I mean, it looks like it is quite possible, but I don't know enough to do it.
|msg4999||Author: [hidden] (ber)||Date: 2014-03-07 08:04|
David, thanks for the patch! I admin I probably have to look up a bit before I can check that it is helpful, but maybe we'll find somebody else to review it on the list. As for the unit tests: Usually I copy one of the existing test files and adapt them. Best, Bernhard
|msg5006||Author: [hidden] (ber)||Date: 2014-03-10 11:32|
To switch over from Issue2550831: David, Yes, it would be cool to now that this change also got some production exposure.
|msg5015||Author: [hidden] (ber)||Date: 2014-03-11 14:20|
I'm peeking a look in the code trying to understand the impliciation and if this is the best fix. Am I right that an alternative solution would be to implement __eq__ and keep the old-style class? Because this probably would be the only new style class in that area and I wonder about consistency. If we wanted to go to new style classes, why not make HTMLInputMixin a newstyle class? What about the other objects. in cgi/templating.py
|msg5016||Author: [hidden] (r.david.murray)||Date: 2014-03-11 14:42|
Yeah, I changed that class only in my test just to have a minimal change. I think you would have to implement all the rich comparison methods to fix it fully without changing the class type. I'll try changing HTMLInputMixin to new-style in my tracker instance and see if it breaks anything. I don't think it will.
|msg5017||Author: [hidden] (ber)||Date: 2014-03-12 11:19|
Reproduced an unwanted behaviour, instructions: a) start a demo instance, e.g. ppython demo.py -b postgresql nuke b) login as admin, create an additional user A c) login as demo and in a different webbrowser as A d) create a named query with demo e) verify that A cannot see this query f) as demo: switch the "private" on this query to now and save Observations: private is still shown as "yes" A cannot see the search entry. Wanted behaviour: private shown as "no" and A can see the query. Adding the following small change makes the above test pass. -class HTMLInputMixin: +class HTMLInputMixin(object): Interesting enough, in an old installation, setting a query to non-private worked, so it got broken somewhere.
|msg5018||Author: [hidden] (ber)||Date: 2014-03-12 16:49|
this change removes raising an exception if an attribute is not found: hg log -r 3164 Änderung: 3164:24476db8872f Nutzer: Richard Jones <email@example.com> Datum: Mon Feb 14 04:53:41 2005 +0000 Zusammenfassung: nicer error looking up values of None (response to [SF#1108697] @@ -1565,7 +1568,8 @@ ''' return a new HTMLItem ''' #print 'Link.getattr', (self, attr, self._value) if not self._value: - raise AttributeError, "Can't access missing value" + msg = self._('Attempt to look up %(attr)s on a missing value') + return MissingValue(msg%locals()) i = HTMLItem(self._client, self._prop.classname, self._value) return getattr(i, attr) I'm observing the bad behaviour the first time after Änderung: 3913:00896a2acaa5 Nutzer: Justus Pendleton <firstname.lastname@example.org> Datum: Thu Sep 20 23:44:58 2007 +0000 Zusammenfassung: clean up query display of "Private to you" items No obvious problem found, it probably did not get tested well enough after the rev3913 change. I've decided for the smaller fix, because it limits possible undetected side effect, changing LinkHTMLProperty to a new-style class. Committed with rev4864:f630eb0adcee Thanks again David!
|2014-03-12 16:49:40||ber||set||status: new -> fixed|
messages: + msg5018
|2014-03-12 11:19:38||ber||set||messages: + msg5017|
|2014-03-11 14:42:37||r.david.murray||set||messages: + msg5016|
|2014-03-11 14:20:33||ber||set||messages: + msg5015|
|2014-03-10 11:32:00||ber||set||messages: + msg5006|
|2014-03-07 08:04:20||ber||set||messages: + msg4999|
keywords: + patch
messages: + msg4998
|2014-03-06 19:13:19||r.david.murray||set||title: A LinkHTMLProperty cannot be compared successfully -> An empty LinkHTMLProperty cannot be compared successfully|
|2014-03-06 13:49:54||r.david.murray||set||messages: + msg4996|
messages: + msg4995