Issue 2550830
Created on 2014-03-05 20:21 by r.david.murray, last changed 2014-03-12 16:49 by ber.
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 <richard@users.sourceforge.net>
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 <jpend@users.sourceforge.net>
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!
|
|
Date |
User |
Action |
Args |
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 |
2014-03-06 20:06:21 | r.david.murray | set | files:
+ linkhtmlproperty.patch 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 |
2014-03-06 08:15:53 | ber | set | nosy:
+ ber messages:
+ msg4995 |
2014-03-05 20:21:15 | r.david.murray | create | |
|