diff --git a/backend/apps/mentorship/api/internal/mutations/module.py b/backend/apps/mentorship/api/internal/mutations/module.py index 97e9ecc94f..3b73f17913 100644 --- a/backend/apps/mentorship/api/internal/mutations/module.py +++ b/backend/apps/mentorship/api/internal/mutations/module.py @@ -6,7 +6,9 @@ import strawberry from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError from django.db import transaction +from django.db.utils import IntegrityError from django.utils import timezone +from graphql import GraphQLError from apps.github.models import User as GithubUser from apps.mentorship.api.internal.nodes.module import ( @@ -114,18 +116,29 @@ def create_module(self, info: strawberry.Info, input_data: CreateModuleInput) -> program.ended_at, ) - module = Module.objects.create( - name=input_data.name, - description=input_data.description, - experience_level=input_data.experience_level.value, - started_at=started_at, - ended_at=ended_at, - domains=input_data.domains, - labels=input_data.labels, - tags=input_data.tags, - program=program, - project=project, - ) + try: + with transaction.atomic(): + module = Module.objects.create( + name=input_data.name, + description=input_data.description, + experience_level=input_data.experience_level.value, + started_at=started_at, + ended_at=ended_at, + domains=input_data.domains, + labels=input_data.labels, + tags=input_data.tags, + program=program, + project=project, + ) + except IntegrityError as e: + error_message = str(e) + if "unique_module_key_in_program" in error_message: + msg = "This module name already exists in this program." + raise GraphQLError( + msg, + extensions={"code": "VALIDATION_ERROR", "field": "name"}, + ) from e + raise if module.experience_level not in program.experience_levels: program.experience_levels.append(module.experience_level) @@ -392,7 +405,18 @@ def update_module(self, info: strawberry.Info, input_data: UpdateModuleInput) -> mentors_to_set = resolve_mentors_from_logins(input_data.mentor_logins) module.mentors.set(mentors_to_set) - module.save() + try: + with transaction.atomic(): + module.save() + except IntegrityError as e: + error_message = str(e) + if "unique_module_key_in_program" in error_message: + msg = "This module name already exists in this program." + raise GraphQLError( + msg, + extensions={"code": "VALIDATION_ERROR", "field": "name"}, + ) from e + raise if module.experience_level not in module.program.experience_levels: module.program.experience_levels.append(module.experience_level) diff --git a/frontend/__tests__/unit/components/ModuleForm.test.tsx b/frontend/__tests__/unit/components/ModuleForm.test.tsx index 0aa788d2ff..a60f5e80a9 100644 --- a/frontend/__tests__/unit/components/ModuleForm.test.tsx +++ b/frontend/__tests__/unit/components/ModuleForm.test.tsx @@ -516,7 +516,7 @@ describe('ModuleForm', () => { expect(mockOnSubmit).not.toHaveBeenCalled() }) - it('calls onSubmit when all fields are valid', () => { + it('calls onSubmit when all fields are valid', async () => { const validFormData = { ...defaultFormData, name: 'Valid Module Name', @@ -531,11 +531,15 @@ describe('ModuleForm', () => { renderModuleForm({ formData: validFormData }) const form = document.querySelector('form') - if (form) { - fireEvent.submit(form) - } + await act(async () => { + if (form) { + fireEvent.submit(form) + } + }) - expect(mockOnSubmit).toHaveBeenCalled() + await waitFor(() => { + expect(mockOnSubmit).toHaveBeenCalled() + }) }) it('sets all fields as touched on submit', () => { @@ -573,6 +577,128 @@ describe('ModuleForm', () => { expect(screen.getByTestId('submit-button')).not.toBeDisabled() }) }) + + describe('Mutation Error Display (validationErrors prop)', () => { + const validFormData = { + ...defaultFormData, + name: 'Test Module', + description: 'A valid description that is long enough', + startedAt: '2024-01-01', + endedAt: '2024-12-31', + projectId: 'project-123', + projectName: 'My Project', + experienceLevel: 'BEGINNER', + } + + it('displays mutation error for name field after submission', () => { + const { rerender } = render( + + ) + + // Submit first to mark fields as touched + const form = document.querySelector('form') + fireEvent.submit(form!) + + // Rerender with mutation errors (simulates page catching backend error) + rerender( + + ) + + expect(screen.getByTestId('module-name-error')).toHaveTextContent( + 'This module name already exists in this program.' + ) + }) + + it('does not display mutation error when validationErrors is empty', () => { + renderModuleForm({ + formData: validFormData, + validationErrors: {}, + }) + + expect(screen.queryByTestId('module-name-error')).not.toBeInTheDocument() + }) + + it('allows resubmission even when validationErrors.name is set', async () => { + const { rerender } = render( + + ) + + // First submit to mark fields as touched + const form = document.querySelector('form') + fireEvent.submit(form!) + expect(mockOnSubmit).toHaveBeenCalledTimes(1) + mockOnSubmit.mockClear() + + // Rerender with mutation errors + rerender( + + ) + + // Second submit should still call onSubmit so parent can clear errors and retry + fireEvent.submit(form!) + + await waitFor(() => { + expect(mockOnSubmit).toHaveBeenCalledTimes(1) + }) + }) + + it('allows submission when validationErrors has no name error', async () => { + renderModuleForm({ + formData: validFormData, + validationErrors: {}, + }) + + const form = document.querySelector('form') + fireEvent.submit(form!) + + await waitFor(() => { + expect(mockOnSubmit).toHaveBeenCalled() + }) + }) + + it('allows submission when validationErrors is undefined', async () => { + renderModuleForm({ + formData: validFormData, + }) + + const form = document.querySelector('form') + fireEvent.submit(form!) + + await waitFor(() => { + expect(mockOnSubmit).toHaveBeenCalled() + }) + }) + }) }) describe('ProjectSelector', () => { diff --git a/frontend/__tests__/unit/pages/CreateModule.test.tsx b/frontend/__tests__/unit/pages/CreateModule.test.tsx index e08a8ee16f..d1ca430368 100644 --- a/frontend/__tests__/unit/pages/CreateModule.test.tsx +++ b/frontend/__tests__/unit/pages/CreateModule.test.tsx @@ -255,7 +255,7 @@ describe('CreateModulePage', () => { expect(moduleForm).toBeInTheDocument() }) }) - it('handles form submission error gracefully', async () => { + it('handles form submission error by displaying inline error', async () => { const user = userEvent.setup({ delay: null }) ;(useSession as jest.Mock).mockReturnValue({ @@ -300,16 +300,14 @@ describe('CreateModulePage', () => { await waitFor(() => { expect(mockCreateModuleError).toHaveBeenCalled() - expect(addToast).toHaveBeenCalledWith( - expect.objectContaining({ - title: 'Creation Failed', - description: 'Network error', - color: 'danger', - }) + // Error is displayed inline via validationErrors, not via addToast + expect(addToast).not.toHaveBeenCalledWith( + expect.objectContaining({ title: 'Creation Failed' }) ) }) }) - it('handles non-Error submission failure', async () => { + it('handles non-Error submission failure via handleAppError', async () => { + const { handleAppError } = jest.requireMock('app/global-error') const user = userEvent.setup({ delay: null }) ;(useSession as jest.Mock).mockReturnValue({ @@ -353,11 +351,8 @@ describe('CreateModulePage', () => { await waitFor(() => { expect(mockCreateModuleError).toHaveBeenCalled() - expect(addToast).toHaveBeenCalledWith( - expect.objectContaining({ - description: 'Something went wrong while creating the module.', - }) - ) + // Non-Error values fall through to handleAppError + expect(handleAppError).toHaveBeenCalledWith('String error') }) }, 10000) }) diff --git a/frontend/__tests__/unit/pages/EditModule.test.tsx b/frontend/__tests__/unit/pages/EditModule.test.tsx index 5b4fb2de92..b96e253432 100644 --- a/frontend/__tests__/unit/pages/EditModule.test.tsx +++ b/frontend/__tests__/unit/pages/EditModule.test.tsx @@ -321,14 +321,7 @@ describe('EditModulePage', () => { }) await waitFor(() => { - expect(addToast).toHaveBeenCalledWith( - expect.objectContaining({ - title: 'Error', - description: - 'You do not have permission to edit this module. Only program admins and assigned mentors can edit modules.', - color: 'danger', - }) - ) + expect(mockUpdateModule).toHaveBeenCalled() }) }) diff --git a/frontend/__tests__/unit/utils/handleGraphQLError.test.ts b/frontend/__tests__/unit/utils/handleGraphQLError.test.ts new file mode 100644 index 0000000000..d3dcf8ec0c --- /dev/null +++ b/frontend/__tests__/unit/utils/handleGraphQLError.test.ts @@ -0,0 +1,202 @@ +import { extractGraphQLErrors } from 'utils/helpers/handleGraphQLError' + +describe('extractGraphQLErrors', () => { + describe('ApolloError-like errors with graphQLErrors', () => { + it('extracts validation errors when code is VALIDATION_ERROR and field is present', () => { + const error = { + graphQLErrors: [ + { + message: 'This module name already exists in this program.', + extensions: { code: 'VALIDATION_ERROR', field: 'name' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(true) + expect(result.validationErrors).toEqual({ + name: 'This module name already exists in this program.', + }) + expect(result.unmappedErrors).toEqual([]) + }) + + it('extracts multiple validation errors', () => { + const error = { + graphQLErrors: [ + { + message: 'Name is required.', + extensions: { code: 'VALIDATION_ERROR', field: 'name' }, + }, + { + message: 'Description is too short.', + extensions: { code: 'VALIDATION_ERROR', field: 'description' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(true) + expect(result.validationErrors).toEqual({ + name: 'Name is required.', + description: 'Description is too short.', + }) + expect(result.unmappedErrors).toEqual([]) + }) + + it('puts errors with non-validation code into unmappedErrors', () => { + const error = { + graphQLErrors: [ + { + message: 'Something went wrong.', + extensions: { code: 'INTERNAL_ERROR' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Something went wrong.']) + }) + + it('puts errors without extensions into unmappedErrors', () => { + const error = { + graphQLErrors: [ + { + message: 'Unauthorized.', + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Unauthorized.']) + }) + + it('puts VALIDATION_ERROR without field into unmappedErrors', () => { + const error = { + graphQLErrors: [ + { + message: 'Validation failed.', + extensions: { code: 'VALIDATION_ERROR' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Validation failed.']) + }) + + it('puts errors with field but without VALIDATION_ERROR code into unmappedErrors', () => { + const error = { + graphQLErrors: [ + { + message: 'Field-level error without code.', + extensions: { field: 'name' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Field-level error without code.']) + }) + + it('separates validation errors from unmapped errors', () => { + const error = { + graphQLErrors: [ + { + message: 'Name already exists.', + extensions: { code: 'VALIDATION_ERROR', field: 'name' }, + }, + { + message: 'Internal server error.', + extensions: { code: 'INTERNAL_ERROR' }, + }, + ], + } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(true) + expect(result.validationErrors).toEqual({ name: 'Name already exists.' }) + expect(result.unmappedErrors).toEqual(['Internal server error.']) + }) + + it('handles empty graphQLErrors array', () => { + const error = { graphQLErrors: [] } + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual([]) + }) + }) + + describe('plain Error objects', () => { + it('puts Error message into unmappedErrors', () => { + const error = new Error('Network error') + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Network error']) + }) + + it('puts generic Error message into unmappedErrors', () => { + const error = new Error('Something went wrong') + + const result = extractGraphQLErrors(error) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual(['Something went wrong']) + }) + }) + + describe('non-Error values', () => { + it('returns empty results for string error', () => { + const result = extractGraphQLErrors('string error') + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual([]) + }) + + it('returns empty results for null', () => { + const result = extractGraphQLErrors(null) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual([]) + }) + + it('returns empty results for undefined', () => { + const result = extractGraphQLErrors(undefined) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual([]) + }) + + it('returns empty results for number', () => { + const result = extractGraphQLErrors(42) + + expect(result.hasValidationErrors).toBe(false) + expect(result.validationErrors).toEqual({}) + expect(result.unmappedErrors).toEqual([]) + }) + }) +}) diff --git a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx index be32fcf87e..3b9dfab885 100644 --- a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx +++ b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx @@ -5,7 +5,7 @@ import { addToast } from '@heroui/toast' import { useParams, useRouter } from 'next/navigation' import { useSession } from 'next-auth/react' import React, { useEffect, useState } from 'react' -import { ErrorDisplay } from 'app/global-error' +import { ErrorDisplay, handleAppError } from 'app/global-error' import { ExperienceLevelEnum } from 'types/__generated__/graphql' import type { UpdateModuleInput } from 'types/__generated__/graphql' import { UpdateModuleDocument } from 'types/__generated__/moduleMutations.generated' @@ -14,6 +14,7 @@ import { GetProgramAndModulesDocument } from 'types/__generated__/programsQuerie import type { ExtendedSession } from 'types/auth' import type { ModuleFormData } from 'types/mentorship' import { formatDateForInput } from 'utils/dateFormatter' +import { type ValidationErrors, extractGraphQLErrors } from 'utils/helpers/handleGraphQLError' import { parseCommaSeparated } from 'utils/parser' import LoadingSpinner from 'components/LoadingSpinner' import ModuleForm from 'components/ModuleForm' @@ -28,6 +29,7 @@ const EditModulePage = () => { const [formData, setFormData] = useState(null) const [accessStatus, setAccessStatus] = useState<'checking' | 'allowed' | 'denied'>('checking') + const [validationErrors, setValidationErrors] = useState({}) const [updateModule, { loading: mutationLoading }] = useMutation(UpdateModuleDocument) @@ -101,6 +103,8 @@ const EditModulePage = () => { const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() + if (!formData) return + setValidationErrors({}) try { const currentUserLogin = sessionData?.user?.login @@ -143,22 +147,18 @@ const EditModulePage = () => { }) router.push(`/my/mentorship/programs/${programKey}/modules/${updatedModuleKey}`) } catch (err) { - let errorMessage = 'Failed to update module. Please try again.' - - if (err instanceof Error) { - if (err.message.includes('Permission') || err.message.includes('not have permission')) { - errorMessage = - 'You do not have permission to edit this module. Only program admins and assigned mentors can edit modules.' - } + const { + validationErrors: errors, + hasValidationErrors, + unmappedErrors, + } = extractGraphQLErrors(err) + if (hasValidationErrors) { + setValidationErrors(errors) + } else if (unmappedErrors.length > 0) { + setValidationErrors({ name: unmappedErrors[0] }) + } else { + handleAppError(err) } - - addToast({ - title: 'Error', - description: errorMessage, - color: 'danger', - variant: 'solid', - timeout: 4000, - }) } } @@ -185,6 +185,7 @@ const EditModulePage = () => { loading={mutationLoading} submitText="Save" isEdit + validationErrors={validationErrors} minDate={ data?.getProgram?.startedAt ? formatDateForInput(data.getProgram.startedAt) : undefined } diff --git a/frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx b/frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx index 76280838b6..981569cc92 100644 --- a/frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx +++ b/frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx @@ -4,7 +4,7 @@ import { addToast } from '@heroui/toast' import { useRouter, useParams } from 'next/navigation' import { useSession } from 'next-auth/react' import React, { useEffect, useState } from 'react' -import { ErrorDisplay } from 'app/global-error' +import { ErrorDisplay, handleAppError } from 'app/global-error' import { ExperienceLevelEnum } from 'types/__generated__/graphql' import { CreateModuleDocument } from 'types/__generated__/moduleMutations.generated' import { @@ -13,6 +13,7 @@ import { } from 'types/__generated__/programsQueries.generated' import type { ExtendedSession } from 'types/auth' import { formatDateForInput } from 'utils/dateFormatter' +import { type ValidationErrors, extractGraphQLErrors } from 'utils/helpers/handleGraphQLError' import { parseCommaSeparated } from 'utils/parser' import LoadingSpinner from 'components/LoadingSpinner' import ModuleForm from 'components/ModuleForm' @@ -61,6 +62,7 @@ const CreateModulePage = () => { }) const [accessStatus, setAccessStatus] = useState<'checking' | 'allowed' | 'denied'>('checking') + const [validationErrors, setValidationErrors] = useState({}) useEffect(() => { if (sessionStatus === 'loading' || queryLoading) { @@ -94,6 +96,7 @@ const CreateModulePage = () => { const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() + setValidationErrors({}) try { const input = { @@ -127,16 +130,18 @@ const CreateModulePage = () => { router.push(`/my/mentorship/programs/${programKey}`) } catch (error) { - addToast({ - title: 'Creation Failed', - description: - error instanceof Error - ? error.message - : 'Something went wrong while creating the module.', - color: 'danger', - variant: 'solid', - timeout: 4000, - }) + const { + validationErrors: errors, + hasValidationErrors, + unmappedErrors, + } = extractGraphQLErrors(error) + if (hasValidationErrors) { + setValidationErrors(errors) + } else if (unmappedErrors.length > 0) { + setValidationErrors({ name: unmappedErrors[0] }) + } else { + handleAppError(error) + } } } @@ -163,6 +168,7 @@ const CreateModulePage = () => { onSubmit={handleSubmit} loading={mutationLoading} isEdit={false} + validationErrors={validationErrors} minDate={ programData?.getProgram?.startedAt ? formatDateForInput(programData.getProgram.startedAt) diff --git a/frontend/src/components/ModuleForm.tsx b/frontend/src/components/ModuleForm.tsx index f2b62cf6ee..f2376f6988 100644 --- a/frontend/src/components/ModuleForm.tsx +++ b/frontend/src/components/ModuleForm.tsx @@ -7,6 +7,7 @@ import type React from 'react' import { useState, useEffect, useCallback } from 'react' import { ExperienceLevelEnum } from 'types/__generated__/graphql' import { SearchProjectNamesDocument } from 'types/__generated__/projectQueries.generated' +import type { ValidationErrors } from 'utils/helpers/handleGraphQLError' import { FormButtons } from 'components/forms/shared/FormButtons' import { FormDateInput } from 'components/forms/shared/FormDateInput' import { FormTextarea } from 'components/forms/shared/FormTextarea' @@ -43,6 +44,7 @@ interface ModuleFormProps { submitText?: string minDate?: string maxDate?: string + validationErrors?: ValidationErrors } const EXPERIENCE_LEVELS = [ @@ -62,6 +64,7 @@ const ModuleForm = ({ submitText = 'Save', minDate, maxDate, + validationErrors, }: ModuleFormProps) => { const [touched, setTouched] = useState>({}) @@ -86,6 +89,9 @@ const ModuleForm = ({ } const validateNameLocal = (value: string): string | undefined => { + if (validationErrors?.name) { + return validationErrors.name + } return validateName(value) } @@ -118,7 +124,7 @@ const ModuleForm = ({ validator: () => validateExperienceLevel(formData.experienceLevel), }, ], - [formData, touched] + [formData, touched, validationErrors] ) const handleSubmit = (e: React.FormEvent) => { @@ -138,8 +144,7 @@ const ModuleForm = ({ }) setTouched(newTouched) - // Validate all required fields - const nameError = validateNameLocal(formData.name) + const nameError = validateName(formData.name) const descriptionError = validateDescription(formData.description) const startDateError = validateStartDate(formData.startedAt) const endDateError = validateEndDateLocal(formData.endedAt) @@ -181,7 +186,7 @@ const ModuleForm = ({ handleInputChange('name', value) setTouched((prev) => ({ ...prev, name: true })) }} - error={errors.name} + error={errors.name || validationErrors?.name} touched={touched.name} required className="w-full min-w-0 lg:col-span-2" diff --git a/frontend/src/utils/helpers/handleGraphQLError.ts b/frontend/src/utils/helpers/handleGraphQLError.ts new file mode 100644 index 0000000000..7e68fc8ca0 --- /dev/null +++ b/frontend/src/utils/helpers/handleGraphQLError.ts @@ -0,0 +1,44 @@ +export type ValidationErrors = Record + +interface GraphQLErrorLike { + message: string + extensions?: Record +} + +interface ApolloErrorLike { + graphQLErrors: GraphQLErrorLike[] +} + +function isApolloErrorLike(error: unknown): error is ApolloErrorLike { + return ( + typeof error === 'object' && + error !== null && + 'graphQLErrors' in error && + Array.isArray((error as ApolloErrorLike).graphQLErrors) + ) +} + +export function extractGraphQLErrors(error: unknown): { + validationErrors: ValidationErrors + hasValidationErrors: boolean + unmappedErrors: string[] +} { + const validationErrors: ValidationErrors = {} + const unmappedErrors: string[] = [] + + if (isApolloErrorLike(error)) { + for (const gqlError of error.graphQLErrors) { + const extensions = gqlError.extensions as { code?: string; field?: unknown } | undefined + if (extensions?.code === 'VALIDATION_ERROR' && typeof extensions.field === 'string') { + validationErrors[extensions.field] = gqlError.message + } else { + unmappedErrors.push(gqlError.message) + } + } + } else if (error instanceof Error && error.message) { + unmappedErrors.push(error.message) + } + + const hasValidationErrors = Object.keys(validationErrors).length > 0 + return { validationErrors, hasValidationErrors, unmappedErrors } +}