Conversation
…uestion Images PR. Had to refactor some things. Avoided moving over some random changes to make merge with vector store refactor less of a headache. Also added a couple tests, and changed how the buttons look in PostResponseModal (simplified a lot)
| // don't include studentDeleted or TADeleted questions | ||
| status: Not( | ||
| In([ | ||
| asyncQuestionStatus.StudentDeleted, | ||
| asyncQuestionStatus.TADeleted, | ||
| ]), | ||
| ), |
There was a problem hiding this comment.
oh yeah this was a thing, Apparently all this time TADeleted async questions were being fetched I guess
bhunt02
left a comment
bhunt02
left a comment
There was a problem hiding this comment.
initial pass: everything looks pretty good. noticed some things so i'll let you review my comments, but i will make some time to manually test if necessary and then approve after pending issues!
packages/common/index.ts
Outdated
| loc?: Loc | ||
| name: string | ||
| type?: string | ||
| type?: string // "inserted_question", "inserted_async_question", etc. Will get put into a proper enum in the Vector Store Refactor but it will need to add inserted_async_question to the enum |
There was a problem hiding this comment.
thus far in that refactor i've been treating them largely the same, but i suppose this distinction is useful. i thought though that with the asyncQuestionId property that would suffice...
There was a problem hiding this comment.
huh right I guess I hadn't really thought of that. Yeah, I think I'll change it back to just be "inserted_question". It could make a data export very slightly more messy, but it's probably not gonna be worth the extra work in your refactor
| courseId, | ||
| userToken, | ||
| ); | ||
| await this.chatbotApiService.addDocumentChunk( |
There was a problem hiding this comment.
if this step fails, we should probably revert any changes... maybe keeping this logic on the chatbot side would be preferable rather than performing consecutive http requests?
There was a problem hiding this comment.
yeah that's true, I guess this way was just the most minimal amount of changes required which will make it less of a pain for the vector store refactor me thinks.
Eventually this whole endpoint will be changed a lot once the async question images are redone (since it needs to request the chatbot to get image summaries before prompting the chatbot a second time for the AiAnswer), and that will probably need to wait until after the vector store refactor since I imagine there will be some rather unpleasant and non-trivial merge conflicts, but that's okay.
Also, I think the async question chunks are more disposable. So if it does delete all the chunks and then fails to re-add them, not much is lost.
But yeah I agree, this is certainly something that should be changed in the future
| export const ERROR_MESSAGES = { | ||
| common: { | ||
| pageOutOfBounds: "Can't retrieve out of bounds page.", | ||
| noDiskSpace: |
There was a problem hiding this comment.
hm, we might need to apply this across a few different routes in the future...
There was a problem hiding this comment.
Yeah for sure, I sprinkled it everywhere for the async question images, but it should probably also be added to like the chatbot document upload and stuff.
| export function setupIntegrationTest( | ||
| module: Type<any>, | ||
| modifyModule?: ModuleModifier, | ||
| modifyModules?: ModuleModifier[], |
There was a problem hiding this comment.
good change LOL it makes way more sense for it to be possibly multiple. maybe we can set up an overload that will check Array.isArray(modifyModules) for this behaviour so it doesn't always have to be passed as an array
There was a problem hiding this comment.
true true, i shall do that real quick
There was a problem hiding this comment.
er I guess I don't really need an overload, I just changed the param type to modifyModules?: ModuleModifier | ModuleModifier[], and then used isArray as you suggested
…on' with 'asyncQuestionId' works instead
2 participants
Description
Cannibalized the saveToChatbot feature from #259 and refactored pieces of it due to new changes (and upcoming ones in #416 , i've tried to minimize the changes here to make the merge as small of a headache as possible).
Sister PR on Chatbot repo: https://github.com/ubco-db/chatbot/pull/62
screenshots below
(the checkbox is default enabled)
Mobile view
Mobile view on that weird edge case where the staff member created their own question (I re-did this)
Here this anytime question got split into 4 chunks. Upon updating the response, these chunks are deleted and replaced with new ones. Note that since I posted this screenshot, I've modified the names of the document to say "Previously Asked Anytime Question: {40 characters from the abstract/question}"
Note that this doesn't cover when professors delete an Anytime question. Ideally, I think you would add some sort of check asking "this anytime question was inserted into the chatbot's knowledge base, would you like to delete those chunks as well?" but tbh it's probably not worth the effort.
This also doesn't cover if the professor modifies the chunks (right now, it always deletes all chunks associated with the anytime question). I wouldn't even know how to handle this properly from a user perspective, other than maybe to show some kind of warning if they click saveToChatbot and the chunks had been previously modified. This would be a good thing to do but would add a lot of code and should wait until #416 is done anyway (since that one adds lastModifiedAt and firstInsertedAt, which would allow you to tell if any of these chunks were modified).
Closes #250
Type of change
yarn installHow Has This Been Tested?
Did a tiny bit of manual testing:
Checklist:
console.logs, leftover unused logic, or anything else that was accidentally committed)