From 2da2888d5908f0d8b0274f07693d4050dff103a3 Mon Sep 17 00:00:00 2001 From: Todd Dembrey <todd.dembrey@torchbox.com> Date: Fri, 9 Aug 2019 16:52:26 +0100 Subject: [PATCH] Feature/updates from demo 1 (#1396) * Allow an approver to make changes to the PAF whilst they are approving * Expose the applicant view of the project and allow them to edit Applicants can only edit in the first phase Applicants can only edit the base data on the project * Improve the display of the addressfield and value * Add tests in for the views * De-submissionify the emails to the user * Fix from rebase --- addressfield/fields.py | 4 + .../messages/email/applicant_base.html | 2 +- .../templates/messages/email/comment.html | 2 +- opentech/apply/funds/blocks.py | 9 +- .../applicationsubmission_admin_detail.html | 4 - .../funds/applicationsubmission_detail.html | 5 +- opentech/apply/projects/forms.py | 2 + opentech/apply/projects/models.py | 18 +++ .../includes/supporting_documents.html | 7 +- .../project_applicant_detail.html | 18 +++ .../application_projects/project_detail.html | 28 +--- .../application_projects/project_form.html | 4 +- .../projects/templatetags/approval_tools.py | 5 + opentech/apply/projects/tests/factories.py | 13 +- opentech/apply/projects/tests/test_forms.py | 6 +- opentech/apply/projects/tests/test_views.py | 127 +++++++++++++++++- opentech/apply/projects/views.py | 64 ++++++++- opentech/apply/users/tests/factories.py | 17 ++- 18 files changed, 272 insertions(+), 63 deletions(-) create mode 100644 opentech/apply/projects/templates/application_projects/project_applicant_detail.html diff --git a/addressfield/fields.py b/addressfield/fields.py index 7692b9eb4..f039b7181 100644 --- a/addressfield/fields.py +++ b/addressfield/fields.py @@ -14,6 +14,10 @@ with open(filepath, encoding='utf8') as address_data: VALIDATION_DATA = {country['iso']: country for country in countries} +ADDRESS_FIELDS_ORDER = [ + 'thoroughfare', 'premise', 'localityname', 'administrativearea', 'postalcode', 'country' +] + def flatten_data(data): flattened = dict() diff --git a/opentech/apply/activity/templates/messages/email/applicant_base.html b/opentech/apply/activity/templates/messages/email/applicant_base.html index d88936a30..842c1d005 100644 --- a/opentech/apply/activity/templates/messages/email/applicant_base.html +++ b/opentech/apply/activity/templates/messages/email/applicant_base.html @@ -1,7 +1,7 @@ {% extends "messages/email/base.html" %} {% block salutation %}Dear {{ source.user.get_full_name|default:"applicant" }},{% endblock %} -{% block more_info %}You can access your application here: {{ request.scheme }}://{{ request.get_host }}{{ source.get_absolute_url }} +{% block more_info %}You can find more information here: {{ request.scheme }}://{{ request.get_host }}{{ source.get_absolute_url }} If you have any questions, please submit them here: {{ request.scheme }}://{{ request.get_host }}{{ source.get_absolute_url }}#communications If you have any issues accessing the submission system or other general inquiries, please email us at hello@opentech.fund diff --git a/opentech/apply/activity/templates/messages/email/comment.html b/opentech/apply/activity/templates/messages/email/comment.html index 79c0cf525..e89ca785f 100644 --- a/opentech/apply/activity/templates/messages/email/comment.html +++ b/opentech/apply/activity/templates/messages/email/comment.html @@ -1,5 +1,5 @@ {% extends "messages/email/applicant_base.html" %} -{% block content %}There has been a new comment on your application: {{ source.title }} +{% block content %}There has been a new comment on "{{ source.title }}" {{ comment.user }}: {{ comment.message }}{% endblock %} diff --git a/opentech/apply/funds/blocks.py b/opentech/apply/funds/blocks.py index bec9b723d..c0cbb6e2d 100644 --- a/opentech/apply/funds/blocks.py +++ b/opentech/apply/funds/blocks.py @@ -5,7 +5,7 @@ from django.utils.translation import ugettext_lazy as _ from wagtail.core import blocks -from addressfield.fields import AddressField +from addressfield.fields import AddressField, ADDRESS_FIELDS_ORDER from opentech.apply.categories.blocks import CategoryQuestionBlock from opentech.apply.stream_forms.blocks import FormFieldsBlock from opentech.apply.utils.blocks import ( @@ -69,18 +69,15 @@ class AddressFieldBlock(ApplicationSingleIncludeFieldBlock): # Based on the fields listed in addressfields/widgets.py return ', '.join( data[field] - for field in order_fields + for field in ADDRESS_FIELDS_ORDER if data[field] ) def prepare_data(self, value, data, serialize): - order_fields = [ - 'thoroughfare', 'premise', 'localityname', 'administrativearea', 'postalcode', 'country' - ] data = json.loads(data) data = { field: data[field] - for field in order_fields + for field in ADDRESS_FIELDS_ORDER } if serialize: diff --git a/opentech/apply/funds/templates/funds/applicationsubmission_admin_detail.html b/opentech/apply/funds/templates/funds/applicationsubmission_admin_detail.html index 1f4d62f15..49940120e 100644 --- a/opentech/apply/funds/templates/funds/applicationsubmission_admin_detail.html +++ b/opentech/apply/funds/templates/funds/applicationsubmission_admin_detail.html @@ -36,10 +36,6 @@ </div> {% endblock %} -{% block project %} - {% include 'funds/includes/project_block.html' %} -{% endblock %} - {% block screening_status %} {% include 'funds/includes/screening_status_block.html' %} {% endblock %} diff --git a/opentech/apply/funds/templates/funds/applicationsubmission_detail.html b/opentech/apply/funds/templates/funds/applicationsubmission_detail.html index cbd7575fe..0583233d3 100644 --- a/opentech/apply/funds/templates/funds/applicationsubmission_detail.html +++ b/opentech/apply/funds/templates/funds/applicationsubmission_detail.html @@ -99,9 +99,8 @@ {% endblock %} {% endif %} - {% if request.user.is_apply_staff and object.project %} - {% block project %} - {% endblock %} + {% if object.project %} + {% include 'funds/includes/project_block.html' %} {% endif %} {% block determination %} diff --git a/opentech/apply/projects/forms.py b/opentech/apply/projects/forms.py index c7e51daf6..13d19826f 100644 --- a/opentech/apply/projects/forms.py +++ b/opentech/apply/projects/forms.py @@ -61,6 +61,8 @@ class ProjectEditForm(forms.ModelForm): 'proposed_start': forms.DateInput, } + +class ProjectApprovalForm(ProjectEditForm): def save(self, *args, **kwargs): self.instance.user_has_updated_details = True return super().save(*args, **kwargs) diff --git a/opentech/apply/projects/models.py b/opentech/apply/projects/models.py index 441070ab4..c3fb3e4a6 100644 --- a/opentech/apply/projects/models.py +++ b/opentech/apply/projects/models.py @@ -1,6 +1,8 @@ import collections import decimal +import json +from addressfield.fields import ADDRESS_FIELDS_ORDER from django.conf import settings from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError @@ -100,6 +102,14 @@ class Project(models.Model): def __str__(self): return self.title + def get_address_display(self): + address = json.loads(self.contact_address) + return ', '.join( + address.get(field) + for field in ADDRESS_FIELDS_ORDER + if address.get(field) + ) + @classmethod def create_from_submission(cls, submission): """ @@ -134,6 +144,14 @@ class Project(models.Model): if self.proposed_start > self.proposed_end: raise ValidationError(_('Proposed End Date must be after Proposed Start Date')) + def editable_by(self, user): + if self.editable: + return True + + # Approver can edit it when they are approving + return user.is_approver and self.can_make_approval + + @property def editable(self): # Someone must lead the project to make changes return self.lead and not self.is_locked diff --git a/opentech/apply/projects/templates/application_projects/includes/supporting_documents.html b/opentech/apply/projects/templates/application_projects/includes/supporting_documents.html index e308cdb7e..f0da56dcf 100644 --- a/opentech/apply/projects/templates/application_projects/includes/supporting_documents.html +++ b/opentech/apply/projects/templates/application_projects/includes/supporting_documents.html @@ -1,3 +1,6 @@ +{% load approval_tools %} +{% user_can_edit_project object request.user as editable %} + <div class="docs-block wrapper--outer-space-large"> <div class="docs-block__header"> <h4 class="docs-block__heading">Supporting documents</h4> @@ -23,7 +26,7 @@ <p class="docs-block__title">Approval Form</p> </div> <div class="docs-block__row-inner"> - {% if object.editable %} + {% if editable %} <a class="docs-block__link" href="{% url 'apply:projects:edit' pk=object.pk %}"> {% if object.user_has_updated_details %} Edit @@ -45,7 +48,7 @@ <svg class="icon docs-block__icon"><use xlink:href="#tick"></use></svg> <p class="docs-block__title">Supporting documents</p> </div> - {% if object.editable %} + {% if editable %} <div class="docs-block__row-inner"> <a data-fancybox data-src="#upload-supporting-doc" class="docs-block__link" href="#">Upload new</a> </div> diff --git a/opentech/apply/projects/templates/application_projects/project_applicant_detail.html b/opentech/apply/projects/templates/application_projects/project_applicant_detail.html new file mode 100644 index 000000000..2ca4b4597 --- /dev/null +++ b/opentech/apply/projects/templates/application_projects/project_applicant_detail.html @@ -0,0 +1,18 @@ +{% extends "application_projects/project_detail.html" %} + +{% block notifications %} +{% if not object.editable %} +<div class="wrapper wrapper--sidebar"> + <div class="wrapper--sidebar--inner wrapper--error"> + <div> + <p>Your project is not editable at this point.</p> + {% if not object.lead %} + <p>We are awaiting a lead to be assigned.</p> + {% else %} + <p>It is currently under review by a staff member.</p> + {% endif %} + </div> + </div> +</div> +{% endif %} +{% endblock %} diff --git a/opentech/apply/projects/templates/application_projects/project_detail.html b/opentech/apply/projects/templates/application_projects/project_detail.html index 01d4870ba..59f074e20 100644 --- a/opentech/apply/projects/templates/application_projects/project_detail.html +++ b/opentech/apply/projects/templates/application_projects/project_detail.html @@ -56,26 +56,10 @@ <div class="wrapper wrapper--large wrapper--tabs js-tabs-content"> <div class="tabs__content" id="tab-1"> - + {% block notifications %} + {% endblock %} <div class="wrapper wrapper--sidebar"> <article class="wrapper--sidebar--inner"> - <header class="heading heading--submission-meta heading-text zeta"> - <span>Last edit: <strong>{{ object.updated_at.timestamp.date }} by {{ object.updated_by }}</strong></span> - - {# {% if request.user|has_edit_perm:object %} #} - {% if request.user %} - <a class="link link--edit-submission is-active" href="#"> - Edit - <svg class="icon icon--pen"><use xlink:href="#pen"></use></svg> - </a> - {% else %} - <span class="link link--edit-submission"> - Edit - <svg class="icon icon--padlock"><use xlink:href="#padlock"></use></svg> - </span> - {% endif %} - </header> - <h3>Project Information</h3> <div class="grid grid--proposal-info"> <div> @@ -108,7 +92,7 @@ <div class="rich-text--hidden js-rich-text-hidden"> <div> <h5>Address</h5> - <p>{{ object.contact_address|default:"-" }}</p> + <p>{{ object.get_address_display|default:"-"}}</p> </div> <div> @@ -118,7 +102,7 @@ <div> <h5>Value</h5> - <p>{{ object.value|default:"-" }}</p> + <p>${{ object.value|default:"-" }}</p> </div> </div> @@ -128,9 +112,7 @@ {# {% include "funds/includes/invoice_block.html" %} #} {# </div> #} - {% if request.user.is_apply_staff %} - {% include "application_projects/includes/supporting_documents.html" %} - {% endif %} + {% include "application_projects/includes/supporting_documents.html" %} </article> <aside class="sidebar"> diff --git a/opentech/apply/projects/templates/application_projects/project_form.html b/opentech/apply/projects/templates/application_projects/project_form.html index 4db3df2c7..6834a47d5 100644 --- a/opentech/apply/projects/templates/application_projects/project_form.html +++ b/opentech/apply/projects/templates/application_projects/project_form.html @@ -2,12 +2,12 @@ {% load static %} -{% block title %}Editing: {{ object.name }}{% endblock %} +{% block title %}Editing: {{ object.title }}{% endblock %} {% block content %} <div class="admin-bar"> <div class="admin-bar__inner"> - <h2 class="heading heading--no-margin">Editing: {{ object.name }}</h2> + <h2 class="heading heading--no-margin">Editing: {{ object.title}}</h2> </div> </div> diff --git a/opentech/apply/projects/templatetags/approval_tools.py b/opentech/apply/projects/templatetags/approval_tools.py index 1af889f3e..2c51d1a59 100644 --- a/opentech/apply/projects/templatetags/approval_tools.py +++ b/opentech/apply/projects/templatetags/approval_tools.py @@ -11,3 +11,8 @@ def user_has_approved(project, user): @register.simple_tag def user_can_approve_project(project, user): return user.is_approver and not user_has_approved(project, user) + + +@register.simple_tag +def user_can_edit_project(project, user): + return project.editable_by(user) diff --git a/opentech/apply/projects/tests/factories.py b/opentech/apply/projects/tests/factories.py index efea3ef46..b91c0aadf 100644 --- a/opentech/apply/projects/tests/factories.py +++ b/opentech/apply/projects/tests/factories.py @@ -5,9 +5,13 @@ import factory from django.utils import timezone from opentech.apply.funds.tests.factories import ApplicationSubmissionFactory -from opentech.apply.projects.models import (DocumentCategory, PacketFile, - Project) -from opentech.apply.users.tests.factories import UserFactory +from opentech.apply.projects.models import ( + DocumentCategory, + PacketFile, + Project, +) +from opentech.apply.users.tests.factories import StaffFactory, UserFactory + ADDRESS = { 'country': 'GB', @@ -48,6 +52,7 @@ class ProjectFactory(factory.DjangoModelFactory): user = factory.SubFactory(UserFactory) title = factory.Sequence('name {}'.format) + lead = factory.SubFactory(StaffFactory) contact_legal_name = 'test' contact_email = 'test@example.com' contact_address = json.dumps(ADDRESS) @@ -56,6 +61,8 @@ class ProjectFactory(factory.DjangoModelFactory): proposed_start = factory.LazyFunction(timezone.now) proposed_end = factory.LazyFunction(timezone.now) + is_locked = False + class Meta: model = Project diff --git a/opentech/apply/projects/tests/test_forms.py b/opentech/apply/projects/tests/test_forms.py index ae05b044a..5c0d157d5 100644 --- a/opentech/apply/projects/tests/test_forms.py +++ b/opentech/apply/projects/tests/test_forms.py @@ -1,10 +1,10 @@ from django.test import TestCase -from ..forms import ProjectEditForm +from ..forms import ProjectApprovalForm from .factories import ProjectFactory, address_to_form_data -class TestProjectEditForm(TestCase): +class TestProjectApprovalForm(TestCase): def test_updating_fields_sets_changed_flag(self): project = ProjectFactory() @@ -20,7 +20,7 @@ class TestProjectEditForm(TestCase): 'proposed_end': project.proposed_end, } data.update(address_to_form_data()) - form = ProjectEditForm(instance=project, data=data) + form = ProjectApprovalForm(instance=project, data=data) form.save() self.assertTrue(project.user_has_updated_details) diff --git a/opentech/apply/projects/tests/test_views.py b/opentech/apply/projects/tests/test_views.py index 70fc10611..9c5f69a07 100644 --- a/opentech/apply/projects/tests/test_views.py +++ b/opentech/apply/projects/tests/test_views.py @@ -1,17 +1,24 @@ from io import BytesIO +from django.core.exceptions import PermissionDenied from django.test import RequestFactory, TestCase -from opentech.apply.users.tests.factories import (ReviewerFactory, - StaffFactory, - SuperUserFactory, - UserFactory) +from opentech.apply.users.tests.factories import ( + ApproverFactory, + ReviewerFactory, + StaffFactory, + SuperUserFactory, + UserFactory, +) from opentech.apply.utils.testing.tests import BaseViewTestCase from ..forms import SetPendingForm from ..views import ProjectDetailView -from .factories import (DocumentCategoryFactory, PacketFileFactory, - ProjectFactory) +from .factories import ( + DocumentCategoryFactory, + PacketFileFactory, + ProjectFactory, +) class TestCreateApprovalView(BaseViewTestCase): @@ -69,7 +76,16 @@ class TestProjectDetailView(TestCase): request = self.factory.get('/projects/1/') request.user = UserFactory() - response = ProjectDetailView.as_view()(request, pk=self.project.pk) + with self.assertRaises(PermissionDenied): + ProjectDetailView.as_view()(request, pk=self.project.pk) + + def test_owner_has_access(self): + owner = UserFactory() + request = self.factory.get('/projects/1/') + request.user = owner + project = ProjectFactory(user=owner) + + response = ProjectDetailView.as_view()(request, pk=project.pk) self.assertEqual(response.status_code, 200) @@ -164,3 +180,100 @@ class TestUploadDocumentView(BaseViewTestCase): project.refresh_from_db() self.assertEqual(project.packet_files.count(), 1) + + +class BaseProjectEditTestCase(BaseViewTestCase): + url_name = 'funds:projects:{}' + base_view_name = 'edit' + + def get_kwargs(self, instance): + return {'pk': instance.id} + + +class TestUserProjectEditView(BaseProjectEditTestCase): + user_factory = UserFactory + + def test_does_not_have_access(self): + project = ProjectFactory() + response = self.get_page(project) + + self.assertEqual(response.status_code, 403) + + def test_owner_has_access(self): + project = ProjectFactory(user=self.user) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.redirect_chain, []) + + def test_no_lead_redirects(self): + project = ProjectFactory(user=self.user, lead=None) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertRedirects(response, self.url(project, 'detail')) + + def test_locked_redirects(self): + project = ProjectFactory(user=self.user, is_locked=True) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertRedirects(response, self.url(project, 'detail')) + + +class TestStaffProjectEditView(BaseProjectEditTestCase): + user_factory = StaffFactory + + def test_staff_user_has_access(self): + project = ProjectFactory() + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.redirect_chain, []) + + def test_no_lead_redirects(self): + project = ProjectFactory(user=self.user, lead=None) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertRedirects(response, self.url(project, 'detail')) + + def test_locked_redirects(self): + project = ProjectFactory(user=self.user, is_locked=True) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertRedirects(response, self.url(project, 'detail')) + + +class TestApproverProjectEditView(BaseProjectEditTestCase): + user_factory = ApproverFactory + + def test_approver_has_access_locked(self): + project = ProjectFactory(is_locked=True) + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.redirect_chain, []) + + +class TestSuperProjectEditView(BaseProjectEditTestCase): + user_factory = StaffFactory + + def test_has_access(self): + project = ProjectFactory() + response = self.get_page(project) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.redirect_chain, []) + + +class TestReviewerProjectEditView(BaseProjectEditTestCase): + user_factory = ReviewerFactory + + def test_does_not_have_access(self): + project = ProjectFactory() + response = self.get_page(project) + + self.assertEqual(response.status_code, 403) + self.assertEqual(response.redirect_chain, []) diff --git a/opentech/apply/projects/views.py b/opentech/apply/projects/views.py index a97746f82..150d6ec9e 100644 --- a/opentech/apply/projects/views.py +++ b/opentech/apply/projects/views.py @@ -1,8 +1,11 @@ from copy import copy +from django.contrib import messages +from django.core.exceptions import PermissionDenied from django.db import transaction from django.shortcuts import redirect from django.utils.decorators import method_decorator +from django.utils.translation import ugettext_lazy as _ from django.views.generic import CreateView, DetailView, FormView, UpdateView from opentech.apply.activity.messaging import MESSAGES, messenger @@ -11,10 +14,17 @@ from opentech.apply.users.decorators import staff_required from opentech.apply.utils.views import (DelegateableView, DelegatedViewMixin, ViewDispatcher) -from .forms import (CreateApprovalForm, ProjectEditForm, RejectionForm, - RemoveDocumentForm, SetPendingForm, UpdateProjectLeadForm, - UploadDocumentForm) -from .models import CONTRACTING, Approval, PacketFile, Project +from .forms import ( + CreateApprovalForm, + ProjectApprovalForm, + ProjectEditForm, + RejectionForm, + RemoveDocumentForm, + SetPendingForm, + UpdateProjectLeadForm, + UploadDocumentForm, +) +from .models import CONTRACTING, Approval, Project, PacketFile @method_decorator(staff_required, name='dispatch') @@ -167,8 +177,20 @@ class AdminProjectDetailView(ActivityContextMixin, DelegateableView, DetailView) return context -class ApplicantProjectDetailView(DetailView): +class ApplicantProjectDetailView(ActivityContextMixin, DelegateableView, DetailView): + form_views = [ + CommentFormView, + ] + model = Project + template_name_suffix = '_applicant_detail' + + def dispatch(self, request, *args, **kwargs): + project = self.get_object() + # This view is only for applicants. + if project.user != request.user: + raise PermissionDenied + return super().dispatch(request, *args, **kwargs) class ProjectDetailView(ViewDispatcher): @@ -176,7 +198,35 @@ class ProjectDetailView(ViewDispatcher): applicant_view = ApplicantProjectDetailView -@method_decorator(staff_required, name='dispatch') -class ProjectEditView(UpdateView): +class ProjectApprovalEditView(UpdateView): + form_class = ProjectApprovalForm + model = Project + + def dispatch(self, request, *args, **kwargs): + project = self.get_object() + if not project.editable_by(request.user): + messages.info(self.request, _('You are not allowed to edit the project at this time')) + return redirect(project) + return super().dispatch(request, *args, **kwargs) + + +class ApplicantProjectEditView(UpdateView): form_class = ProjectEditForm model = Project + + def dispatch(self, request, *args, **kwargs): + project = self.get_object() + # This view is only for applicants. + if project.user != request.user: + raise PermissionDenied + + if not project.editable_by(request.user): + messages.info(self.request, _('You are not allowed to edit the project at this time')) + return redirect(project) + + return super().dispatch(request, *args, **kwargs) + + +class ProjectEditView(ViewDispatcher): + admin_view = ProjectApprovalEditView + applicant_view = ApplicantProjectEditView diff --git a/opentech/apply/users/tests/factories.py b/opentech/apply/users/tests/factories.py index 8db74b40f..5d008ce43 100644 --- a/opentech/apply/users/tests/factories.py +++ b/opentech/apply/users/tests/factories.py @@ -3,7 +3,12 @@ from django.contrib.auth.models import Group import factory -from ..groups import APPLICANT_GROUP_NAME, REVIEWER_GROUP_NAME, STAFF_GROUP_NAME +from ..groups import ( + APPLICANT_GROUP_NAME, + APPROVER_GROUP_NAME, + REVIEWER_GROUP_NAME, + STAFF_GROUP_NAME, +) class GroupFactory(factory.DjangoModelFactory): @@ -57,6 +62,16 @@ class StaffFactory(OAuthUserFactory): self.groups.add(GroupFactory(name=STAFF_GROUP_NAME)) +class ApproverFactory(StaffFactory): + @factory.post_generation + def groups(self, create, extracted, **kwargs): + if create: + self.groups.add( + GroupFactory(name=STAFF_GROUP_NAME), + GroupFactory(name=APPROVER_GROUP_NAME), + ) + + class SuperUserFactory(StaffFactory): is_superuser = True -- GitLab