Skip to content

Multiple Reference Timelines: Back-end models, controllers extensions #5491

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

Merged
merged 57 commits into from
Jan 18, 2023

Conversation

purfectliterature
Copy link
Contributor

@purfectliterature purfectliterature commented Dec 15, 2022

Closes #3092.

Warning Merge this PR together with the front-end extensions in #5631! These PRs shall remain as Draft until this feature is ready to ship.

Feature overview

Multiple Reference Timelines (internally abbreviated as MRT) is a feature that allows different students to follow different assignment opening and closing times, thus following different timelines. This feature is useful for teachers who manage different classes that may have different homework deadlines, yet whose students still belong to the same course.

The building blocks for this feature was originally introduced in #3130, but the application-level implementations were deferred and tracked in #3092. This PR, and subsequent ones, aim to complete these deferred works.

For brevity, reference timelines and reference times may be referred to as timelines and times in these documentations. Personal times will still be referred to as is.

Notable changes

  • ℹ️ No structural changes to database schema. Follows the schema implemented in Migration + models for multiple reference timelines #3130.
  • ✈️ One migration: add title and weight columns to course_reference_timelines table.
  • 🔥 Course::LessonPlan::Item#reference_time_for now supports non-default (custom) reference timelines.
  • 🔥 Add validations to Course::ReferenceTime
  • 🔥 Add validations to Course::ReferenceTimeline
  • 🔥 Add CRUD endpoints for Course::ReferenceTime
  • 🔥 Add CRUD endpoints for Course::ReferenceTimeline
  • 🔨 Add model, controller tests, and test helpers for these validations and invariants

Database schema

schema

New routes

  • GET /courses/:id/timelines
  • GET /courses/:id/timelines/:id
  • POST /courses/:id/timelines
  • PATCH /courses/:id/timelines/:id
  • DELETE /courses/:id/timelines/:id
  • POST /courses/:id/timelines/:id/times
  • PATCH /courses/:id/timelines/:id/times/:id
  • DELETE /courses/:id/timelines/:id/times/:id
  • PATCH /courses/:id/users/assign_timeline

Pending

Pending work Tracked in Status
Front-end: Timeline Designer #4202 #5631
Front-end: Assigning students to timelines #4202 #5631
Back-end: Closing reminders to follow custom times #3240 Deferred
Back-end: Address the remaining # TODO(#3092)s #3092 Deferred

todo: to add invariants, sanity checks, and test plan descriptions (tests have already been added)

@purfectliterature purfectliterature added Enhancement Feature Ruby Pull requests that update Ruby code labels Dec 15, 2022
@purfectliterature purfectliterature self-assigned this Dec 15, 2022
@purfectliterature purfectliterature force-pushed the phillmont/multiple-reference-timelines branch 9 times, most recently from 66bfb2f to a6c3f93 Compare December 16, 2022 08:36
Copy link
Member

@ekowidianto ekowidianto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and good work! @purfectliterature
Some nits and notes below:

  • Remove app/mailers/.keep.
  • Add tests to items feature tests (eg submission, video and survey) to ensure that the correct time is selected/shown to a user when the user is assigned to a different timeline.
  • I believe you will still make more changes to the API response so I wont comment much yet on the jbuilder as of now. Note: Remember to pass the permissions to the frontend too.
  • For the case when an item has different/no start_time, end_time and bonus_end_time in the default timeline as compared to those in an alternative timeline, line 22 in app/views/course/assessment/assessments/show.json.jbuilder may break. start_time, end_time, bonus_end_time should be loaded via time_for instead of accessing the value directly from the item (which only loads the default time). I have listed a few other places below to check (May not be complete, please help to check if I miss anything):
    • app/models/course/survey/response.rb line 27
    • app/services/course/assessment/submission/calculate_exp_service.rb line 22 and line 23
    • app/views/course/assessment/assessments/show.json.jbuilder line 22 and line 43
    • app/views/course/survey/surveys/_survey.json.jbuilder line 4 to 6
  • Check AR queries using default_reference_time. Seems like there's a few places that use the default timeline only:
    • app/models/course/video.rb
    • app/models/components/course/surveys_ability_component.rb
    • app/controllers/course/courses_controller.rb
    • app/controllers/course/statistics/aggregate_controller.rb
    • app/models/components/course/surveys_ability_component.rb (TODO(#3092))
  • Remove TODO(#3092) in the course_user model file. It's not relevant anymore.

Note to self:

  • Check object duplication from a course with multiple timelines to another course with single/multiple timelines

def item_is_straggling(personal_time, reference_time)
if reference_time.end_at.present? && personal_time.end_at.present?
reference_time.end_at < personal_time.end_at
elsif reference_time.end_at.present? && personal_time.end_at.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to revisit this.. for unsubmitted items, personal_time might be recomputed (and removed) if the new reference time of a new reference timeline is created with no end_at

@purfectliterature purfectliterature force-pushed the phillmont/multiple-reference-timelines branch from a6c3f93 to 420eab9 Compare December 21, 2022 11:44
@purfectliterature purfectliterature force-pushed the phillmont/multiple-reference-timelines branch 10 times, most recently from 9bd4370 to 44d2dd6 Compare January 16, 2023 02:58
@purfectliterature purfectliterature force-pushed the phillmont/multiple-reference-timelines branch 2 times, most recently from 18c5a41 to 8b99b60 Compare January 16, 2023 07:55
@purfectliterature purfectliterature force-pushed the phillmont/multiple-reference-timelines branch from a45cd39 to d82d829 Compare January 18, 2023 05:18
redux toolkit already re-exports the same `createSelector` from reselect
@ekowidianto ekowidianto marked this pull request as ready for review January 18, 2023 14:54
@ekowidianto ekowidianto merged commit 9a1098b into master Jan 18, 2023
@ekowidianto ekowidianto deleted the phillmont/multiple-reference-timelines branch January 18, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Feature Ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deferred work on supporting multiple reference timelines
2 participants