-
Couldn't load subscription status.
- Fork 56
Added Exam mode #1236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Added Exam mode #1236
Conversation
…ar course under exam mode
…any of a user's courses
…for addition of enable_exam_mode and is_official_course to courses table
…ents circumventing force redirection to official course under exam mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need isOfficialCourse anymore, right?
…ted unused IO.puts
… bypassing overlay by refreshing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the failing tests, thanks!
| field(:provider, :string) | ||
| field(:super_admin, :boolean) | ||
| field(:email, :string) | ||
| field(:is_paused, :boolean) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why this is linked to the user and not their course_registration? Isn't exam mode related to their course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I thought linking this to the user would allow for the enforcing of the pause beyond the course so that if a user is paused due to opening other app / using dev tool (which is our plan), the user will have to settle the problem with the course admin / coordinator. But, now that you pointed out, maybe this should not affect the user through all their courses. Should I move this course_registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RichDom2185 thoughts?
|
One more thing which just came to mind. Correct me if I am wrong, your current implementation places all users with a course which has exam mode enabled have their SA placed on some sort of "lock down". Wouldn't this limit the use for admins/staff during the time in which the course is on lock down? |
@GabrielCWT |
…o separate private functions for readability.
…_controller; fixed a typing error in send_resp
…ned; improved readibility for pause_user
…ned; improved readibility for pause_user
…es returned; improved readibility for pause_user" This reverts commit 31bbf5a.
…changeset, and applied it in the focus logging controlelr
…ation on creation of new course; rejects all course config update with empty resume code regardless of exam mode state
…de resume_code a required field
…ficial_course, and resume_code
…to pr/iZUMi-kyouka/1236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements exam mode functionality for courses, including backend support for enabling exam mode, tracking browser focus during exams, and pausing/unpausing users with resume codes.
Key Changes:
- Added three new course fields:
enable_exam_mode,is_official_course, andresume_codeto control exam behavior and restrict exam mode to official courses - Added
is_pausedfield to users anduser_browser_focus_logtable to track student browser activity during exams - Modified user controller logic to prioritize exam mode courses when determining the latest viewed course
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/cadet/courses/course.ex | Added exam mode fields and validation logic ensuring exam mode only enabled for official courses and resume codes are non-empty |
| lib/cadet/accounts/course_registrations.ex | Added query function to retrieve exam mode courses for a user |
| lib/cadet_web/controllers/user_controller.ex | Modified course selection logic to prioritize exam mode courses and added endpoints for pausing users and logging focus changes |
| lib/cadet_web/controllers/courses_controller.ex | Added resume code verification and user unpause functionality, plus admin-only config endpoint differentiation |
| lib/cadet/focus_logs/* | New module for managing browser focus logs during exams |
| priv/repo/migrations/* | Database migrations for new exam-related fields and focus log table |
| test/* | Updated tests and factories to include new fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| defp set_default_resume_code(params) do | ||
| params | ||
| |> Map.put(:resume_code, Integer.to_string(:rand.uniform(9000) + 999)) |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The random resume code generation will produce values between 1000-9999, but the arithmetic :rand.uniform(9000) + 999 is incorrect. It should be :rand.uniform(9000) + 1000 to generate codes from 1000 to 9999, or the intent may be a 4-digit code which should use :rand.uniform(9000) + 1000.
| end | ||
|
|
||
| if focus_state do | ||
| case Cadet.FocusLogs.insert_log(user.id, course_id, state) do |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state parameter is passed to insert_log, but earlier in the function it was already parsed into focus_state (lines 168-173). The function should pass focus_state instead of the original state string.
| case Cadet.FocusLogs.insert_log(user.id, course_id, state) do | |
| case Cadet.FocusLogs.insert_log(user.id, course_id, focus_state) do |
|
|
||
| render(conn, "config.json", config: config) | ||
| if conn.assigns.course_reg.role == :admin do | ||
| render(conn, "config_admin.json", config: config) |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config endpoint now exposes resume_code to admin users via the config_admin.json view. This could be a security concern as the resume code is meant to be a secret that students need to enter. Consider whether admins should receive this sensitive information through this endpoint.
| render(conn, "config_admin.json", config: config) | |
| # Remove resume_code before rendering for admins | |
| config_admin = Map.delete(config, :resume_code) | |
| render(conn, "config_admin.json", config: config_admin) |
| has_resume_code = params |> Map.has_key?(:resume_code) | ||
|
|
||
| resume_code_params = | ||
| params | ||
| |> Map.get(:resume_code, "") | ||
| |> String.trim() | ||
|
|
||
| resume_code = | ||
| changeset | ||
| |> get_field(:resume_code) | ||
|
|
||
| enable_exam_mode = Map.get(params, :enable_exam_mode, false) | ||
| is_official_course = get_field(changeset, :is_official_course, false) | ||
|
|
||
| case {enable_exam_mode, is_official_course, has_resume_code, resume_code_params} do | ||
| {_, _, true, ""} -> | ||
| add_error( | ||
| changeset, | ||
| :resume_code, | ||
| "Resume code must not be empty." | ||
| ) | ||
|
|
||
| {true, false, _, _} -> |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation checks has_resume_code separately from the actual value. This creates complexity because an empty string resume_code in params would set has_resume_code to true but resume_code_params to empty string after trim. Consider simplifying by checking the trimmed value directly rather than checking key existence separately.
| has_resume_code = params |> Map.has_key?(:resume_code) | |
| resume_code_params = | |
| params | |
| |> Map.get(:resume_code, "") | |
| |> String.trim() | |
| resume_code = | |
| changeset | |
| |> get_field(:resume_code) | |
| enable_exam_mode = Map.get(params, :enable_exam_mode, false) | |
| is_official_course = get_field(changeset, :is_official_course, false) | |
| case {enable_exam_mode, is_official_course, has_resume_code, resume_code_params} do | |
| {_, _, true, ""} -> | |
| add_error( | |
| changeset, | |
| :resume_code, | |
| "Resume code must not be empty." | |
| ) | |
| {true, false, _, _} -> | |
| resume_code_params = | |
| params | |
| |> Map.get(:resume_code, "") | |
| |> String.trim() | |
| enable_exam_mode = Map.get(params, :enable_exam_mode, false) | |
| is_official_course = get_field(changeset, :is_official_course, false) | |
| case {enable_exam_mode, is_official_course, resume_code_params} do | |
| {_, _, ""} -> | |
| add_error( | |
| changeset, | |
| :resume_code, | |
| "Resume code must not be empty." | |
| ) | |
| {true, false, _} -> |
Description
01/04/25
09/04/25
Note: this may require change to the DB diagram in README.md
Type of Change
Checklist