diff --git a/packages/common/index.ts b/packages/common/index.ts index 10c49efb0..ce11983b2 100644 --- a/packages/common/index.ts +++ b/packages/common/index.ts @@ -351,6 +351,7 @@ export enum MailServiceType { ASYNC_QUESTION_NEW_COMMENT_ON_MY_POST = 'async_question_new_comment_on_my_post', ASYNC_QUESTION_NEW_COMMENT_ON_OTHERS_POST = 'async_question_new_comment_on_others_post', COURSE_CLONE_SUMMARY = 'course_clone_summary', + CHATBOT_ANSWER_UPDATED = 'chatbot_answer_updated', } /** * Represents one of three possible user roles in a course. @@ -525,6 +526,22 @@ export interface UpdateChatbotQuestionParams { }[] } +export class NotifyUpdatedChatbotAnswerParams { + @IsString() + oldAnswer!: string + + @IsString() + newAnswer!: string + + @IsString() + @IsOptional() + oldQuestion?: string + + @IsString() + @IsOptional() + newQuestion?: string +} + // this is the response from the backend when new questions are asked // if question is I don't know, only answer and questionId are returned export interface ChatbotAskResponse { diff --git a/packages/frontend/app/(dashboard)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx b/packages/frontend/app/(dashboard)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx index 3395d2ca1..7886457b5 100644 --- a/packages/frontend/app/(dashboard)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx +++ b/packages/frontend/app/(dashboard)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx @@ -31,6 +31,7 @@ interface FormValues { answer: string verified: boolean suggested: boolean + emailNotifyOnAnswerUpdate?: boolean sourceDocuments: SourceDocument[] selectedDocuments: { docId: string @@ -46,6 +47,23 @@ interface EditChatbotQuestionModalProps { cid: number deleteQuestion: (id: string) => void } +type AnswerUpdateCheckboxProps = { + form: any + originalAnswer: string + checked?: boolean + onChange?: (e: any) => void +} + +const AnswerUpdateCheckbox: React.FC = ({ + form, + originalAnswer, + checked, + onChange, +}) => { + const currentAnswer = Form.useWatch('answer', form) + const changed = (currentAnswer ?? '') !== (originalAnswer ?? '') + return +} const EditChatbotQuestionModal: React.FC = ({ open, @@ -150,16 +168,49 @@ const EditChatbotQuestionModal: React.FC = ({ }) } + const { emailNotifyOnAnswerUpdate, ...sanitizedValues } = values const valuesWithId = { - ...values, + ...sanitizedValues, id: editingRecord.vectorStoreId, sourceDocuments: values.sourceDocuments || [], } await API.chatbot.staffOnly .updateQuestion(cid, valuesWithId) - .then(() => { + .then(async () => { message.success('Question updated successfully') + if (emailNotifyOnAnswerUpdate) { + try { + const resp = await API.chatbot.staffOnly.notifyAnswerUpdate( + cid, + editingRecord.vectorStoreId, + { + oldAnswer: editingRecord.answer, + newAnswer: values.answer, + oldQuestion: editingRecord.question, + newQuestion: values.question, + }, + ) + if (resp?.recipients != undefined) { + if (resp.totalRecipients === 5) { + message.success( + `Notification email sent to ${resp.recipients} user${ + resp.recipients === 1 ? '' : 's' + } (max 5).`, + ) + } else { + message.success( + `Notification email sent to ${resp.recipients} user${ + resp.recipients === 1 ? '' : 's' + }`, + ) + } + } + } catch (e) { + const errorMessage = getErrorMessage(e) + message.error('Failed to send notification email: ' + errorMessage) + } + } onSuccessfulUpdate() }) .catch((e) => { @@ -227,6 +278,7 @@ const EditChatbotQuestionModal: React.FC = ({ question: editingRecord.question, verified: editingRecord.verified, suggested: editingRecord.suggested, + emailNotifyOnAnswerUpdate: false, sourceDocuments: editingRecord.sourceDocuments, }} clearOnDestroy @@ -245,6 +297,7 @@ const EditChatbotQuestionModal: React.FC = ({ , classNames: { @@ -256,6 +309,25 @@ const EditChatbotQuestionModal: React.FC = ({ > + +

+ Sends an email to the student(s) who previously asked this + question with a before/after of the answer. +

+ + } + > + + => this.req('DELETE', `/api/v1/chatbot/document/${courseId}/${docId}`), + notifyAnswerUpdate: async ( + courseId: number, + vectorStoreId: string, + body: { + oldAnswer: string + newAnswer: string + oldQuestion?: string + newQuestion?: string + }, + ): Promise<{ + recipients: number + totalRecipients: number + unsubscribedRecipients: number + }> => + this.req( + 'POST', + `/api/v1/chatbot/question/${courseId}/${vectorStoreId}/notify`, + undefined, + body, + ), uploadDocument: async ( courseId: number, body: FormData, diff --git a/packages/server/src/chatbot/chatbot.controller.ts b/packages/server/src/chatbot/chatbot.controller.ts index 0b727fa33..93031fe08 100644 --- a/packages/server/src/chatbot/chatbot.controller.ts +++ b/packages/server/src/chatbot/chatbot.controller.ts @@ -51,6 +51,7 @@ import { OrganizationChatbotSettings, OrganizationChatbotSettingsDefaults, OrganizationRole, + NotifyUpdatedChatbotAnswerParams, Role, UpdateChatbotProviderBody, UpdateChatbotQuestionParams, @@ -378,6 +379,29 @@ export class ChatbotController { // } // resets all chatbot data for the course. Unused + + // staff-only: send notification email to all students who asked this question + // Body must include oldAnswer and newAnswer, and can optionally include question changes for context + @Post('question/:courseId/:vectorStoreId/notify') + @UseGuards(CourseRolesGuard) + @Roles(Role.PROFESSOR, Role.TA) + async notifyUpdatedAnswer( + @Param('courseId', ParseIntPipe) courseId: number, + @Param('vectorStoreId') vectorStoreId: string, + @Body() body: NotifyUpdatedChatbotAnswerParams, + @User({ courses: true }) user: UserModel, + ): Promise<{ + recipients: number; + totalRecipients: number; + unsubscribedRecipients: number; + }> { + return await this.chatbotService.notifyUpdatedAnswer( + courseId, + vectorStoreId, + body, + user, + ); + } // @Get('resetCourse/:courseId') // @UseGuards(CourseRolesGuard) // @Roles(Role.PROFESSOR, Role.TA) diff --git a/packages/server/src/chatbot/chatbot.module.ts b/packages/server/src/chatbot/chatbot.module.ts index ee640d5ce..3dba0cdcf 100644 --- a/packages/server/src/chatbot/chatbot.module.ts +++ b/packages/server/src/chatbot/chatbot.module.ts @@ -7,6 +7,7 @@ import { CacheModule } from '@nestjs/cache-manager'; import { ChatbotDataSourceService } from './chatbot-datasource/chatbot-datasource.service'; import { ChatbotDataSourceModule } from './chatbot-datasource/chatbot-datasource.module'; import { PostgresConnectionOptions } from 'typeorm/driver/postgres/PostgresConnectionOptions'; +import { MailService } from 'mail/mail.service'; @Module({ controllers: [ChatbotController], @@ -21,7 +22,12 @@ export class ChatbotModule { CacheModule.register(), ChatbotDataSourceModule.forRoot(connectionOptions), ], - providers: [ChatbotService, ChatbotApiService, ChatbotSettingsSubscriber], + providers: [ + ChatbotService, + ChatbotApiService, + ChatbotSettingsSubscriber, + MailService, + ], }; } } diff --git a/packages/server/src/chatbot/chatbot.service.spec.ts b/packages/server/src/chatbot/chatbot.service.spec.ts index 9140becbe..c97754422 100644 --- a/packages/server/src/chatbot/chatbot.service.spec.ts +++ b/packages/server/src/chatbot/chatbot.service.spec.ts @@ -37,7 +37,9 @@ import { CreateOrganizationChatbotSettingsBody, dropUndefined, ERROR_MESSAGES, + NotifyUpdatedChatbotAnswerParams, OrganizationChatbotSettingsDefaults, + Role, UpdateChatbotProviderBody, UpdateLLMTypeBody, UpsertCourseChatbotSettings, @@ -51,6 +53,8 @@ import { chatbotTables, } from './chatbot-datasource/chatbot-datasource.service'; import { ChatbotApiService } from './chatbot-api.service'; +import { MailService } from 'mail/mail.service'; +import { UserSubscriptionModel } from '../mail/user-subscriptions.entity'; const mockCacheManager = { get: jest.fn(), @@ -59,6 +63,10 @@ const mockCacheManager = { reset: jest.fn(), }; +const mockMailService = { + sendEmail: jest.fn(), +}; + describe('ChatbotService', () => { let service: ChatbotService; let subscriber: ChatbotSettingsSubscriber; @@ -91,6 +99,10 @@ describe('ChatbotService', () => { provide: CACHE_MANAGER, useValue: mockCacheManager, }, + { + provide: MailService, + useValue: mockMailService, + }, ], }).compile(); @@ -133,6 +145,7 @@ describe('ChatbotService', () => { ).then(); }); updateChatbotRepositorySpy.mockRestore(); + mockMailService.sendEmail.mockReset(); }); beforeEach(async () => { @@ -1876,4 +1889,146 @@ describe('ChatbotService', () => { }, ); }); + + describe('notifyUpdatedAnswer', () => { + const makeBody = ( + overrides: Partial = {}, + ): NotifyUpdatedChatbotAnswerParams => ({ + oldAnswer: 'old', + newAnswer: 'new', + ...overrides, + }); + + const makeUser = (courseId: number, role: Role): any => ({ + id: 1, + courses: [{ courseId, role }], + }); + + it('throws when answer did not change', async () => { + await expect( + service.notifyUpdatedAnswer( + 1, + 'vec-id', + makeBody({ oldAnswer: 'same', newAnswer: ' same ' }), + makeUser(1, Role.PROFESSOR), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('throws when there are no recipients', async () => { + const findSpy = jest + .spyOn(ChatbotQuestionModel, 'find') + .mockResolvedValue([] as any); + + await expect( + service.notifyUpdatedAnswer( + 1, + 'vec-id', + makeBody(), + makeUser(1, Role.PROFESSOR), + ), + ).rejects.toBeInstanceOf(BadRequestException); + + expect(mockMailService.sendEmail).not.toHaveBeenCalled(); + findSpy.mockRestore(); + }); + + it('sends individual emails and caps at 5 recipients', async () => { + const courseId = 1; + const vectorStoreId = 'vec-id'; + const emails = [ + 'u1@example.com', + 'u2@example.com', + 'u3@example.com', + 'u4@example.com', + 'u5@example.com', + 'u6@example.com', + ]; + + const findSpy = jest + .spyOn(ChatbotQuestionModel, 'find') + .mockResolvedValue( + emails.map((email) => ({ + interaction: { + course: { id: courseId, name: 'Test Course' }, + user: { email }, + }, + })) as any, + ); + const subscriptionSpy = jest + .spyOn(UserSubscriptionModel, 'find') + .mockResolvedValue([]); + + const result = await service.notifyUpdatedAnswer( + courseId, + vectorStoreId, + makeBody(), + makeUser(courseId, Role.PROFESSOR), + ); + + expect(mockMailService.sendEmail).toHaveBeenCalledTimes(5); + const calledRecipients = mockMailService.sendEmail.mock.calls.map( + (args) => args[0].receiverOrReceivers, + ); + expect(calledRecipients).toEqual(emails.slice(0, 5)); + expect(result).toEqual({ + recipients: 5, + totalRecipients: 5, + unsubscribedRecipients: 0, + }); + findSpy.mockRestore(); + subscriptionSpy.mockRestore(); + }); + + it('does not email unsubscribed recipients and reports counts', async () => { + const courseId = 1; + const vectorStoreId = 'vec-id'; + const emails = [ + 'u1@example.com', + 'u2@example.com', + 'u3@example.com', + 'u4@example.com', + 'u5@example.com', + ]; + const unsubscribedEmails = [emails[1], emails[3]]; + + const findSpy = jest + .spyOn(ChatbotQuestionModel, 'find') + .mockResolvedValue( + emails.map((email) => ({ + interaction: { + course: { id: courseId, name: 'Test Course' }, + user: { email }, + }, + })) as any, + ); + const subscriptionSpy = jest + .spyOn(UserSubscriptionModel, 'find') + .mockResolvedValue( + unsubscribedEmails.map((email) => ({ user: { email } })) as any, + ); + + const result = await service.notifyUpdatedAnswer( + courseId, + vectorStoreId, + makeBody(), + makeUser(courseId, Role.PROFESSOR), + ); + + expect(mockMailService.sendEmail).toHaveBeenCalledTimes(3); + const calledRecipients = mockMailService.sendEmail.mock.calls.map( + (args) => args[0].receiverOrReceivers, + ); + expect(calledRecipients).toEqual( + emails.filter((email) => !unsubscribedEmails.includes(email)), + ); + expect(result).toEqual({ + recipients: 3, + totalRecipients: 5, + unsubscribedRecipients: 2, + }); + findSpy.mockRestore(); + subscriptionSpy.mockRestore(); + }); + }); }); diff --git a/packages/server/src/chatbot/chatbot.service.ts b/packages/server/src/chatbot/chatbot.service.ts index d5a971567..1931af7b2 100644 --- a/packages/server/src/chatbot/chatbot.service.ts +++ b/packages/server/src/chatbot/chatbot.service.ts @@ -21,10 +21,13 @@ import { CreateOrganizationChatbotSettingsBody, dropUndefined, ERROR_MESSAGES, + MailServiceType, + NotifyUpdatedChatbotAnswerParams, OllamaLLMType, OllamaModelDescription, OpenAILLMType, OrganizationChatbotSettingsDefaults, + Role, UpdateChatbotProviderBody, UpdateLLMTypeBody, UpsertCourseChatbotSettings, @@ -39,6 +42,8 @@ import { CACHE_MANAGER } from '@nestjs/cache-manager'; import { Cache } from 'cache-manager'; import { ChatbotApiService } from './chatbot-api.service'; import { ChatbotDataSourceService } from './chatbot-datasource/chatbot-datasource.service'; +import { MailService } from '../mail/mail.service'; +import { UserSubscriptionModel } from '../mail/user-subscriptions.entity'; // OpenAI makes it really annoying to scrape any data related // to the model capabilities (text, image processing) (selfish, much?) @@ -137,6 +142,7 @@ export class ChatbotService { private dataSource: DataSource, private chatbotDataSource: ChatbotDataSourceService, private chatbotApiService: ChatbotApiService, + private readonly mailService: MailService, @Inject(CACHE_MANAGER) private cacheManager: Cache, ) {} @@ -316,6 +322,137 @@ export class ChatbotService { return question; } + async notifyUpdatedAnswer( + courseId: number, + vectorStoreId: string, + body: NotifyUpdatedChatbotAnswerParams, + user: UserModel, + ): Promise<{ + recipients: number; + totalRecipients: number; + unsubscribedRecipients: number; + }> { + const { oldAnswer, newAnswer, oldQuestion, newQuestion } = body ?? {}; + + if ((oldAnswer ?? '').trim() === (newAnswer ?? '').trim()) { + throw new BadRequestException( + 'Cannot send notification: answer was not changed.', + ); + } + + const asked = await ChatbotQuestionModel.find({ + where: { + vectorStoreId, + interaction: { + course: { + id: courseId, + }, + }, + }, + relations: { interaction: { user: true, course: true } }, + }); + + const recipients = Array.from( + new Set( + asked + .filter((a) => a.interaction?.course?.id === courseId) + .map((a) => a.interaction?.user?.email) + .filter((email): email is string => !!email), + ), + ); + + if (recipients.length === 0) { + throw new BadRequestException( + 'Cannot send notification: no recipients for this question.', + ); + } + + const qChanged = + (oldQuestion ?? '').trim() !== (newQuestion ?? oldQuestion ?? '').trim(); + + const questionSection = qChanged + ? `Question (before)${ + oldQuestion ?? '' + } + Question (after)${ + newQuestion ?? '' + }` + : `Question${ + oldQuestion ?? newQuestion ?? '' + }`; + + const courseName = asked[0]?.interaction?.course?.name ?? 'your course'; + + const courseRole = + user.courses?.find((uc) => uc.courseId === courseId)?.role ?? null; + + const staffDescriptor = + courseRole === Role.PROFESSOR + ? 'the Instructor' + : courseRole === Role.TA + ? 'a TA' + : 'a staff member'; + + const html = ` +
+

The answer to a chatbot question you asked in ${courseName} has been updated by ${staffDescriptor}.

+ + ${questionSection} + + + + + + + + +
Answer (before)${oldAnswer}
Answer (after)${newAnswer}
+
+

+ Want to follow up? Create an + Anytime Question + to discuss it more! +

+
+ `; + + // TODO: Once we get our official HelpMe email, we can remove this cap + // (this was added so we don't get rate limited by gmail as easily). + // Note that we CANNOT just 1 email with all the recipients, since then students + // will be able to see each others emails (and that they asked a particular question) + const limitedRecipients = recipients.slice(0, 5); + const unsubscribed = await UserSubscriptionModel.find({ + where: { + isSubscribed: false, + user: { email: In(limitedRecipients) }, + service: { serviceType: MailServiceType.CHATBOT_ANSWER_UPDATED }, + }, + relations: { user: true }, + }); + const unsubscribedEmails = new Set( + unsubscribed.map((sub) => sub.user.email), + ); + const subscribedRecipients = limitedRecipients.filter( + (email) => !unsubscribedEmails.has(email), + ); + + for (const email of subscribedRecipients) { + await this.mailService.sendEmail({ + receiverOrReceivers: email, + subject: `Chatbot answer updated by staff member for ${courseName}`, + type: MailServiceType.CHATBOT_ANSWER_UPDATED, + content: html, + track: false, + }); + } + + return { + recipients: subscribedRecipients.length, + totalRecipients: limitedRecipients.length, + unsubscribedRecipients: unsubscribedEmails.size, + }; + } + async isChatbotServiceLegacy(courseId: number): Promise { const course = await CourseModel.findOne({ where: { id: courseId }, diff --git a/packages/server/src/lmsIntegration/lmsIntegration.service.spec.ts b/packages/server/src/lmsIntegration/lmsIntegration.service.spec.ts index 2c6e1dd59..1c3247ec0 100644 --- a/packages/server/src/lmsIntegration/lmsIntegration.service.spec.ts +++ b/packages/server/src/lmsIntegration/lmsIntegration.service.spec.ts @@ -6,11 +6,7 @@ import { } from './lmsIntegration.service'; import { Test, TestingModule } from '@nestjs/testing'; import { CACHE_MANAGER } from '@nestjs/cache-manager'; -import { - TestChatbotConnectionOptions, - TestConfigModule, - TestTypeOrmModule, -} from '../../test/util/testUtils'; +import { TestConfigModule, TestTypeOrmModule } from '../../test/util/testUtils'; import { AbstractLMSAdapter, LMSIntegrationAdapter, @@ -44,7 +40,6 @@ import { LMSAnnouncementModel } from './lmsAnnouncement.entity'; import { LMSAssignmentModel } from './lmsAssignment.entity'; import { FactoryModule } from 'factory/factory.module'; import { FactoryService } from 'factory/factory.service'; -import { ChatbotModule } from '../chatbot/chatbot.module'; import { ChatbotApiService } from '../chatbot/chatbot-api.service'; import { LMSAccessTokenModel } from './lms-access-token.entity'; import { UserModel } from '../profile/user.entity'; @@ -70,12 +65,7 @@ describe('LMSIntegrationService', () => { beforeAll(async () => { const module: TestingModule = await Test.createTestingModule({ - imports: [ - TestTypeOrmModule, - TestConfigModule, - FactoryModule, - ChatbotModule.forRoot(TestChatbotConnectionOptions), - ], + imports: [TestTypeOrmModule, TestConfigModule, FactoryModule], providers: [ LMSIntegrationService, LMSIntegrationAdapter, diff --git a/packages/server/test/chatbot.integration.ts b/packages/server/test/chatbot.integration.ts index f561333e6..17f3a5a32 100644 --- a/packages/server/test/chatbot.integration.ts +++ b/packages/server/test/chatbot.integration.ts @@ -7,6 +7,7 @@ import { CreateOrganizationChatbotSettingsBody, dropUndefined, ERROR_MESSAGES, + MailServiceType, OrganizationRole, Role, UpdateChatbotProviderBody, @@ -18,14 +19,22 @@ import { CourseFactory, InteractionFactory, LLMTypeFactory, + mailServiceFactory, OrganizationChatbotSettingsFactory, OrganizationCourseFactory, OrganizationFactory, OrganizationUserFactory, + userSubscriptionFactory, UserCourseFactory, UserFactory, } from './util/factories'; -import { setupIntegrationTest } from './util/testUtils'; +import { + expectEmailNotSent, + expectEmailSent, + mockEmailService, + overrideEmailService, + setupIntegrationTest, +} from './util/testUtils'; import { ChatbotQuestionModel } from 'chatbot/question.entity'; import { OrganizationModel } from '../src/organization/organization.entity'; import { CourseModel } from '../src/course/course.entity'; @@ -40,7 +49,10 @@ import { UserModel } from '../src/profile/user.entity'; import { ChatbotApiService } from '../src/chatbot/chatbot-api.service'; describe('ChatbotController Integration', () => { - const { supertest } = setupIntegrationTest(ChatbotModule); + const { supertest } = setupIntegrationTest( + ChatbotModule, + overrideEmailService, + ); describe('PATCH /chatbot/questionScore/:courseId/:questionId', () => { it('should allow a student to update the score of a question', async () => { @@ -133,6 +145,323 @@ describe('ChatbotController Integration', () => { }); }); + describe('POST /chatbot/question/:courseId/:vectorStoreId/notify', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + const createStaffForCourse = async (role: Role = Role.PROFESSOR) => { + const staff = await UserFactory.create(); + const course = await CourseFactory.create(); + await UserCourseFactory.create({ + user: staff, + course, + role, + }); + return { staff, course }; + }; + + const createAskedQuestion = async ( + course: CourseModel, + vectorStoreId: string, + email: string, + ) => { + const student = await UserFactory.create({ email }); + await UserCourseFactory.create({ + user: student, + course, + role: Role.STUDENT, + }); + const interaction = await InteractionFactory.create({ + user: student, + course, + }); + await ChatbotQuestionModel.create({ + vectorStoreId, + questionText: 'Original question text', + responseText: 'Old answer', + suggested: true, + interaction, + }).save(); + return student; + }; + + const notifyBody = (overrides: Record = {}) => ({ + oldAnswer: 'Old answer', + newAnswer: 'New updated answer', + oldQuestion: 'Old question?', + newQuestion: 'New updated question?', + ...overrides, + }); + + it('only allows professors and TAs to use notify endpoint', async () => { + const { course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-access-control'; + await createAskedQuestion(course, vectorStoreId, 'asked@example.com'); + + const ta = await UserFactory.create(); + await UserCourseFactory.create({ + user: ta, + course, + role: Role.TA, + }); + + const student = await UserFactory.create(); + await UserCourseFactory.create({ + user: student, + course, + role: Role.STUDENT, + }); + + await supertest({ userId: ta.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody()) + .expect(201); + + await supertest({ userId: student.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody()) + .expect(403); + }); + + it('sends individual emails (capped at 5 recipients) to users who asked the question', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + + const vectorStoreId = 'vec-notify-123'; + + const students = await Promise.all( + Array.from({ length: 6 }).map((_, i) => + createAskedQuestion( + course, + vectorStoreId, + `chatbot-notify-${i}@example.com`, + ), + ), + ); + + const res = await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody()) + .expect(201); + + expect(res.body).toEqual({ + recipients: 5, + totalRecipients: 5, + unsubscribedRecipients: 0, + }); + const calledEmails = mockEmailService.sendEmail.mock.calls.map( + (args) => args[0].receiverOrReceivers, + ); + const calledTypes = mockEmailService.sendEmail.mock.calls.map( + (args) => args[0].type, + ); + const allStudentEmails = new Set(students.map((s) => s.email)); + + expect(calledEmails).toHaveLength(5); + expect(new Set(calledEmails).size).toBe(5); + for (const email of calledEmails) { + expect(allStudentEmails.has(email)).toBe(true); + } + expect(calledTypes).toEqual( + Array(5).fill(MailServiceType.CHATBOT_ANSWER_UPDATED), + ); + }); + + it('respects explicit unsubscribes and returns recipient counts', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + + const vectorStoreId = 'vec-notify-unsubscribed'; + const students = await Promise.all( + Array.from({ length: 5 }).map((_, i) => + createAskedQuestion( + course, + vectorStoreId, + `chatbot-notify-unsub-${i}@example.com`, + ), + ), + ); + + const mailService = await mailServiceFactory.create({ + serviceType: MailServiceType.CHATBOT_ANSWER_UPDATED, + name: 'chatbot_answer_updated', + }); + + for (const [i, student] of students.entries()) { + await userSubscriptionFactory.create({ + user: student, + service: mailService, + isSubscribed: i < 3, + }); + } + + const res = await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody()) + .expect(201); + + expect(res.body).toEqual({ + recipients: 3, + totalRecipients: 5, + unsubscribedRecipients: 2, + }); + const calledEmails = mockEmailService.sendEmail.mock.calls.map( + (args) => args[0].receiverOrReceivers, + ); + const calledTypes = mockEmailService.sendEmail.mock.calls.map( + (args) => args[0].type, + ); + const subscribedEmails = new Set( + students.slice(0, 3).map((student) => student.email), + ); + const unsubscribedEmails = new Set( + students.slice(3).map((student) => student.email), + ); + + expect(calledEmails).toHaveLength(3); + for (const email of calledEmails) { + expect(subscribedEmails.has(email)).toBe(true); + expect(unsubscribedEmails.has(email)).toBe(false); + } + expect(calledTypes).toEqual( + Array(3).fill(MailServiceType.CHATBOT_ANSWER_UPDATED), + ); + }); + + it('returns 400 and does not send emails when answer did not change', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-unchanged'; + await createAskedQuestion(course, vectorStoreId, 'unchanged@example.com'); + + await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send( + notifyBody({ oldAnswer: 'Same answer', newAnswer: ' Same answer ' }), + ) + .expect(400); + + expectEmailNotSent(); + }); + + it('only notifies students who asked the target question', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-only-asked'; + const askedStudents = await Promise.all( + Array.from({ length: 2 }).map((_, i) => + createAskedQuestion(course, vectorStoreId, `asked-${i}@example.com`), + ), + ); + + const otherStudent = await UserFactory.create({ + email: 'not-asked@example.com', + }); + await UserCourseFactory.create({ + user: otherStudent, + course, + role: Role.STUDENT, + }); + + await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody()) + .expect(201); + + expectEmailSent( + askedStudents.map((s) => s.email), + Array(askedStudents.length).fill( + MailServiceType.CHATBOT_ANSWER_UPDATED, + ), + ); + const calledRecipients = mockEmailService.sendEmail.mock.calls.map( + (call) => call[0].receiverOrReceivers, + ); + expect(calledRecipients).not.toContain(otherStudent.email); + }); + + it('succeeds when oldAnswer is empty string and newAnswer is non-empty', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-empty-old'; + await createAskedQuestion(course, vectorStoreId, 'empty-old@example.com'); + + const res = await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody({ oldAnswer: '', newAnswer: 'Updated answer' })) + .expect(201); + + expect(res.body.recipients).toBe(1); + }); + + it('succeeds when newAnswer is empty string and oldAnswer is non-empty', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-empty-new'; + await createAskedQuestion(course, vectorStoreId, 'empty-new@example.com'); + + const res = await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(notifyBody({ oldAnswer: 'Old answer', newAnswer: '' })) + .expect(201); + + expect(res.body.recipients).toBe(1); + }); + + it('returns 400 if oldAnswer is missing', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-missing-old'; + await createAskedQuestion( + course, + vectorStoreId, + 'missing-old@example.com', + ); + + await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send({ newAnswer: 'updated' }) + .expect(400); + }); + + it('returns 400 if newAnswer is missing', async () => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = 'vec-notify-missing-new'; + await createAskedQuestion( + course, + vectorStoreId, + 'missing-new@example.com', + ); + + await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send({ oldAnswer: 'old' }) + .expect(400); + }); + + it.each([ + [{ oldAnswer: 123, newAnswer: 'new' }, 'oldAnswer number'], + [{ oldAnswer: 'old', newAnswer: 456 }, 'newAnswer number'], + [ + { oldAnswer: 'old', newAnswer: 'new', oldQuestion: 999 }, + 'oldQuestion number', + ], + [ + { oldAnswer: 'old', newAnswer: 'new', newQuestion: false }, + 'newQuestion boolean', + ], + ])('returns 400 for invalid DTO type: %s', async (body, _label) => { + const { staff, course } = await createStaffForCourse(Role.PROFESSOR); + const vectorStoreId = `vec-notify-invalid-${Math.random().toString(36).slice(2)}`; + await createAskedQuestion( + course, + vectorStoreId, + 'invalid-type@example.com', + ); + + await supertest({ userId: staff.id }) + .post(`/chatbot/question/${course.id}/${vectorStoreId}/notify`) + .send(body) + .expect(400); + + expectEmailNotSent(); + }); + }); + describe('GET /chatbot/history', () => { it('should return the chatbot history for a user', async () => { const user = await UserFactory.create(); diff --git a/packages/server/test/question.integration.ts b/packages/server/test/question.integration.ts index 9c60f15ad..90aafe3b2 100644 --- a/packages/server/test/question.integration.ts +++ b/packages/server/test/question.integration.ts @@ -29,7 +29,6 @@ import { modifyMockNotifs, setupIntegrationTest, } from './util/testUtils'; -import { forEach } from 'lodash'; import { QuestionTypeModel } from 'questionType/question-type.entity'; import { StudentTaskProgressModel } from 'studentTaskProgress/studentTaskProgress.entity'; @@ -202,7 +201,7 @@ describe('Question Integration', () => { const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, @@ -217,7 +216,7 @@ describe('Question Integration', () => { queueId: queue.id, }; questionTypes.push(sendQuestionTypes); - }); + } await TACourseFactory.create({ user, courseId: queue.courseId }); expect(await QuestionModel.count({ where: { queueId: 1 } })).toEqual(0); @@ -225,14 +224,7 @@ describe('Question Integration', () => { expect(response.status).toBe(403); }); it('post question fails with non-existent queue', async () => { - const course = await CourseFactory.create(); const user = await UserFactory.create(); - const ta = await TACourseFactory.create({ - course: course, - user: await UserFactory.create(), - }); - // wait 0.25s to help deadlock issue? - await new Promise((resolve) => setTimeout(resolve, 250)); await supertest({ userId: user.id }) .post('/questions/999') .send({ @@ -266,14 +258,14 @@ describe('Question Integration', () => { course: course2, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: course2.id, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queueImNotIn, questionTypes); expect(response.status).toBe(404); @@ -288,14 +280,14 @@ describe('Question Integration', () => { isDisabled: true, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: course.id, }); questionTypes.push(currentQuestionType); - }); + } const user = await UserFactory.create(); await StudentCourseFactory.create({ user, courseId: queue.courseId }); @@ -370,14 +362,14 @@ describe('Question Integration', () => { }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: course.id, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queue2, questionTypes); @@ -710,14 +702,14 @@ describe('Question Integration', () => { status: OpenQuestionStatus.Drafting, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: course2.id, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queue2, questionTypes); expect(response.status).toBe(201); @@ -751,14 +743,14 @@ describe('Question Integration', () => { status: OpenQuestionStatus.Drafting, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: queue.courseId, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queue, questionTypes); expect(response.status).toBe(201); @@ -792,14 +784,14 @@ describe('Question Integration', () => { courseId: queue.courseId, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: queue.courseId, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queue, questionTypes); expect(response.status).toBe(201); @@ -829,14 +821,14 @@ describe('Question Integration', () => { courseId: queue.courseId, }); const questionTypes = []; - forEach(QuestionTypes, async (questionType) => { + for (const questionType of QuestionTypes) { const currentQuestionType = await QuestionTypeFactory.create({ name: questionType.name, color: questionType.color, cid: queue.courseId, }); questionTypes.push(currentQuestionType); - }); + } const response = await postQuestion(user, queue, questionTypes); expect(response.status).toBe(201);