[Feature:InstructorUI] Registration date#13011
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13011 +/- ##
============================================
- Coverage 21.09% 21.09% -0.01%
- Complexity 10115 10119 +4
============================================
Files 279 279
Lines 38167 38180 +13
Branches 585 585
============================================
+ Hits 8051 8053 +2
- Misses 29544 29555 +11
Partials 572 572
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
43fe247 to
c01fc06
Compare
dagemcn
left a comment
dagemcn
left a comment
There was a problem hiding this comment.
I did mostly a code review to start off here, some of my comments could be worth opening up to the group at the meeting to see what their thoughts are. Most of the changes look very solid, I just left comments with a few nitpicks.
| :param database: Object for interacting with given database for environment | ||
| :type database: migrator.db.Database | ||
| """ | ||
| database.execute(""" |
There was a problem hiding this comment.
Is it necessary to store this data in the master and the course database? This provides opportunity for the data the become misaligned between the two databases, which can create bugs.
There was a problem hiding this comment.
Changed it so that it would only fall under course, no longer needed for master.
| 'manual_registration' => false, | ||
| 'preferred_given_name' => "", | ||
| 'preferred_family_name' => "", | ||
| 'preferred_given_name' => null, |
There was a problem hiding this comment.
These changes seem unrelated to this PR.
github-project-automation
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
|
|
||
| /** | ||
| * Updates the date_registered timestamp for a user in a course to the current time. | ||
| * Used for self-registration and self-rejoin, NOT for manual adds/edits by an instructor. |
There was a problem hiding this comment.
While I requested this feature to monitor students in self-registration courses, it is likely useful for all courses. Even if we don't use it for non-self registration in this PR, let's not restrict the future usage by including these comments. We should also open an issue for what future code changes are necessary to use this feature for non-self registration courses.
| else { | ||
| $this->core->getUser()->setRegistrationSection($default_section); | ||
| $this->core->getQueries()->insertCourseUser($this->core->getUser(), $term, $course); | ||
| $this->core->getQueries()->updateRegistrationDate($user_id); |
There was a problem hiding this comment.
Would this be better to be put into updateUser() and insertCourseUser() instead of adding another query?
dagemcn
left a comment
dagemcn
left a comment
There was a problem hiding this comment.
The code here is looking very good, I was able to verify the functionality works as intended. Course instructors are able to see the time students registered in the manage students page, the time remains accessible in the manage students page if the student drops the course, and also remains the accessible and consistent if the student picks up the course again. Also tested with instructor user registering as student and as a TA.
github-project-automation
Bot
moved this from Work in Progress
to Awaiting Maintainer Review
in Submitty Development
github-project-automation
Bot
moved this from Awaiting Maintainer Review
to Done
in Submitty Development
When [PR#13011](Submitty/Submitty#13011) was merged, a new field, `date_registered`, was added to the users table, which rainbow grades parses. This field causes an unknown token error. This PR adds handling for that field, however it is currently unused. To test: 1. Navigate to rainbow grades 2. Check some options 3. Click build 4. Make sure that rainbow grades builds and can be viewed
4 participants
Why is this Change Important & Necessary?
Partially fixes Issue #11547
Submitty currently has no record of when a student self-registered or self-rejoined a course. Instructors have no way to see this information, which can be useful wondering if a student joined the course late or had dropped the course. This PR adds a
date_registeredtimestamp that is recorded whenever a student registers or rejoins a course, and displays it to instructors in the Edit Student modal.What is the New Behavior?
date_registeredcolumn is added to the per-courseuserstable.NOW()) whenever a student self-registersfor a course (
CourseRegistrationController) or self-rejoins acourse (
SelfRejoinController).import process, do not have this field set — it remains
NULL/N/A,since no self-registration event occurred for them.
applicable) as a new read-only field in the Edit Student modal
under Manage Students.
What steps should a reviewer take to reproduce or test the bug or new feature?
run_migrator.py -e course --course <semester> <course> migrate).an existing course).
modal for that student. Confirm "Registration Date" shows a
timestamp.
or via course setup/import (not self-registered). Confirm
"Registration Date" shows "N/A".
SELECT user_id, date_registered FROM users WHERE user_id=' ';
Automated Testing & Documentation
N/A
Other information
Possibly need to implement Cypress E2E testing for this.
Does this PR include migrations to update existing installations?
This PR includes a migration for course databases (
users.date_registered) to update existing installations. This is not a breaking change as the new column defaults to NULL and existing functionality is unaffected.