From 497303748646caa286aa09098d0a6cadd7194f87 Mon Sep 17 00:00:00 2001 From: Ramon Wenger Date: Fri, 8 Apr 2022 17:32:10 +0200 Subject: [PATCH] Update error handling in mutation for school class creation --- .../src/graphql/gql/mutations/createClass.gql | 13 +++- server/api/schema.py | 3 +- server/api/schema_public.py | 2 +- server/basicknowledge/queries.py | 2 + server/books/models/module.py | 1 + server/core/tests/base_test.py | 2 - server/schema.graphql | 27 ++++--- server/users/schema/__init__.py | 4 + server/users/{ => schema}/mutations.py | 29 +++----- server/users/{ => schema}/mutations_public.py | 0 server/users/schema/queries.py | 43 +++++++++++ server/users/{schema.py => schema/types.py} | 73 ++++++++----------- server/users/tests/test_school_classes.py | 34 +++++++-- 13 files changed, 148 insertions(+), 85 deletions(-) create mode 100644 server/users/schema/__init__.py rename server/users/{ => schema}/mutations.py (93%) rename server/users/{ => schema}/mutations_public.py (100%) create mode 100644 server/users/schema/queries.py rename server/users/{schema.py => schema/types.py} (83%) diff --git a/client/src/graphql/gql/mutations/createClass.gql b/client/src/graphql/gql/mutations/createClass.gql index 76619626..64794ab4 100644 --- a/client/src/graphql/gql/mutations/createClass.gql +++ b/client/src/graphql/gql/mutations/createClass.gql @@ -1,9 +1,14 @@ mutation CreateSchoolClass($input: CreateSchoolClassInput!) { createSchoolClass(input: $input) { - success - schoolClass { - id - name + result { + __typename + ...on SchoolClassNode { + id + name + } + ...on DuplicateName { + reason + } } } } diff --git a/server/api/schema.py b/server/api/schema.py index 97987e8e..3cc57896 100644 --- a/server/api/schema.py +++ b/server/api/schema.py @@ -22,8 +22,7 @@ from surveys.schema import SurveysQuery from surveys.mutations import SurveyMutations from rooms.mutations import RoomMutations from rooms.schema import RoomsQuery, ModuleRoomsQuery -from users.schema import AllUsersQuery, UsersQuery -from users.mutations import ProfileMutations +from users.schema import AllUsersQuery, UsersQuery, ProfileMutations class Query(UsersQuery, AllUsersQuery, ModuleRoomsQuery, RoomsQuery, ObjectivesQuery, BookQuery, AssignmentsQuery, diff --git a/server/api/schema_public.py b/server/api/schema_public.py index 70b6239f..afdad998 100644 --- a/server/api/schema_public.py +++ b/server/api/schema_public.py @@ -2,7 +2,7 @@ import graphene from django.conf import settings from graphene_django.debug import DjangoDebug -from users.mutations_public import UserMutations +from users.schema import UserMutations class PublicMutation(UserMutations, graphene.ObjectType): diff --git a/server/basicknowledge/queries.py b/server/basicknowledge/queries.py index 5e971669..2a3e4f6f 100644 --- a/server/basicknowledge/queries.py +++ b/server/basicknowledge/queries.py @@ -2,6 +2,7 @@ import graphene from graphene import relay from graphene_django import DjangoObjectType +from api.graphene_wagtail import GenericStreamFieldType from api.utils import get_object from notes.models import InstrumentBookmark from notes.schema import InstrumentBookmarkNode @@ -25,6 +26,7 @@ class InstrumentTypeNode(DjangoObjectType): class InstrumentNode(DjangoObjectType): bookmarks = graphene.List(InstrumentBookmarkNode) type = graphene.Field(InstrumentTypeNode) + contents = GenericStreamFieldType() class Meta: model = BasicKnowledge diff --git a/server/books/models/module.py b/server/books/models/module.py index c58456e1..29e9c1e8 100644 --- a/server/books/models/module.py +++ b/server/books/models/module.py @@ -52,6 +52,7 @@ class Module(StrictHierarchyPage): parent_page_types = ['books.Topic'] subpage_types = ['books.Chapter'] + #todo: isn't this a duplicate definition? def get_child_ids(self): return self.get_children().values_list('id', flat=True) diff --git a/server/core/tests/base_test.py b/server/core/tests/base_test.py index 2532be57..0f349f81 100644 --- a/server/core/tests/base_test.py +++ b/server/core/tests/base_test.py @@ -29,5 +29,3 @@ class SkillboxTestCase(TestCase): user = self.teacher request.user = user return GQLClient(schema=schema, context_value=request) - - diff --git a/server/schema.graphql b/server/schema.graphql index ed66a2c0..61efe16d 100644 --- a/server/schema.graphql +++ b/server/schema.graphql @@ -181,9 +181,9 @@ input AssignmentInput { } type AssignmentNode implements Node { + id: ID! created: DateTime! modified: DateTime! - id: ID! title: String! assignment: String! solution: String @@ -197,9 +197,9 @@ type AssignmentNode implements Node { } type ChapterBookmarkNode implements Node { + id: ID! user: PrivateUserNode! note: NoteNode - id: ID! chapter: ChapterNode! } @@ -343,11 +343,12 @@ input CreateSchoolClassInput { } type CreateSchoolClassPayload { - success: Boolean - schoolClass: SchoolClassNode + result: CreateSchoolClassResult clientMutationId: String } +union CreateSchoolClassResult = SchoolClassNode | DuplicateName + input CreateSnapshotInput { module: String! selectedClass: ID! @@ -463,6 +464,10 @@ type DjangoDebugSQL { encoding: String } +type DuplicateName { + reason: String +} + type FieldError { code: String } @@ -482,9 +487,9 @@ enum InputTypes { } type InstrumentBookmarkNode implements Node { + id: ID! user: PrivateUserNode! note: NoteNode - id: ID! uuid: UUID instrument: InstrumentNode! } @@ -555,9 +560,9 @@ type Logout { } type ModuleBookmarkNode { + id: ID! user: PrivateUserNode! note: NoteNode - id: ID! module: ModuleNode! } @@ -869,10 +874,10 @@ type Query { } type RoomEntryNode implements Node { + id: ID! title: String! description: String slug: String! - id: ID! room: RoomNode! author: PublicUserNode contents: GenericStreamFieldType @@ -891,10 +896,10 @@ type RoomEntryNodeEdge { } type RoomNode implements Node { + id: ID! title: String! description: String slug: String! - id: ID! schoolClass: SchoolClassNode! appearance: String! userCreated: Boolean! @@ -1017,9 +1022,9 @@ type SpellCheckStepNode { } type StudentSubmissionNode implements Node { + id: ID! created: DateTime! modified: DateTime! - id: ID! text: String! document: String! assignment: AssignmentNode! @@ -1088,10 +1093,10 @@ type SyncModuleVisibilityPayload { } type TeamNode implements Node { - name: String! - code: String id: ID! + name: String! isDeleted: Boolean! + code: String creator: PrivateUserNode members: [PublicUserNode] pk: Int diff --git a/server/users/schema/__init__.py b/server/users/schema/__init__.py new file mode 100644 index 00000000..77af8a53 --- /dev/null +++ b/server/users/schema/__init__.py @@ -0,0 +1,4 @@ +from .queries import * +from .types import * +from .mutations import * +from .mutations_public import * diff --git a/server/users/mutations.py b/server/users/schema/mutations.py similarity index 93% rename from server/users/mutations.py rename to server/users/schema/mutations.py index 433d8548..48b65b18 100644 --- a/server/users/mutations.py +++ b/server/users/schema/mutations.py @@ -1,6 +1,7 @@ import graphene from django.contrib.auth import update_session_auth_hash from django.core.exceptions import PermissionDenied +from django.db import IntegrityError from django.db.models import Q from graphene import relay from graphql_relay import from_global_id @@ -9,8 +10,9 @@ from api.utils import get_object from core.logger import get_logger from users.inputs import PasswordUpdateInput from users.models import SchoolClass, SchoolClassMember, Team -from users.schema import SchoolClassNode, TeamNode -from users.serializers import PasswordSerialzer, AvatarUrlSerializer +from users.schema import SchoolClassNode, TeamNode, UpdateError, FieldError, CreateSchoolClassResult +from users.serializers import AvatarUrlSerializer, PasswordSerialzer + logger = get_logger(__name__) @@ -19,15 +21,6 @@ class CodeNotFoundException(Exception): pass -class FieldError(graphene.ObjectType): - code = graphene.String() - - -class UpdateError(graphene.ObjectType): - field = graphene.String() - errors = graphene.List(FieldError) - - class TeacherOnlyMutation(relay.ClientIDMutation): class Meta: abstract = True @@ -204,18 +197,20 @@ class CreateSchoolClass(TeacherOnlyMutation): class Input: name = graphene.String() - success = graphene.Boolean() - school_class = graphene.Field(SchoolClassNode) + result = CreateSchoolClassResult() @classmethod def mutate_and_get_payload(cls, root, info, **kwargs): name = kwargs.get('name') user = info.context.user - school_class = SchoolClass.objects.create(name=name) - SchoolClassMember.objects.create(school_class=school_class, user=user) - user.set_selected_class(school_class) - return cls(success=True, school_class=school_class) + try: + school_class = SchoolClass.objects.create(name=name) + SchoolClassMember.objects.create(school_class=school_class, user=user) + user.set_selected_class(school_class) + return cls(result=school_class) + except IntegrityError: + return cls(result={"reason": "Name wird bereits verwendet"}) class CreateTeam(TeacherOnlyMutation): diff --git a/server/users/mutations_public.py b/server/users/schema/mutations_public.py similarity index 100% rename from server/users/mutations_public.py rename to server/users/schema/mutations_public.py diff --git a/server/users/schema/queries.py b/server/users/schema/queries.py new file mode 100644 index 00000000..cf0e44a0 --- /dev/null +++ b/server/users/schema/queries.py @@ -0,0 +1,43 @@ +import graphene +from graphene_django.filter import DjangoFilterConnectionField + +from basicknowledge.models import BasicKnowledge +from basicknowledge.queries import InstrumentNode +from books.models import Module +from books.schema.queries import ModuleNode +from users.models import User + +from .types import PrivateUserNode + + +class UsersQuery(object): + me = graphene.Field(PrivateUserNode) + all_users = DjangoFilterConnectionField(PrivateUserNode) + my_activity = DjangoFilterConnectionField(ModuleNode) + my_instrument_activity = DjangoFilterConnectionField(InstrumentNode) + + def resolve_me(self, info, **kwargs): + return info.context.user + + def resolve_all_users(self, info, **kwargs): + if not info.context.user.is_superuser: + return User.objects.none() + else: + return User.objects.all() + + def resolve_my_activity(self, info, **kwargs): + return Module.objects.all() + + def resolve_my_instrument_activity(self, info, **kwargs): + return BasicKnowledge.objects.all() + + +class AllUsersQuery(object): + me = graphene.Field(PrivateUserNode) + all_users = DjangoFilterConnectionField(PrivateUserNode) + + def resolve_all_users(self, info, **kwargs): + if not info.context.user.is_superuser: + return User.objects.none() + else: + return User.objects.all() diff --git a/server/users/schema.py b/server/users/schema/types.py similarity index 83% rename from server/users/schema.py rename to server/users/schema/types.py index af467712..4029b53e 100644 --- a/server/users/schema.py +++ b/server/users/schema/types.py @@ -9,13 +9,23 @@ from graphene_django import DjangoObjectType from graphene_django.filter import DjangoFilterConnectionField from graphql_relay import to_global_id -from basicknowledge.models import BasicKnowledge -from basicknowledge.queries import InstrumentNode from books.models import Module from books.schema.queries import ModuleNode from users.models import SchoolClass, SchoolClassMember, Team, User +class RecentModuleFilter(FilterSet): + class Meta: + model = Module + fields = ('recent_modules',) + + order_by = OrderingFilter( + fields=( + ('recent_modules__visited', 'visited'), + ) + ) + + class SchoolClassNode(DjangoObjectType): pk = graphene.Int() members = graphene.List('users.schema.ClassMemberNode') @@ -64,18 +74,6 @@ class TeamNode(DjangoObjectType): return self.members.all() -class RecentModuleFilter(FilterSet): - class Meta: - model = Module - fields = ('recent_modules',) - - order_by = OrderingFilter( - fields=( - ('recent_modules__visited', 'visited'), - ) - ) - - class PublicUserNode(DjangoObjectType): full_name = graphene.String(required=True) is_me = graphene.Boolean() @@ -184,34 +182,25 @@ class ClassMemberNode(ObjectType): return info.context.user.pk == parent.user.pk -class UsersQuery(object): - me = graphene.Field(PrivateUserNode) - all_users = DjangoFilterConnectionField(PrivateUserNode) - my_activity = DjangoFilterConnectionField(ModuleNode) - my_instrument_activity = DjangoFilterConnectionField(InstrumentNode) - - def resolve_me(self, info, **kwargs): - return info.context.user - - def resolve_all_users(self, info, **kwargs): - if not info.context.user.is_superuser: - return User.objects.none() - else: - return User.objects.all() - - def resolve_my_activity(self, info, **kwargs): - return Module.objects.all() - - def resolve_my_instrument_activity(self, info, **kwargs): - return BasicKnowledge.objects.all() +class DuplicateName(graphene.ObjectType): + reason = graphene.String() -class AllUsersQuery(object): - me = graphene.Field(PrivateUserNode) - all_users = DjangoFilterConnectionField(PrivateUserNode) +class CreateSchoolClassResult(graphene.Union): + class Meta: + types = (SchoolClassNode, DuplicateName) - def resolve_all_users(self, info, **kwargs): - if not info.context.user.is_superuser: - return User.objects.none() - else: - return User.objects.all() + @classmethod + def resolve_type(cls, instance, info): + if type(instance).__name__ == "SchoolClass": + return SchoolClassNode + return DuplicateName + + +class FieldError(graphene.ObjectType): + code = graphene.String() + + +class UpdateError(graphene.ObjectType): + field = graphene.String() + errors = graphene.List(FieldError) diff --git a/server/users/tests/test_school_classes.py b/server/users/tests/test_school_classes.py index b604de16..dae6a14a 100644 --- a/server/users/tests/test_school_classes.py +++ b/server/users/tests/test_school_classes.py @@ -1,11 +1,12 @@ -from django.test import TestCase, RequestFactory +from django.db import transaction +from django.test import TestCase from graphene import Context from graphene.test import Client from graphql_relay import to_global_id from api.schema import schema from api.utils import get_graphql_mutation, get_object -from core.factories import UserFactory, TeacherFactory +from core.factories import TeacherFactory, UserFactory from core.tests.base_test import SkillboxTestCase from users.models import SchoolClass, User from users.services import create_users @@ -101,20 +102,41 @@ class ModifySchoolClassTest(SkillboxTestCase): self.assertEqual(SchoolClass.objects.count(), 2) class_name = 'Moordale' mutation = get_graphql_mutation('createClass.gql') - result = self.client.execute(mutation, variables={ + query_result = self.client.execute(mutation, variables={ 'input': { 'name': class_name } }) - self.assertIsNone(result.get('errors')) - id = result.get('data').get('createSchoolClass').get('schoolClass').get('id') + self.assertIsNone(query_result.get('errors')) + result = query_result.get('data').get('createSchoolClass').get('result') + self.assertEqual(result.get('__typename'), 'SchoolClassNode') + id = result.get('id') self.assertEqual(SchoolClass.objects.count(), 3) school_class = get_object(SchoolClass, id) self.assertEqual(school_class.name, class_name) self.assertEqual(school_class.get_teacher(), self.teacher) self.assertEqual(self.teacher.selected_class.name, class_name) - def test_create_school_class_fail(self): + def test_create_school_class_duplicate_name_fail(self): + self.assertEqual(SchoolClass.objects.count(), 2) + class_name = 'skillbox' + mutation = get_graphql_mutation('createClass.gql') + # if we don't do this, django wraps the whole test in an atomic operation, + # and we trigger an exception so the query later in the test would fail + with transaction.atomic(): + query_result = self.client.execute(mutation, variables={ + 'input': { + 'name': class_name + } + }) + self.assertIsNone(query_result.get('errors')) + result = query_result.get('data').get('createSchoolClass').get('result') + self.assertEqual(result.get('__typename'), 'DuplicateName') + reason = result.get('reason') + self.assertEqual(reason, 'Name wird bereits verwendet') + self.assertEqual(SchoolClass.objects.count(), 2) + + def test_create_school_class_fail_permission(self): self.assertEqual(SchoolClass.objects.count(), 2) mutation = get_graphql_mutation('createClass.gql') result = self.student_client.execute(mutation, variables={