Send Email Notification on Chatbot Answer Change by ribhavsharma · Pull Request #421 · ubco-db/helpme

ribhavsharma · 2025-11-04T08:06:25Z

Description

Added a checkbox in the edit chatbot answer modal, that lets the professor send out notifications to relevant students when a chatbot answer is changed.

Closes #305

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
This requires a run of yarn install
This change requires an addition/change to the production .env variables. These changes are below:
This change requires developers to add new .env variables. The file and variables needed are below:
This change requires a database query to update old data on production. This query is below:

How Has This Been Tested?

Tested locally, the correct email shows up in the "sent emails" log file.

Checklist:

I have performed a code review of my own code (under the "Files Changed" tab on github) to ensure nothing is committed that shouldn't be (e.g. leftover console.logs, leftover unused logic, or anything else that was accidentally committed)
I have commented my code where needed
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have checked that new and existing tests pass locally with my changes
Any work that this PR is dependent on has been merged into the main branch
Any UI changes have been checked to work on desktop, tablet, and mobile

…anged

...)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx

AdamFipke

see comments

packages/server/src/chatbot/chatbot.controller.ts

packages/common/index.ts

packages/server/src/chatbot/chatbot.controller.ts

AdamFipke · 2025-11-26T00:22:54Z

packages/server/src/chatbot/chatbot.controller.ts

