Add summarize reviews button on admin page #471
Conversation
… courses with at least 3 reviews
…endpoint/function names
There was a problem hiding this comment.
Great work Helen! It's amazing that the Cornellians Say feature is coming along smoothly with the backend almost finished! I just left a couple comments to just help maintain consistency in our code and database. I'm so excited to see the future frontend development of this feature!
| const [isUpdatingAI, setIsUpdatingAI] = useState(false); | ||
| const [updateStatusAI, setUpdateStatusAI] = useState(0); |
There was a problem hiding this comment.
Once you pull the other two admin button updates from main, you can use the global updating variable and updated variables for the summaries.
| // Call when user selects "Sumarize Reviews" button. Calls endpoint to generate | ||
| // summaries and tags using AI for all courses with a freshness above a certain | ||
| // threshold, then updates those courses to include these new summaries and tags. |
There was a problem hiding this comment.
I think it'd be cleaner if these were docstring comments -- we probably need to update that throughout our codebase too for consistency
| if (isUpdatingAI) { | ||
| return; |
There was a problem hiding this comment.
You shouldn't need to check for this since the new code for disabling buttons should prevent a function call to summarizeReviews() if any button is updating.
| setIsUpdatingAI(true); | ||
| setUpdateStatusAI(1); | ||
|
|
||
| axios.post('/api/ai/update-all-courses') |
There was a problem hiding this comment.
Maybe /api/ai/summary/courses would be a better endpoint name?
Also make this an await statement
| <div hidden={!(updateStatusAI === -1)} className=""> | ||
| <p>Error updating courses with AI.</p> | ||
| </div> |
There was a problem hiding this comment.
I like the error message here! I'm wondering if we could do this for all of our admin page buttons by using a singular error variable?
| for (const courseId of courseIds) { | ||
| const success = await updateCourseWithAI(courseId); | ||
| if (success) { | ||
| results.success.push(courseId); |
There was a problem hiding this comment.
To keep this consistent with our current codebase, you could do something like:
const success = await updateCourseWithAI(courseId);
if (success) {
return res.status(200).json({result: courseId});
}
return res.status(400).json({result: courseId});
And maybe also put the await statement in a try-catch for an error response too (500)
| * returns a message indicating whether classSumary, classTags, and freshness have | ||
| * been updated successfully for the given courseId | ||
| */ | ||
| aiRouter.post('/update-course-summary', async (req, res) => { |
There was a problem hiding this comment.
Endpoint could be /summary/update instead?
There was a problem hiding this comment.
When would we use this endpoint as opposed to the one with update-all? Are there times we may want to target a class to resummarize?
| * @body a course ID | ||
| * returns a block of text containing all reviews from that course | ||
| */ | ||
| aiRouter.post('/get-course-review-text', async (req, res) => { |
There was a problem hiding this comment.
review/text or something similar
There was a problem hiding this comment.
When will this endpoint be called?
| { $set: { classSummary: summary, classTags: tags, freshness: 0 } } | ||
| ); | ||
|
|
||
| console.log(`Course ${courseId} updated successfully with summary and tags.`); |
There was a problem hiding this comment.
Instead of the courseId, use the course subject + number so it's easier to track within Postman.
There was a problem hiding this comment.
This would definitely be helpful for readability purposes. I think something like
const course = await Classes.findOne({ _id: courseId })
const courseName = course.classSub.toUppercase() + ' ' + course.classNum;
could work, and then put that variable in the print statement
| classSummary: { type: String, required: false }, // 50 word summary of the course generated by openAI | ||
| classTags: { | ||
| type: Map, // dictionary keys are lectures, assignments, professors, skills, and resources | ||
| of: [{ type: String }, { type: String }], // tuple where first element is the adjective and second element is its connotation | ||
| required: false | ||
| }, // tags describing lectures, assignments, professors, skills, and resources | ||
| freshness: { type: Number, default: 0 } // number of reviews since last openAI call |
There was a problem hiding this comment.
I'm not sure if it would make sense to put all of this within an AI object instead? Like
AI:
{
classSummary
classTags
freshness
}
or maybe renaming classTags to classTagsReviews will help differentiate between it and the tags for similar courses
There was a problem hiding this comment.
Great work on the AI feature, and it's super cool that the backend is coming along so well. I left a few questions about where specific endpoints would be used -- it seemed to me that /update-all-courses/ would be the main, if only, route that we use, so not sure if the other ones will be as necessary. Happy to chat about my comments, and amazing job!! 🎆
| { role: "system", content: "You are creating a 50 word summary based on the collection of course reviews provided." }, | ||
| { | ||
| role: "system", content: ` | ||
| You are given a collection of course reviews provided where each review is separated by a /. You |
There was a problem hiding this comment.
Maybe our Chat query could be more descriptive: (ex. ... a collection of college course reviews ...) But if we're happy with the current quality of summaries, it's probably not necessary
|
|
||
| const response = completion.choices[0].message.content; | ||
| const summaryMatch = response.match(/Summary: ([\s\S]*?)(?=Tags)/); | ||
| const summary = summaryMatch ? summaryMatch[1].trim() : "Summary not found."; |
There was a problem hiding this comment.
Do we plan to handle in front-end if the summary is not found?
| const adjective = match[2].trim(); | ||
| const connotation = match[3].trim(); | ||
| if (adjective !== "N/A" && connotation != "N/A") { | ||
| tagsObject[category] = [adjective, connotation]; |
There was a problem hiding this comment.
Also here, how does the error handling work in the future if there are less than 5 tags or if we have trouble parsing the tags for a certain class?
| * returns a message summarizing how many courses were updated successfully | ||
| * and how many failed. | ||
| */ | ||
| aiRouter.post('/update-all-courses', async (req, res) => { |
There was a problem hiding this comment.
I think the route name could be a bit clearer, something like /api/ai/summarize-courses/ maybe?
| { $set: { classSummary: summary, classTags: tags, freshness: 0 } } | ||
| ); | ||
|
|
||
| console.log(`Course ${courseId} updated successfully with summary and tags.`); |
There was a problem hiding this comment.
This would definitely be helpful for readability purposes. I think something like
const course = await Classes.findOne({ _id: courseId })
const courseName = course.classSub.toUppercase() + ' ' + course.classNum;
could work, and then put that variable in the print statement
| //get all courses with at least minimum reviews (will be changed later to check for freshness as well) | ||
| const courseIds = await getCoursesWithMinReviews(minReviews); | ||
| if (!courseIds || courseIds.length === 0) { | ||
| return res.status(404).json({ error: `No courses found with at least ${minReviews} reviews.` }); |
There was a problem hiding this comment.
I guess this not error behavior? So could return a "200 OK" status with the message that no courses that meet both our freshness and minimum review count criteria are currently up for resummarizing
| } | ||
|
|
||
| //show how many courses were updated successfully | ||
| const message = `Update completed. ${results.success.length} courses updated successfully. ${results.failed.length} courses failed to update.`; |
There was a problem hiding this comment.
From above where we took the lack of summary or tags at face value and just filled in with placeholder strings, should we consider those here as "failed" updates?
| * returns a message indicating whether classSumary, classTags, and freshness have | ||
| * been updated successfully for the given courseId | ||
| */ | ||
| aiRouter.post('/update-course-summary', async (req, res) => { |
There was a problem hiding this comment.
When would we use this endpoint as opposed to the one with update-all? Are there times we may want to target a class to resummarize?
| * @body a course ID | ||
| * returns a block of text containing all reviews from that course | ||
| */ | ||
| aiRouter.post('/get-course-review-text', async (req, res) => { |
There was a problem hiding this comment.
When will this endpoint be called?
Summary
This PR works on the "Cornellians Say" feature.
PR Type
Mobile + Desktop Screenshots & Recordings
QA - Test Plan
-tested by pressing the button and checking that the database was correctly updated
-tested that the correct messages were being displayed by running different scenarios
Breaking Changes & Notes
-added a button under admin tools that can be pressed to generate and update database with new summaries/tags
-will show a message while the endpoint is running
-will show a success message if successfully updated and an error message if something went wrong
Added to documentation?
What GIF represents this PR?
gif