disqus / gargoyle

Feature switches in Django
http://engineering.disqus.com
Apache License 2.0
746 stars 112 forks source link

"not Anonymous" condition broken? #38

Closed akaihola closed 10 years ago

akaihola commented 12 years ago

Either I've understood the documentation wrong, or there's a problem when trying to test if the user is not anonymous. A switch with a "not Anonymous" condition seems to never be active.

Here's how to reproduce:

mkdir /tmp/gargoyletest
cd /tmp/gargoyletest
virtualenv --no-site-packages --distribute .
. bin/activate
git clone https://github.com/disqus/gargoyle.git
cd gargoyle
python setup.py develop
python setup.py test
rm -rf *.egg
pip install Django==1.4.1 South==0.7.6
cd /tmp/gargoyletest/gargoyle/example_project
./manage.py runserver

In a browser:

In the terminal, type Ctrl-C and continue:

cat >>urls.py <<EOF
from django.http import HttpResponse
from django.template import Context, Template
urlpatterns += patterns('',
    url(r'test/$',
        lambda request: HttpResponse(
            Template('''

{% load gargoyle_tags %}
<pre>
    {% ifswitch not-anonymous %}
        Is not anonymous
    {% else %}
        Is anonymous
    {% endifswitch %}
</pre>

''').render(Context())
        )
    )
)
EOF
./manage.py runserver

Back in the browser:

Renders: "Is anonymous" <----------- WHY?

Renders: "Is anonymous" (as expected).

I also wrote a test for this. Is it written correctly? Run my test by hitting Ctrl-C in the terminal and pasting the following:

cd /tmp/gargoyletest/gargoyle
patch -p0 <<EOF
diff --git tests/tests.py tests/tests.py
index 1d6b44a..1bdb9e4 100644
--- tests/tests.py
+++ tests/tests.py
@@ -425,6 +425,18 @@ class APITest(TestCase):

         self.assertTrue(self.gargoyle.is_active('test', user))

+        switch.clear_conditions(
+            condition_set=condition_set,
+        )
+
+        switch.add_condition(
+            condition_set=condition_set,
+            field_name='is_anonymous',
+            condition='0',
+        )
+
+        self.assertFalse(self.gargoyle.is_active('test', user))
+
         switch.add_condition(
             condition_set=condition_set,
             field_name='percent',
EOF
python setup.py test

The test we just added fails <------- WHY?

Update: now that Gargoyle supports Django 1.4, removed the 1.3 hack and installed 1.4.1 (and South 0.7.6) instead. The symptom is still same as before.

akaihola commented 12 years ago

I ran the tests with a debugger breakpoint in UserConditionSet.is_active(). This is what I see for a passing ("is anonymous") test:

> /tmp/gargoyletest/gargoyle/gargoyle/builtins.py(43)is_active()
     42             import ipdb;ipdb.set_trace()  ## DEBUG
---> 43             return bool(condition)
     44         return None

ipdb> print instance
AnonymousUser
ipdb> print conditions
{u'auth.user': {u'is_anonymous': [[u'i', u'1']]}}
ipdb> print self.get_namespace()
auth.user
ipdb> print condition
[[u'i', u'1']]

Here's the same for a failing ("is not anonymous") test:

> /tmp/gargoyletest/gargoyle/gargoyle/builtins.py(43)is_active()
     42             import ipdb;ipdb.set_trace()  ## DEBUG
---> 43             return bool(condition)
     44         return None

ipdb> print instance
AnonymousUser
ipdb> print conditions
{u'auth.user': {u'is_anonymous': [[u'i', u'0']]}}
ipdb> print self.get_namespace()
auth.user
ipdb> print condition
[[u'i', u'0']]

It doesn't look right to me.

amandasaurus commented 11 years ago

I encounted this problem. There is a bug where if your conditions is "not X", then rather than the default being True, but False for some users (as one expects), it will actually be False for all.

I have submitted an issue & pull request which fixes it for me https://github.com/disqus/gargoyle/pull/53

Does this fix your problem?

akaihola commented 11 years ago

@rory No, your patch doesn't seem to fix the case I described. If I create a switch with the condition "not Anonymous", a template with an {% ifswitch %}, log in to the site and load the page, the stuff inside {% ifswitch %} doesn't render.

I tried both with the current disqus/gargoyle HEAD with your patch, and the HEAD of rory/gargoyle.

In the debugger, I see that SwitchManager.is_active(), is called without any *instances arguments. Maybe this is the problem?

In the for switch loop, I see the following values:

ipdb> switch
<UserConditionSet: User>
ipdb> conditions
{u'auth.user': {u'is_anonymous': [[u'e', u'1']]}}
ipdb> instances
()
akaihola commented 11 years ago

@rory Sorry, turns out we didn't have Django's request context processor activated. Fixed that, now works ok. Your patch is good. Hope @dcramer accepts it into a new release soon.