diff --git a/apps/bandwagon/cron.py b/apps/bandwagon/cron.py index 4c2ca907df..f5892f2f8e 100644 --- a/apps/bandwagon/cron.py +++ b/apps/bandwagon/cron.py @@ -35,7 +35,7 @@ def migrate_collection_users(): if collectionusers: collection.author_id = collectionusers[0].user_id try: - collection.save(); + collection.save() collectionusers[0].delete() except: task_log.warning("No author found for collection: %d" diff --git a/apps/bandwagon/helpers.py b/apps/bandwagon/helpers.py index b169715311..18edcf4995 100644 --- a/apps/bandwagon/helpers.py +++ b/apps/bandwagon/helpers.py @@ -1,5 +1,4 @@ import math -import random import jinja2 from jingo import register, env @@ -47,7 +46,7 @@ def barometer(context, collection): c = dict(context.items()) request = c['request'] - user_vote = 0 # Non-zero if logged in and voted. + user_vote = 0 # Non-zero if logged in and voted. if request.user.is_authenticated(): # TODO: Use reverse when bandwagon is on Zamboni. @@ -68,7 +67,6 @@ def barometer(context, collection): login_title = _('Log in to vote for this collection') up_title = down_title = cancel_title = login_title - up_class = 'upvotes' down_class = 'downvotes' @@ -87,5 +85,5 @@ def barometer(context, collection): down_class += ' voted' up_class c.update(locals()) - c.update({ 'c': collection, }) + c.update({'c': collection}) return c diff --git a/apps/bandwagon/models.py b/apps/bandwagon/models.py index d0699652cd..fd42b5873c 100644 --- a/apps/bandwagon/models.py +++ b/apps/bandwagon/models.py @@ -279,7 +279,7 @@ class CollectionUser(models.Model): class CollectionVote(models.Model): - collection = models.ForeignKey(Collection) + collection = models.ForeignKey(Collection, related_name='votes') user = models.ForeignKey(UserProfile, related_name='votes') vote = models.SmallIntegerField(default=0) created = models.DateTimeField(null=True, auto_now_add=True) @@ -287,6 +287,17 @@ class CollectionVote(models.Model): class Meta: db_table = 'collections_votes' + @staticmethod + def post_save_or_delete(sender, instance, **kwargs): + from . import tasks + tasks.collection_votes.delay(instance.collection_id) + + +models.signals.post_save.connect(CollectionVote.post_save_or_delete, + sender=CollectionVote) +models.signals.post_delete.connect(CollectionVote.post_save_or_delete, + sender=CollectionVote) + class SyncedCollection(Collection): """Collection that automatically sets type=SYNC.""" diff --git a/apps/bandwagon/sql/collectionvote.sql b/apps/bandwagon/sql/collectionvote.sql deleted file mode 100644 index 97841f97de..0000000000 --- a/apps/bandwagon/sql/collectionvote.sql +++ /dev/null @@ -1,4 +0,0 @@ -ALTER TABLE `collections_votes` CHANGE COLUMN `id` `id` int(11) NOT NULL; - -ALTER TABLE `collections_votes` DROP PRIMARY KEY, - ADD PRIMARY KEY(`collection_id`, `user_id`); diff --git a/apps/bandwagon/tasks.py b/apps/bandwagon/tasks.py index 4917b9612f..8b0df25879 100644 --- a/apps/bandwagon/tasks.py +++ b/apps/bandwagon/tasks.py @@ -1 +1,21 @@ -from . import cron +import logging + +from django.db.models import Count + +from celery.decorators import task + +from . import cron # Pull in tasks run through cron. +from .models import Collection, CollectionVote + +log = logging.getLogger('z.task') + + +@task +def collection_votes(*ids): + log.info('[%s@%s] Updating collection votes.' % + (len(ids), collection_votes.rate_limit)) + for collection in ids: + v = CollectionVote.objects.filter(collection=collection) + votes = dict(v.values_list('vote').annotate(Count('vote'))) + qs = Collection.objects.filter(id=collection) + qs.update(upvotes=votes.get(1, 0), downvotes=votes.get(-1, 0)) diff --git a/apps/bandwagon/tests/test_helpers.py b/apps/bandwagon/tests/test_helpers.py index 1923ec5628..bbced0a014 100644 --- a/apps/bandwagon/tests/test_helpers.py +++ b/apps/bandwagon/tests/test_helpers.py @@ -5,7 +5,6 @@ from mock import Mock from pyquery import PyQuery as pq import jingo -import amo from bandwagon.helpers import (user_collection_list, barometer, collection_favorite) from bandwagon.models import Collection @@ -58,7 +57,6 @@ class TestHelpers(test.TestCase): eq_(doc('form')[0].action, remora_url('collections/vote/%s/up' % collection.uuid)) - def test_user_collection_list(self): c1 = Collection.objects.create( uuid='eb4e3cd8-5cf1-4832-86fb-a90fc6d3765c') diff --git a/apps/bandwagon/tests/test_models.py b/apps/bandwagon/tests/test_models.py index 0fe7dfc770..cba473fa19 100644 --- a/apps/bandwagon/tests/test_models.py +++ b/apps/bandwagon/tests/test_models.py @@ -8,7 +8,6 @@ import amo from addons.models import Addon, AddonRecommendation from bandwagon.models import (Collection, SyncedCollection, RecommendedCollection) -import settings from users.models import UserProfile @@ -31,7 +30,7 @@ class TestCollections(test_utils.TestCase): def test_is_subscribed(self): c = Collection.objects.get(pk=512) u = UserProfile() - u.nickname='unique' + u.nickname = 'unique' u.save() c.subscriptions.create(user=u) assert c.is_subscribed(u), "User isn't subscribed to collection." diff --git a/apps/bandwagon/tests/test_views.py b/apps/bandwagon/tests/test_views.py index 82f9b23803..4ab76b4672 100644 --- a/apps/bandwagon/tests/test_views.py +++ b/apps/bandwagon/tests/test_views.py @@ -1,7 +1,8 @@ -from nose.tools import eq_, with_setup +from nose.tools import eq_ import test_utils -from bandwagon.models import Collection +from amo.urlresolvers import reverse +from bandwagon.models import Collection, CollectionVote class TestViews(test_utils.TestCase): @@ -27,3 +28,67 @@ class TestViews(test_utils.TestCase): ('/collection/404', 404)] for test in tests: self.check_response(*test) + + +class TestVotes(test_utils.TestCase): + fixtures = ['users/test_backends'] + + def setUp(self): + self.client.login(username='jbalogh@mozilla.com', password='foo') + args = ['fligtar', 'slug'] + Collection.objects.create(slug='slug', author_id=9945) + self.c_url = reverse('collections.detail', args=args) + self.up = reverse('collections.vote', args=args + ['up']) + self.down = reverse('collections.vote', args=args + ['down']) + + def test_login_required(self): + self.client.logout() + r = self.client.post(self.up, follow=True) + url, _ = r.redirect_chain[-1] + eq_(r.status_code, 200) + self.assert_(reverse('users.login') in url) + + def test_post_required(self): + r = self.client.get(self.up, follow=True) + self.assertRedirects(r, self.c_url) + + def check(self, upvotes=0, downvotes=0): + c = Collection.uncached.get(slug='slug', author=9945) + eq_(c.upvotes, upvotes) + eq_(c.downvotes, downvotes) + eq_(CollectionVote.objects.filter(user=4043307, vote=1).count(), + upvotes) + eq_(CollectionVote.objects.filter(user=4043307, vote=-1).count(), + downvotes) + eq_(CollectionVote.objects.filter(user=4043307).count(), + upvotes + downvotes) + + def test_upvote(self): + self.client.post(self.up) + self.check(upvotes=1) + + def test_downvote(self): + self.client.post(self.down) + self.check(downvotes=1) + + def test_down_then_up(self): + self.client.post(self.down) + self.check(downvotes=1) + self.client.post(self.up) + self.check(upvotes=1) + + def test_up_then_up(self): + self.client.post(self.up) + self.check(upvotes=1) + self.client.post(self.up) + self.check(upvotes=0) + + def test_normal_response(self): + r = self.client.post(self.up, follow=True) + self.assertRedirects(r, self.c_url) + + def test_ajax_response(self): + r = self.client.post(self.up, follow=True, + HTTP_X_REQUESTED_WITH='XMLHttpRequest') + assert not r.redirect_chain + eq_(r.status_code, 200) diff --git a/apps/bandwagon/urls.py b/apps/bandwagon/urls.py index 09a53f256d..30cd650e62 100644 --- a/apps/bandwagon/urls.py +++ b/apps/bandwagon/urls.py @@ -1,14 +1,21 @@ -from django.conf.urls.defaults import patterns, url +from django.conf.urls.defaults import patterns, url, include from . import views +detail_urls = patterns('', + url('^$', views.collection_detail, name='collections.detail'), + url('^vote/(?Pup|down)$', views.collection_vote, + name='collections.vote'), +) + + urlpatterns = patterns('', url('^collection/(?P[^/]+)/?$', views.legacy_redirect), url('^collections/$', views.collection_listing, name='collections.list'), - url('^collections/(?P[^/]+)/$', views.user_listing, + url('^collections/(?P[^/]+)/$', views.user_listing, name='collections.user'), - url('^collections/(?P[^/]+)/(?P[^/]+)$', - views.collection_detail, name='collections.detail'), + url('^collections/(?P[^/]+)/(?P[^/]+)/', + include(detail_urls)), ) diff --git a/apps/bandwagon/views.py b/apps/bandwagon/views.py index d7a85a2789..573f2d2873 100644 --- a/apps/bandwagon/views.py +++ b/apps/bandwagon/views.py @@ -1,4 +1,5 @@ from django import http +from django.contrib.auth.decorators import login_required from django.shortcuts import get_object_or_404, redirect import jingo @@ -8,7 +9,7 @@ import amo.utils from addons.models import Addon from addons.views import BaseFilter from translations.query import order_by_translation -from .models import Collection, CollectionAddon +from .models import Collection, CollectionAddon, CollectionVote def legacy_redirect(self, uuid): @@ -22,7 +23,7 @@ def collection_listing(request): return http.HttpResponse() -def user_listing(request, user): +def user_listing(request, username): return http.HttpResponse() @@ -41,7 +42,7 @@ class CollectionAddonFilter(BaseFilter): .with_index(addons='downloads_type_idx')) -def collection_detail(request, user, slug): +def collection_detail(request, username, slug): # TODO: owner=user when dd adds owner to collections cn = get_object_or_404(Collection.objects, slug=slug) base = cn.addons.all() & Addon.objects.listed(request.APP) @@ -64,3 +65,26 @@ def get_notes(collection): for note in notes: rv[note.addon_id] = note.comments yield rv + + +@login_required +def collection_vote(request, username, slug, direction): + cn = get_object_or_404(Collection.objects, slug=slug) + if request.method != 'POST': + return redirect(cn.get_url_path()) + + vote = {'up': 1, 'down': -1}[direction] + cv, new = CollectionVote.objects.get_or_create( + collection=cn, user=request.amo_user, defaults={'vote': vote}) + + if not new: + if cv.vote == vote: # Double vote => cancel. + cv.delete() + else: + cv.vote = vote + cv.save() + + if request.is_ajax(): + return http.HttpResponse() + else: + return redirect(cn.get_url_path()) diff --git a/apps/users/fixtures/users/test_backends.json b/apps/users/fixtures/users/test_backends.json index 966511c7d3..ac5b6a16f1 100644 --- a/apps/users/fixtures/users/test_backends.json +++ b/apps/users/fixtures/users/test_backends.json @@ -88,7 +88,7 @@ "emailhidden": 1, "user": null, "password": "sha512$32e15df727a054aa56cf69accc142d1573372641a176aab9b0f1458e27dc6f3b$5bd3bd7811569776a07fbbb5e50156aa6ebdd0bec9267249b57da065340f0324190f1ad0d5f609dca19179a86c64807e22f789d118e6f7109c95b9c64ae8f619", - "nickname": "Justin Scott (fligtar)", + "nickname": "fligtar", "resetcode_expires": "2010-01-12 15:28:07", "resetcode": "", "created": "2007-03-05 13:09:37", diff --git a/migrations/54-drop-collection-vote-trigger.sql b/migrations/54-drop-collection-vote-trigger.sql new file mode 100644 index 0000000000..a23b569327 --- /dev/null +++ b/migrations/54-drop-collection-vote-trigger.sql @@ -0,0 +1,2 @@ +DROP TRIGGER IF EXISTS collection_vote_insert; +DROP TRIGGER IF EXISTS collection_vote_delete;