diff --git a/CHANGELOG.md b/CHANGELOG.md index d6a9f660..aaea5e9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +master +======== + +v0.10.0 +======== + * Upgrade to Django REST framework 3 + * `sections` field in Note API resource is now a writeable multi-valued + field. This is in contrast to the previous approach, where note sections + were treated like separate, individual resources independent of the note. + * `related_topics` fields in API resources should now be links, not names + v0.9.0 ======= * Upgrade to Django 1.7 diff --git a/editorsnotes/api/serializers/notes.py b/editorsnotes/api/serializers/notes.py index c781296f..41823d07 100644 --- a/editorsnotes/api/serializers/notes.py +++ b/editorsnotes/api/serializers/notes.py @@ -1,9 +1,9 @@ -from lxml import etree +from lxml import etree, html from rest_framework import serializers from editorsnotes.main.models import (Note, TextNS, CitationNS, NoteReferenceNS, - Document) + Document, NoteSection) from editorsnotes.main.models.notes import NOTE_STATUS_CHOICES from .base import (RelatedTopicSerializerMixin, CurrentProjectDefault, @@ -17,18 +17,17 @@ class TextNSSerializer(serializers.ModelSerializer): section_type = serializers.ReadOnlyField(source='section_type_label') class Meta: model = TextNS - fields = ('section_id', 'section_type', 'ordering', 'content',) + fields = ('section_id', 'section_type', 'content',) class CitationNSSerializer(serializers.ModelSerializer): section_id = serializers.ReadOnlyField(source='note_section_id') - #note_id = serializers.ReadOnlyField(source='note_id') section_type = serializers.ReadOnlyField(source='section_type_label') document = HyperlinkedProjectItemField(view_name='api:api-documents-detail', queryset=Document.objects.all()) document_description = serializers.SerializerMethodField() class Meta: model = CitationNS - fields = ('section_id', 'section_type', 'ordering', + fields = ('section_id', 'section_type', 'document', 'document_description', 'content',) def get_document_description(self, obj): return etree.tostring(obj.document.description) @@ -41,7 +40,7 @@ class NoteReferenceNSSerializer(serializers.ModelSerializer): note_reference_title = serializers.SerializerMethodField() class Meta: model = NoteReferenceNS - fields = ('section_id', 'section_type', 'ordering', + fields = ('section_id', 'section_type', 'note_reference', 'note_reference_title', 'content',) def get_note_reference_title(self, obj): return obj.note_reference.title @@ -59,17 +58,24 @@ def _serializer_from_section_type(section_type): return serializer class NoteSectionField(serializers.RelatedField): - def get_attribute(self, obj): - return obj.sections.all().select_subclasses()\ - .select_related('citationns__document__project', - 'notereferencens__note__project') - def to_representation(self, value): - return [self._serialize_section(section) for section in value] - def _serialize_section(self, section): - serializer_class = _serializer_from_section_type( - section.section_type_label) + def __init__(self, *args, **kwargs): + kwargs['queryset'] = NoteSection.objects.all() + super(NoteSectionField, self).__init__(*args, **kwargs) + def to_representation(self, section): + serializer_class = _serializer_from_section_type(section.section_type_label) serializer = serializer_class(section, context=self.context) return serializer.data + def to_internal_value(self, data): + section_type = data['section_type'] + serializer_class = _serializer_from_section_type(section_type) + serializer = serializer_class(data=data, context={ + 'request': self.context['request'] + }) + if serializer.is_valid(): + if 'section_id' in data: + serializer.validated_data['section_id'] = data['section_id'] + serializer.validated_data['section_type'] = section_type + return serializer.validated_data class NoteStatusField(serializers.ReadOnlyField): def get_attribute(self, obj): @@ -89,7 +95,7 @@ class NoteSerializer(RelatedTopicSerializerMixin, updaters = UpdatersField() status = NoteStatusField() related_topics = TopicAssignmentField() - sections = NoteSectionField(read_only=True) + sections = NoteSectionField(many=True, source='get_sections_with_subclasses') class Meta: model = Note fields = ('id', 'title', 'url', 'project', 'is_private', 'last_updated', @@ -97,17 +103,79 @@ class Meta: validators = [ UniqueToProjectValidator('title') ] + # TODO Make sure all section IDs are valid? + def _create_note_section(self, note, data): + section_type = data.pop('section_type') + section_klass = _serializer_from_section_type(section_type).Meta.model + section = section_klass.objects.create( + note=note, + creator=self.context['request'].user, + last_updater=self.context['request'].user, + **data) + return section + def create(self, validated_data): + sections_data = validated_data.pop('get_sections_with_subclasses') + note = super(NoteSerializer, self).create(validated_data) + for idx, section_data in enumerate(sections_data, 1): + section_data['ordering'] = idx + self._create_note_section(note, section_data) + return note + def update(self, instance, validated_data): + sections_data = validated_data.pop('get_sections_with_subclasses') + note = super(NoteSerializer, self).update(instance, validated_data) -class MinimalNoteSerializer(RelatedTopicSerializerMixin, - serializers.ModelSerializer): - status = NoteStatusField() - url = URLField() - project = ProjectSlugField(default=CurrentProjectDefault()) - related_topics = TopicAssignmentField() - class Meta: - model = Note - fields = ('id', 'title', 'url', 'project', 'related_topics', 'content', - 'status', 'is_private',) - validators = [ - UniqueToProjectValidator('title') - ] + # Maybe do this over? It's not perty. + # Go through every section in the update and save an instance if + # necessary. + existing_sections = note.get_sections_with_subclasses() + existing_sections_by_id = { + section.note_section_id: section + for section in existing_sections + } + + existing_order = tuple(ns.id for ns in existing_sections) + new_order = [] + in_update = [] + + for section in sections_data: + + section_id = section.pop('section_id', None) + if section_id is None: + # New section; create it and add it to the note + new_section = self._create_note_section(note, section) + new_order.append(new_section.id) + continue + + del section['section_type'] + + # TODO: Make sure no changing of section types + existing_section = existing_sections_by_id[section_id] + in_update.append(section_id) + new_order.append(existing_section.id) + changed = False + + for field, value in section.items(): + old_value = getattr(existing_section, field) + setattr(existing_section, field, value) + if changed: continue + + if isinstance(value, html.HtmlElement): + changed = etree.tostring(value) != etree.tostring(old_value) + else: + changed = value != old_value + + if changed: + existing_section.last_updater = self.context['request'].user + existing_section.save() + + # Delete sections no longer in the note + to_delete = (section for section in existing_sections + if section.note_section_id not in in_update) + for section in to_delete: + section.delete() + + if len(new_order) and existing_order != tuple(new_order): + positions_dict = {v: k for k, v in enumerate(new_order)} + note.sections.bulk_update_order('ordering', positions_dict) + + return note diff --git a/editorsnotes/api/tests.py b/editorsnotes/api/tests.py index 6931824e..f9a44f4f 100644 --- a/editorsnotes/api/tests.py +++ b/editorsnotes/api/tests.py @@ -73,7 +73,8 @@ def create_test_document(**kwargs): 'title': u'Is testing good?', 'related_topics': [], 'content': u'

We need to figure out if it\'s worth it to write tests.

', - 'status': 'open' + 'status': 'open', + 'sections': [] } def create_test_note(**kwargs): data = TEST_NOTE.copy() @@ -797,180 +798,63 @@ def make_section_data(self): description="New document", project=self.project, creator=self.user, last_updater=self.user) - return { - 'text': { + return [ + { 'section_type': 'text', 'content': 'this is the start' }, - 'citation': { + { 'section_type': 'citation', 'document': '/api' + document_obj.get_absolute_url(), 'content': 'A fascinating article.' }, - 'note_reference': { + { 'section_type': 'note_reference', 'note_reference': '/api' + another_note_obj.get_absolute_url(), 'content': 'See also this note.' } - } + ] - def test_note_api_create_note_section(self): - "Adding new note sections to a note in your own project is ok" - note_obj = self.create_test_note() - test_section_data = self.make_section_data() + def test_note_api_create_note_sections(self): + "Create a test note with multiple sections" + # First create a note with multiple sections + data = TEST_NOTE.copy() + data.update({ 'sections': self.make_section_data() }) response = self.client.post( - reverse('api:api-notes-detail', args=[self.project.slug, note_obj.id]), - json.dumps(test_section_data['text']), + reverse('api:api-notes-list', args=[self.project.slug]), + json.dumps(data), content_type='application/json' ) self.assertEqual(response.status_code, 201) self.assertEqual(Revision.objects.count(), 1) + self.assertEqual(len(response.data['sections']), 3) - response = self.client.post( - reverse('api:api-notes-detail', args=[self.project.slug, note_obj.id]), - json.dumps(test_section_data['citation']), - content_type='application/json' - ) - self.assertEqual(response.status_code, 201) - self.assertEqual(Revision.objects.count(), 2) - - response = self.client.post( - reverse('api:api-notes-detail', args=[self.project.slug, note_obj.id]), - json.dumps(test_section_data['note_reference']), - content_type='application/json' - ) - self.assertEqual(response.status_code, 201) - self.assertEqual(Revision.objects.count(), 3) - - response = self.client.post( - reverse('api:api-notes-normalize-section-order', - args=[self.project.slug, note_obj.id]), - json.dumps({}), - content_type='application/json' - ) - self.assertEqual(tuple(note_obj.sections.values_list('ordering', flat=True)), - (100, 200, 300)) - - def test_note_api_create_note_section_bad_permissions(self): - "Adding a new note section in an outside project is NOT OK" - note_obj = self.create_test_note() - - self.client.logout() - self.client.login(username='esther', password='esther') - - response = self.client.post( - reverse('api:api-notes-detail', args=[self.project.slug, note_obj.id]), - json.dumps({ 'does it even matter?': 'no' }), - content_type='application/json' - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], BAD_PERMISSION_MESSAGE) + # Update one of the sections + data.update({ 'sections': response.data['sections'] }) + data['sections'][0]['content'] = 'This is an updated section' - def test_note_api_create_note_section_logged_out(self): - "Adding a new note section while logged out is NOT OK" - note_obj = self.create_test_note() - - self.client.logout() - - response = self.client.post( - reverse('api:api-notes-detail', args=[self.project.slug, note_obj.id]), - json.dumps({ 'does it even matter?': 'no' }), - content_type='application/json' - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], NO_AUTHENTICATION_MESSAGE) - - - def test_note_api_update_note_section(self): - "Updating a note section in your own project is ok" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) response = self.client.put( - note_section_url, - json.dumps({ 'section_type': 'text', 'content': 'different content' }), + reverse('api:api-notes-detail', args=[self.project.slug, + response.data['id']]), + json.dumps(data), content_type='application/json' ) self.assertEqual(response.status_code, 200) - self.assertEqual(Revision.objects.count(), 1) - - def test_note_api_update_note_section_bad_permissions(self): - "Updating a note section in an outside project is NOT OK" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) - self.client.logout() - self.client.login(username='esther', password='esther') - response = self.client.put( - note_section_url, - json.dumps({ 'section_type': 'text', 'content': 'different content' }), - content_type='application/json' - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], BAD_PERMISSION_MESSAGE) + self.assertEqual(Revision.objects.count(), 2) + self.assertEqual(len(response.data['sections']), 3) + self.assertEqual(response.data['sections'][0]['content'], + '
This is an updated section
') - def test_note_api_update_note_section_logged_out(self): - "Updating a note section while logged out is NOT OK" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) - self.client.logout() + # Delete all the sections + data.update({ 'sections': [] }) response = self.client.put( - note_section_url, - json.dumps({ 'section_type': 'text', 'content': 'different content' }), - content_type='application/json' - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], NO_AUTHENTICATION_MESSAGE) - - def test_note_api_delete_note_section(self): - "Deleting a note section in your own project is ok" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) - response = self.client.delete( - note_section_url, - content_type='application/json' - ) - self.assertEqual(response.status_code, 204) - self.assertEqual(note_obj.sections.count(), 0) - - def test_note_api_delete_note_section_bad_permissions(self): - "Deleting a note section in an outside project is NOT OK" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) - self.client.logout() - self.client.login(username='esther', password='esther') - response = self.client.delete( - note_section_url, - content_type='application/json' - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], BAD_PERMISSION_MESSAGE) - - def test_note_api_delete_note_section_logged_out(self): - "Deleting a note section while logged out is NOT OK" - note_obj = self.create_test_note_with_section() - note_section_url = reverse('api:api-notes-section-detail', - args=[self.project.slug, - note_obj.id, - note_obj.sections.get().note_section_id]) - self.client.logout() - response = self.client.delete( - note_section_url, + reverse('api:api-notes-detail', args=[self.project.slug, + response.data['id']]), + json.dumps(data), content_type='application/json' ) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.data['detail'], NO_AUTHENTICATION_MESSAGE) + self.assertEqual(response.status_code, 200) + self.assertEqual(Revision.objects.count(), 3) + self.assertEqual(len(response.data['sections']), 0) + self.assertEqual(main_models.NoteSection.objects.count(), 0) diff --git a/editorsnotes/api/urls.py b/editorsnotes/api/urls.py index 3626c9ad..46c6a82f 100644 --- a/editorsnotes/api/urls.py +++ b/editorsnotes/api/urls.py @@ -20,8 +20,6 @@ url(r'^notes/$', views.NoteList.as_view(), name='api-notes-list'), url(r'^notes/(?P\d+)/$', views.NoteDetail.as_view(), name='api-notes-detail'), url(r'^notes/(?P\d+)/confirm_delete/$', views.NoteConfirmDelete.as_view(), name='api-notes-confirm-delete'), - url(r'^notes/(?P\d+)/s(?P\d+)/$', views.NoteSectionDetail.as_view(), name='api-notes-section-detail'), - url(r'^notes/(?P\d+)/normalize_section_order/$', views.NormalizeSectionOrder.as_view(), name='api-notes-normalize-section-order'), ### Documents ### url(r'^documents/$', views.DocumentList.as_view(), name='api-documents-list'), diff --git a/editorsnotes/api/views/notes.py b/editorsnotes/api/views/notes.py index 3bea1f74..4ab2f8a8 100644 --- a/editorsnotes/api/views/notes.py +++ b/editorsnotes/api/views/notes.py @@ -1,49 +1,16 @@ -from django.http import Http404 -from django.shortcuts import get_object_or_404 -from rest_framework import status -from rest_framework.parsers import JSONParser -from rest_framework.response import Response -from rest_framework.views import APIView -import reversion - -from editorsnotes.main.models import Note, NoteSection -from editorsnotes.main.models.auth import RevisionProject +from editorsnotes.main.models import Note from .base import (BaseListAPIView, BaseDetailView, DeleteConfirmAPIView, - ElasticSearchListMixin, ProjectSpecificMixin) -from ..permissions import ProjectSpecificPermissions -from ..serializers.notes import ( - MinimalNoteSerializer, NoteSerializer, _serializer_from_section_type) - -__all__ = ['NoteList', 'NoteDetail', 'NoteSectionDetail', - 'NormalizeSectionOrder', 'NoteConfirmDelete'] + ElasticSearchListMixin) +from ..serializers.notes import NoteSerializer -class NormalizeSectionOrder(ProjectSpecificMixin, APIView): - parser_classes = (JSONParser,) - permission_classes = (ProjectSpecificPermissions,) - permissions = { - 'POST': ('main.change_note',) - } - def get_object(self): - qs = Note.objects\ - .filter(project__id=self.request.project.id, - id=self.kwargs.get('note_id'))\ - .select_related('sections') - return get_object_or_404(qs) - def post(self, request, *args, **kwargs): - note = self.get_object() - self.check_object_permissions(self.request, note) - step = int(request.GET.get('step', 100)) - note.sections.normalize_ordering_values('ordering', step=step, fill_in_empty=True) +__all__ = ['NoteList', 'NoteDetail', 'NoteConfirmDelete'] - return Response([ - { 'section_id': _id, 'ordering': ordering } - for _id, ordering in note.sections.values_list('note_section_id', 'ordering') - ]) class NoteList(ElasticSearchListMixin, BaseListAPIView): queryset = Note.objects.all() - serializer_class = MinimalNoteSerializer + serializer_class = NoteSerializer + class NoteDetail(BaseDetailView): queryset = Note.objects.all() @@ -52,55 +19,7 @@ class NoteDetail(BaseDetailView): 'GET': ('main.view_private_note',), 'HEAD': ('main.view_private_note',), } - def post(self, request, *args, **kwargs): - """Add a new note section""" - section_type = request.DATA.get('section_type', None) - if section_type is None: - raise Exception('need a section type') - sec_serializer = _serializer_from_section_type(section_type) - serializer = sec_serializer( - data=request.DATA, context={ - 'request': request, - 'create_revision': True - }) - if serializer.is_valid(): - with reversion.create_revision(): - serializer.save(note=self.get_object(), - creator=request.user, - last_updater=request.user) - reversion.set_user(request.user) - reversion.add_meta(RevisionProject, project=request.project) - return Response(serializer.data, status=status.HTTP_201_CREATED) - # FIXME: Both of these should uses the JSONRenderer instead of directly - # returning serializer.data - return Response(serializer.data, status=status.HTTP_400_BAD_REQUEST) - -class NoteSectionDetail(BaseDetailView): - queryset = NoteSection.objects.all() - permissions = { - 'GET': ('main.view_private_note',), - 'HEAD': ('main.view_private_note',), - } - def get_object(self, queryset=None): - queryset = self.get_queryset() - obj = queryset.get() - self.check_object_permissions(self.request, obj) - return obj - def get_queryset(self): - note_id = self.kwargs.get('note_id') - section_id = self.kwargs.get('section_id') - note = Note.objects.get(id=note_id) - qs = note.sections.all()\ - .select_subclasses()\ - .filter(note_section_id=section_id) - if qs.count() != 1: - raise Http404() - self.model = qs[0].__class__ - return qs - def get_serializer_class(self): - section_type = getattr(self.get_object(), 'section_type_label') - return _serializer_from_section_type(section_type) class NoteConfirmDelete(DeleteConfirmAPIView): queryset = Note.objects.all() diff --git a/editorsnotes/main/models/notes.py b/editorsnotes/main/models/notes.py index 4469cce6..aeabb011 100644 --- a/editorsnotes/main/models/notes.py +++ b/editorsnotes/main/models/notes.py @@ -55,6 +55,9 @@ def get_affiliation(self): def has_topic(self, project_topic): return project_topic.id in \ self.related_topics.values_list('topic_id', flat=True) + def get_sections_with_subclasses(self): + # TODO also select_related + return self.sections.select_subclasses() class NoteSectionManager(InheritanceManagerMixin, OrderingManager): pass