+        <br/>
+        <p>You can revisit the course chatbot from your course page.</p>
+      </div>
+    `;


Most of the body of this endpoint should be moved into its own method inside chatbot.service.ts since controllers are technically supposed to just handle like error checking and then calling the right services, and thus are supposed to be pretty slim.

packages/server/src/chatbot/chatbot.controller.ts

AdamFipke · 2025-11-26T01:36:38Z

packages/server/src/chatbot/chatbot.module.ts

+        ChatbotService,
+        ChatbotApiService,
+        ChatbotSettingsSubscriber,
+        MailService,


huh interesting, i guess you didn't need to add MailModule to the imports array for some reason?

AdamFipke · 2025-11-26T01:36:43Z

packages/server/src/lmsIntegration/lmsIntegration.service.spec.ts

-        TestTypeOrmModule,
-        TestConfigModule,
-        FactoryModule,
-        ChatbotModule.forRoot(TestChatbotConnectionOptions),


wait... what?

I didn't really touch the lms stuff but i feel like removing the chatbotmodule would break tests but idk. @bhunt02

...)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx

AdamFipke · 2025-11-26T21:12:59Z

Oh right, and maybe adding a handful of integration or service tests for this would be a pretty good idea!

…comment explaining why we limit the number of recpients

AdamFipke

Thanks for addressing the feedback. I also manually tested it and it seems to work! Going over it I commited a couple small things.

Main things are some comments about tests. The feedback there could also be applied to your other PRs.

I would still see about moving most of the method body of notifyUpdatedAnswer() endpoint into its own function inside chatbot.service.ts for organization reasons.

AdamFipke · 2025-12-02T22:02:21Z

...)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx

+    const { emailNotifyOnAnswerUpdate, ...sanitizedValues } = values
    const valuesWithId = {
-      ...values,
+      ...sanitizedValues,


Huh neat I didn't know you could use the spread operator when deconstructing an object

AdamFipke · 2025-12-02T22:33:59Z

packages/server/test/chatbot.integration.ts

    });
  });

+  describe('POST /chatbot/question/:courseId/:vectorStoreId/notify', () => {


I would possibly add a few more tests for:

Making sure only professors and TAs can use it

Making sure it only notifies students who asked the question, not other students in the course.

Make sure it still succeeds for empty strings for old and new answer

Tests to make sure the DTO is working:

Making sure it fails if oldAnswer is undefined

Making sure it fails if newAnswer is undefined

Make sure you can't send integers or other data types for oldAnswer, newAnswer, oldQuestion, and newQuestion

You may also find it nicer to refactor a bunch of your tests into helper functions (you can see an example in @bhunt02 's PR here https://github.com/ubco-db/helpme/pull/351/files#diff-8d3cece12bb8c1d5223de78a3e45bee5b3becbb4dfef50609332fac922bebd6cR737.

You can probably just get away with using AI to generate these (just use a smart model like gemeni 3 pro or opus/sonnet 4.5) and make sure to go over it so there's no glaring logic issues.

It would also be nice if there was a test to make sure the email didn't have "null" or stuff like that, but that might be a little more effort than it's worth (since it might involve modifying the email test util functions and saving a snapshot). Maybe look into it?

AdamFipke · 2025-12-02T22:38:47Z

packages/server/src/chatbot/chatbot.controller.spec.ts

+import { BadRequestException } from '@nestjs/common';
+import { NotifyUpdatedChatbotAnswerParams, Role } from '@koh/common';
+
+describe('ChatbotController - notifyUpdatedAnswer', () => {


So this is kinda weird. This is like an integration test but with a few more details abstracted away, and it doesn't really follow the rest of the project.

Having just the chatbot.integration.ts tests are enough (since that tests calling the whole endpoint). Once you move most of the body of the notifyUpdatedAnswer to its own function inside chatbot.service.ts, you could make some corresponding unit tests inside chatbot.service.spec.ts, but it's usually not necessary since the integration tests already cover it. Though, having unit tests may be nicer if you want to test for very specific behaviour by mocking a bunch of things (e.g. for making sure the email body is correct).

I think chatbot.service.ts does a lot of jobs already... should the FX for notifying not be delegated to a corresponding proviser?

I think chatbot.service.ts does a lot of jobs already... should the FX for notifying not be delegated to a corresponding proviser?

Hmm maybe. Are you thinking a small-scoped provider (e.g. chatbot-email-notification.service.ts) or larger provider (e.g. notification-email.service.ts)?

Though I don't see us adding many more email notifications to do with the chatbot. And a larger one wouldn't make much sense imo since then it's a big random service file that gets imported to a bunch of other files for only 1 function.

If we're concerned about the size of chatbot.service.ts, it woudl probably make more sense to refactor out the ChatbotOrganizationSettings (or just ChatbotSettings) stuff into its own service.

No what I mean is single responsibility principle. Notifications should be handled by one provider, not many

All the chatbot stuff falls under the purview of the chatbot provider

…-changed-notif

ribhavsharma added 3 commits November 3, 2025 23:56

feat: add option to send email notificatoin when chatbot answer is ch…

d942657

…anged

add mail service to provider

7fbf392

remove unnecessary dependency from lms test

31447de

AdamFipke reviewed Nov 25, 2025

View reviewed changes

...)/course/[cid]/(settings)/settings/chatbot_questions/components/EditChatbotQuestionModal.tsx Outdated Show resolved Hide resolved

AdamFipke mentioned this pull request Nov 26, 2025

Integrate official HelpMe email with system #310

Open

AdamFipke requested changes Nov 26, 2025

View reviewed changes

reduce bottom margin of answer field

eca8b32

AdamFipke mentioned this pull request Nov 26, 2025

Add away toggle for TAs #427

Merged

16 tasks

ribhavsharma and others added 4 commits December 1, 2025 21:45

addressing feedback

11eeef3

trying to fix test

6110ee7

fixing... test

0003ee1

fix notifyUpdatedAnswer only callable by profs. Removed an . Added a …

9569e98

…comment explaining why we limit the number of recpients

AdamFipke requested changes Dec 2, 2025

View reviewed changes

ribhavsharma added 4 commits February 25, 2026 15:09

Merge remote-tracking branch 'origin/main' into ribhav/chatbot-answer…

824bd79

…-changed-notif

addressing feedback

cf46dad

fix test

c41c2ef

fixing flaky test + rm questionable workaround

980bd5d

Conversation

ribhavsharma commented Nov 4, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

AdamFipke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdamFipke commented Nov 26, 2025

Uh oh!

AdamFipke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamFipke Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AdamFipke Dec 2, 2025 •

edited

Loading