diff --git a/deploy/pip_packages.txt b/deploy/pip_packages.txt index b7a0dc66..77766a27 100644 --- a/deploy/pip_packages.txt +++ b/deploy/pip_packages.txt @@ -27,6 +27,7 @@ django-parsley==0.0.2a0 django-waffle==0.9.1 Markdown==2.3.1 -e git+git://github.com/aptivate/aptivate-monkeypatch.git@ed4f821d8d57ba181c418b8d78dfd8bca70d6817#egg=aptivate_monkeypatch +decorators==0.1 # for search django-haystack==2.1.0 @@ -51,6 +52,6 @@ django-debug-toolbar==0.9.4 # TODO: remove when bug fix release is available # working around bug # https://bitbucket.org/logilab/astroid/issue/3/error-after-upgrade-to-pylint-10 -hg+https://bitbucket.org/logilab/astroid +astroid==1.2.1 # pylint 1.4 does not support python 2.6, so until we upgrade to 2.7 we need pylint<1.3 diff --git a/django/econsensus/publicweb/custom_decorators.py b/django/econsensus/publicweb/custom_decorators.py new file mode 100644 index 00000000..f541ded1 --- /dev/null +++ b/django/econsensus/publicweb/custom_decorators.py @@ -0,0 +1,20 @@ +from django.http import Http404 +from actionitems.models import ActionItem +import decorators + + +class is_org_member(decorators.FuncDecorator): + + def decorate_func(self, func, *args, **kwargs): + def wrapped_func(self, request, *args, **kwargs): + item = self.get_object() + if(isinstance(item, ActionItem)): + org = item.origin.organization + else: + org = item.organization + + if org in request.user.organization_set.all(): + return func(self, request, *args, **kwargs) + else: + raise Http404 + return wrapped_func diff --git a/django/econsensus/publicweb/single_action_views.py b/django/econsensus/publicweb/single_action_views.py new file mode 100644 index 00000000..809c7fd7 --- /dev/null +++ b/django/econsensus/publicweb/single_action_views.py @@ -0,0 +1,80 @@ +from django.http import HttpResponseRedirect +from django.shortcuts import get_object_or_404 +from django.views.generic.base import View + +from actionitems.models import ActionItem +from guardian.mixins import LoginRequiredMixin +from notification import models as notification +from signals.management import DECISION_CHANGE + +from .custom_decorators import is_org_member +from .models import Decision + + +class BaseSingleActionView(LoginRequiredMixin, View): + """ SingleActionViews are views used to perform a single, simple + action such as marking an item as done. This used with a + redirection to provide quick, one-click actions that do + not require javascript. + + - Users need to provide a URL route to the single action view; + - Descendant classes should implement a single 'do_action' + method to perform the action; + - Requests are expected to contains a GET parameter 'next' then the + user will be redirected to the given URL. + """ + @is_org_member + def get(self, request, *args, **kwargs): + self.do_action() + return HttpResponseRedirect(request.GET['next']) + + +class BaseWatcherView(BaseSingleActionView): + """ Base single action view for add/remove watcher views """ + def get_object(self): + object_id = self.kwargs['decision_id'] + decision = get_object_or_404(Decision, pk=object_id) + return decision + + def get_user(self): + return self.request.user + + +class BaseActionItemView(BaseSingleActionView): + """ Base single action view for set/unset action item done views """ + def get_object(self): + object_id = self.kwargs['actionitem_id'] + actionitem = get_object_or_404(ActionItem, pk=object_id) + return actionitem + + +class AddWatcher(BaseWatcherView): + """ Single action view used to add a new watcher to a decision """ + def do_action(self): + decision = self.get_object() + user = self.get_user() + notification.observe(decision, user, DECISION_CHANGE) + + +class RemoveWatcher(BaseWatcherView): + """ Single action view used to remove a watcher from a decision """ + def do_action(self): + decision = self.get_object() + user = self.get_user() + notification.stop_observing(decision, user) + + +class SetActionItemDone(BaseActionItemView): + """ Single action view used to set an action item as done """ + def do_action(self): + actionitem = self.get_object() + actionitem.done = True + actionitem.save() + + +class UnsetActionItemDone(BaseActionItemView): + """ Single action view used to unset an action item's done status """ + def do_action(self): + actionitem = self.get_object() + actionitem.done = False + actionitem.save() diff --git a/django/econsensus/publicweb/templates/actionitem_detail_snippet.html b/django/econsensus/publicweb/templates/actionitem_detail_snippet.html index be668414..305c6430 100644 --- a/django/econsensus/publicweb/templates/actionitem_detail_snippet.html +++ b/django/econsensus/publicweb/templates/actionitem_detail_snippet.html @@ -26,6 +26,11 @@ {% else %}{% trans "No deadline" %} {% endif %} +
  • + + {% if object.done %}☑{% else %}☐{% endif %} {% trans "Done" %} + +
    • diff --git a/django/econsensus/publicweb/templates/decision_list.html b/django/econsensus/publicweb/templates/decision_list.html index bf0d2f3b..1afce5b8 100644 --- a/django/econsensus/publicweb/templates/decision_list.html +++ b/django/econsensus/publicweb/templates/decision_list.html @@ -63,13 +63,27 @@ {% for object in object_list %} - {% if tab != 'actionitems' %} + {% if tab == 'actionitems' %} + {% switch "actionitems" %} + {{ object.id }} + + + {% if object.done %}☑{% else %}☐{% endif %} + + + {{ object.description|get_excerpt }} + {{ object.responsible }} + {{ object.deadline }} + + {{ object.origin.id }} + {% endswitch %} + {% else %} {{ object.id }} - + {{ object.excerpt }} {% if tab == 'decision' %} {{ object.decided_date }} @@ -83,16 +97,6 @@ {{ object.last_modified|timesince }} ago {% endif %} {% endif %} - {% if tab == 'actionitems' %} - {% switch "actionitems" %} - {{ object.id }} - {{ object.description|get_excerpt }} - {{ object.responsible }} - {{ object.deadline }} - - {{ object.origin.id }} - {% endswitch %} - {% endif %} {% endfor %} diff --git a/django/econsensus/publicweb/tests/actionitems_view_test.py b/django/econsensus/publicweb/tests/actionitems_view_test.py index a3d618a2..bbfea6d4 100644 --- a/django/econsensus/publicweb/tests/actionitems_view_test.py +++ b/django/econsensus/publicweb/tests/actionitems_view_test.py @@ -1,7 +1,9 @@ -from django.utils.unittest import TestCase -from django.test.client import RequestFactory +from django.contrib.auth.models import User, AnonymousUser from django.core.urlresolvers import reverse, resolve -from django.contrib.auth.models import User +from django.http import Http404 +from django.test.client import RequestFactory +from django.utils.unittest import TestCase + from BeautifulSoup import BeautifulSoup from waffle import Switch @@ -11,9 +13,12 @@ from publicweb.forms import (EconsensusActionItemCreateForm, EconsensusActionItemUpdateForm) +from publicweb.single_action_views import SetActionItemDone, UnsetActionItemDone from publicweb.views import (EconsensusActionitemCreateView, EconsensusActionitemUpdateView, EconsensusActionitemListView) +from django_dynamic_fixture import G +from publicweb.models import Decision class ActionitemsViewTestFast(TestCase): @@ -221,3 +226,109 @@ def test_list_wrongorg(self): response.render() soup = BeautifulSoup(str(response.content)) assert soup.find("p", {"class": "wrongorg"}) + + def test_set_done_view_requires_user_to_be_logged_in(self): + actionitem = ActionItem.objects.create() + user = AnonymousUser() + + view = SetActionItemDone() + view.get_object = lambda: actionitem + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + response = view.dispatch(request) + + self.assertEqual('/accounts/login/?next=/%3Fnext%3D%252Fcountry-critters%252Factionitem%252Flist%252F', response['Location']) + + def test_remove_watcher_view_requires_user_to_be_logged_in(self): + actionitem = ActionItem.objects.create() + user = AnonymousUser() + + view = UnsetActionItemDone() + view.get_object = lambda: actionitem + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + response = view.dispatch(request) + + self.assertEqual('/accounts/login/?next=/%3Fnext%3D%252Fcountry-critters%252Factionitem%252Flist%252F', response['Location']) + + def test_action_item_set_done_sets_item_done(self): + decision = G(Decision, organization=self.bettysorg) + actionitem = ActionItem.objects.create(origin=decision) + user = self.betty + + view = SetActionItemDone() + view.get_object = lambda: actionitem + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + view.dispatch(request) + + self.assertTrue(actionitem.done) + + def test_action_item_unset_done_sets_item_not_done(self): + decision = G(Decision, organization=self.bettysorg) + actionitem = ActionItem.objects.create(done=True, origin=decision) + user = self.betty + + view = UnsetActionItemDone() + view.get_object = lambda: actionitem + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + view.dispatch(request, actionitem_id=actionitem.id) + + self.assertFalse(actionitem.done) + + def test_action_item_set_done_for_non_existant_item_returns_404(self): + decision = G(Decision, organization=self.bettysorg) + actionitem = ActionItem.objects.create(origin=decision) + user = self.betty + + view = SetActionItemDone() + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, actionitem_id=actionitem.id + 1) + + def test_action_item_unset_done_for_non_existant_item_returns_404(self): + decision = G(Decision, organization=self.bettysorg) + actionitem = ActionItem.objects.create(origin=decision) + user = self.betty + + view = UnsetActionItemDone() + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, actionitem_id=actionitem.id + 1) + + def test_action_item_set_done_for_item_for_different_org_returns_404(self): + decision = G(Decision) + actionitem = ActionItem.objects.create(origin=decision) + user = self.betty + + view = SetActionItemDone() + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, actionitem_id=actionitem.id + 1) + + def test_action_item_unset_done_for_item_for_different_org_returns_404(self): + decision = G(Decision) + actionitem = ActionItem.objects.create(origin=decision) + user = self.betty + + view = UnsetActionItemDone() + + request = RequestFactory().get('/', {'next': reverse('actionitem_list', args=[self.bettysorg.slug])}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, actionitem_id=actionitem.id + 1) diff --git a/django/econsensus/publicweb/tests/watcher_view_test.py b/django/econsensus/publicweb/tests/watcher_view_test.py index 785a064a..07ac1aec 100644 --- a/django/econsensus/publicweb/tests/watcher_view_test.py +++ b/django/econsensus/publicweb/tests/watcher_view_test.py @@ -1,37 +1,45 @@ -from django.contrib.auth.models import User +from django.contrib.auth.models import User, AnonymousUser +from django.http import Http404 from django.test.client import RequestFactory from django.test.testcases import SimpleTestCase + from django_dynamic_fixture import N -from mock import patch, MagicMock +from mock import patch, Mock +from organizations.models import Organization from signals.management import DECISION_CHANGE +# For some reason using relative imports throws an error from publicweb.models import Decision -from publicweb.views import AddWatcher, RemoveWatcher +from publicweb.single_action_views import AddWatcher, RemoveWatcher -class WatcherViewTests(SimpleTestCase): +class WatcherViewTest(SimpleTestCase): - @patch('publicweb.views.notification') + @patch('publicweb.single_action_views.notification') def test_add_watcher_view_adds_observer_to_item(self, notifications): - decision = N(Decision) - user = N(User) + organization = N(Organization) + decision = N(Decision, organization=organization) + user = Mock(spec=User, id=1, organization_set=Mock(all=lambda: [organization])) + + view = AddWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user - mock_view = MagicMock(spec=AddWatcher) - mock_view.get_object = lambda: decision - mock_view.get_user = lambda: user - mock_view.get = AddWatcher.get + view.request = request + view.get(request) - mock_view.get(mock_view, RequestFactory().get('/', {'next': '/'})) notifications.observe.assert_called_with( decision, user, DECISION_CHANGE) - @patch("publicweb.views.Decision.objects") + @patch("publicweb.single_action_views.get_object_or_404") def test_get_object_tries_to_get_decision(self, decisions): view = AddWatcher() view.args = [] view.kwargs = {'decision_id': 1} view.get_object() - decisions.get.assert_called_with(pk=1) + decisions.assert_called_with(Decision, pk=1) def test_get_user_looks_for_user_in_request(self): user = N(User) @@ -42,15 +50,85 @@ def test_get_user_looks_for_user_in_request(self): self.assertEqual(user, view.get_user()) - @patch('publicweb.views.notification') + @patch('publicweb.single_action_views.notification') def test_remove_watcher_view_removes_observer_from_item(self, notifications): + organization = N(Organization) + decision = N(Decision, organization=organization) + user = Mock(spec=User, id=1, organization_set=Mock(all=lambda: [organization])) + + view = RemoveWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + view.request = request + view.get(request) + notifications.stop_observing.assert_called_with(decision, user) + + def test_add_watcher_view_requires_user_to_be_logged_in(self): + decision = N(Decision) + user = AnonymousUser() + + view = AddWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + response = view.dispatch(request) + + self.assertEqual('/accounts/login/?next=/%3Fnext%3D%252F', response['Location']) + + def test_remove_watcher_view_requires_user_to_be_logged_in(self): decision = N(Decision) + user = AnonymousUser() + + view = RemoveWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + response = view.dispatch(request) + + self.assertEqual('/accounts/login/?next=/%3Fnext%3D%252F', response['Location']) + + def test_add_watcher_for_non_existant_item_returns_404(self): + Decision.objects.all().delete() user = N(User) - mock_view = MagicMock(spec=RemoveWatcher) - mock_view.get_object = lambda: decision - mock_view.get_user = lambda: user - mock_view.get = RemoveWatcher.get + view = AddWatcher() + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, decision_id=1) + + def test_add_watcher_for_item_from_different_org_returns_404(self): + Decision.objects.all().delete() + organization = N(Organization) + user = Mock(spec=User, id=1, organization_set=Mock(all=lambda: [organization])) + decision = N(Decision) + + view = AddWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, decision_id=1) + + def test_remove_watcher_for_item_from_different_org_returns_404(self): + organization = N(Organization) + user = Mock(spec=User, id=1, organization_set=Mock(all=lambda: [organization])) + decision = N(Decision) + + view = RemoveWatcher() + view.get_object = lambda: decision + + request = RequestFactory().get('/', {'next': '/'}) + request.user = user + + self.assertRaises(Http404, view.dispatch, request, decision_id=1) - mock_view.get(mock_view, RequestFactory().get('/', {'next': '/'})) - notifications.stop_observing.assert_called_with(decision, user) diff --git a/django/econsensus/publicweb/urls.py b/django/econsensus/publicweb/urls.py index e960a3be..09d34e17 100644 --- a/django/econsensus/publicweb/urls.py +++ b/django/econsensus/publicweb/urls.py @@ -11,7 +11,9 @@ EconsensusActionitemDetailView, DecisionSearchView) from models import Feedback -from publicweb.views import AddWatcher, RemoveWatcher +from publicweb.single_action_views import (AddWatcher, RemoveWatcher, + SetActionItemDone, + UnsetActionItemDone) urlpatterns = patterns('econsensus.publicweb.views', @@ -20,10 +22,16 @@ name='your_details'), url(r'^user_settings/notification_settings/(?P[-\w]+)/$', UserNotificationSettings.as_view(), name='notification_settings'), + + # Single action urls url(r'^add_watcher/(?P\d+)/$', AddWatcher.as_view(), name="add_watcher"), url(r'^remove_watcher/(?P\d+)/$', RemoveWatcher.as_view(), - name="remove_watcher"), + name="remove_watcher"), + url(r'^set_actionitem_done/(?P\d+)/$', SetActionItemDone.as_view(), + name="set_actionitem_done"), + url(r'^unset_actionitem_done/(?P\d+)/$', UnsetActionItemDone.as_view(), + name="unset_actionitem_done"), url(r'^(?P[-\w]+)/export_csv/$', ExportCSV.as_view(), diff --git a/django/econsensus/publicweb/views.py b/django/econsensus/publicweb/views.py index c4d493ce..71e17c1a 100644 --- a/django/econsensus/publicweb/views.py +++ b/django/econsensus/publicweb/views.py @@ -944,29 +944,3 @@ def make(cls): def search_view(request, *args, **kwargs): return cls()(request, *args, **kwargs) return login_required(search_view) - - -class BaseWatcherView(View): - def get_object(self): - object_id = self.kwargs['decision_id'] - decision = Decision.objects.get(pk=object_id) - return decision - - def get_user(self): - return self.request.user - - -class AddWatcher(BaseWatcherView): - def get(self, request, *args, **kwargs): - decision = self.get_object() - user = self.get_user() - notification.observe(decision, user, DECISION_CHANGE) - return HttpResponseRedirect(request.GET['next']) - - -class RemoveWatcher(BaseWatcherView): - def get(self, request, *args, **kwargs): - decision = self.get_object() - user = self.get_user() - notification.stop_observing(decision, user) - return HttpResponseRedirect(request.GET['next'])