refactor: fix timezones for marking sessions (doubtfire-lms#524)

* refactor: accept requested timezone from client to filter sessions for

* fix: get correct day

* fix: default timezone if nil

* chore: remove comments

Author: b0ink

app/api/marking_sessions_api.rb

@@ -17,6 +17,7 @@ class MarkingSessionsApi < Grape::API
     requires :unit_id, type: Integer, desc: "Unit ID"
     optional :start_date, type: Date, desc: "Start date for analytics"
     optional :end_date, type: Date, desc: "End date for analytics"
+    optional :timezone, type: String, desc: "Requested timezone to search sessions for"
   end
   get '/units/:unit_id/marking_sessions' do
     unit = Unit.find(params[:unit_id])
@@ -25,8 +26,20 @@ class MarkingSessionsApi < Grape::API
       error!({ error: "You are not authorised to view marking sessions for this unit" }, 403)
     end
 
-    end_date = (params[:end_date] || Time.zone.today).end_of_day
-    start_date = (params[:start_date] || (end_date - 7.days)).beginning_of_day
+    tz = Time.zone
+    tz = ActiveSupport::TimeZone[params[:timezone]] if params[:timezone]
+
+    end_date = if params[:end_date].present?
+                 tz.parse(params[:end_date].to_s).end_of_day
+               else
+                 tz.today.end_of_day
+               end
+
+    start_date = if params[:start_date].present?
+                   tz.parse(params[:start_date].to_s).beginning_of_day
+                 else
+                   (end_date - 7.days).beginning_of_day
+                 end
 
     sessions = MarkingSession
                .includes(:session_activities)

app/api/units_api.rb

@@ -391,6 +391,7 @@ class UnitsApi < Grape::API
   params do
     optional :start_date, type: Date, desc: 'Filter sessions starting from this date'
     optional :end_date, type: Date, desc: 'Filter sessions up to this date'
+    optional :timezone, type: String, desc: 'Requested timezone to search sessions for'
     optional :ignore_sessions_during_tutorials, type: Boolean, desc: 'Filter out sessions that occured during tutorials'
   end
   get '/csv/units/:id/tutor_times_summary' do
@@ -405,6 +406,7 @@ class UnitsApi < Grape::API
       unit.id,
       params[:start_date]&.to_s,
       params[:end_date]&.to_s,
+      params[:timezone]&.to_s,
       ignore_sessions_during_tutorials
     )
 

app/models/unit.rb

@@ -2723,10 +2723,27 @@ def move_files_to_archive
     end
   end
 
-  def get_tutor_times(start_date: nil, end_date: nil, ignore_sessions_during_tutorials: false)
+  def get_tutor_times(start_date: nil, end_date: nil, timezone: nil, ignore_sessions_during_tutorials: false)
     query = MarkingSession.where(unit_id: id)
-    query = query.where('start_time >= ?', start_date.beginning_of_day) if start_date
-    query = query.where('start_time <= ?', end_date.end_of_day) if end_date
+
+
+    tz = Time.zone
+    tz = ActiveSupport::TimeZone[timezone] if timezone
+
+    end_date = if end_date.present?
+             tz.parse(end_date.to_s).end_of_day
+               else
+             tz.today.end_of_day
+               end
+
+    start_date = if start_date.present?
+                   tz.parse(start_date.to_s).beginning_of_day
+                 else
+                   (end_date - 7.days).beginning_of_day
+                 end
+
+    query = query.where(start_time: start_date..end_date)
+
 
     query = query.where(during_tutorial: false) if ignore_sessions_during_tutorials
 
@@ -2782,8 +2799,8 @@ def get_tutor_times(start_date: nil, end_date: nil, ignore_sessions_during_tutor
     end
   end
 
-  def get_tutor_times_csv(start_date: nil, end_date: nil, ignore_sessions_during_tutorials: false)
-    summary = get_tutor_times(start_date: start_date, end_date: end_date, ignore_sessions_during_tutorials: ignore_sessions_during_tutorials)
+  def get_tutor_times_csv(start_date: nil, end_date: nil, timezone: nil, ignore_sessions_during_tutorials: false)
+    summary = get_tutor_times(start_date: start_date, end_date: end_date, timezone: timezone, ignore_sessions_during_tutorials: ignore_sessions_during_tutorials)
 
     CSV.generate() do |csv|
       # Add headers

app/services/session_tracker.rb

@@ -18,7 +18,7 @@ def self.record_assessment_activity(action:, user:, project:, ip_address:, task:
       project_id: project.id,
       task_id: task&.id,
       task_definition_id: task&.task_definition_id,
-      created_at: DateTime.now
+      created_at: Time.zone.now
     )
 
     session.update_session_details

app/sidekiq/download_unit_tutor_times_summary_job.rb

@@ -14,7 +14,7 @@ class DownloadUnitTutorTimesSummaryJob
                   on_conflict: :reject,
                   retry: false
 
-  def perform(unit_id, start_date, end_date, ignore_sessions_during_tutorials)
+  def perform(unit_id, start_date, end_date, timezone, ignore_sessions_during_tutorials)
     start_date = Date.parse(start_date) if start_date
     end_date   = Date.parse(end_date) if end_date
     logger.info "Starting unit tutor times summary csv download..."
@@ -23,7 +23,7 @@ def perform(unit_id, start_date, end_date, ignore_sessions_during_tutorials)
     total(1)
 
     unit = Unit.find(unit_id)
-    csv = unit.get_tutor_times_csv(start_date: start_date, end_date: end_date, ignore_sessions_during_tutorials: ignore_sessions_during_tutorials)
+    csv = unit.get_tutor_times_csv(start_date: start_date, end_date: end_date, timezone: timezone, ignore_sessions_during_tutorials: ignore_sessions_during_tutorials)
 
     store(result: csv)
 

test/api/marking_sessions_api_test.rb

@@ -274,13 +274,8 @@ def test_get_marking_sessions
 
   def test_marking_sessions_tutorial_split
     unit = FactoryBot.create(:unit, student_count: 2, task_count: 2)
-    # convenor = FactoryBot.create(:user, :convenor)
     unit_role = unit.staff.first
     convenor = unit_role.user
-    # convenor = user
-    # unit_role = unit.employ_staff(convenor, Role.convenor)
-
-    # add_auth_header_for(user: convenor)
 
     activity_type = ActivityType.find_by(name: "Feedback")
     activity_type = ActivityType.create(name: "Feedback", abbreviation: "Feedback") if activity_type.nil?
@@ -375,8 +370,7 @@ def test_marking_sessions_tutorial_split
     assert_equal expected_marking_sessions_during_tutorial, MarkingSession.where(during_tutorial: true).count
     assert_equal expected_marking_sessions_outside_tutorial, MarkingSession.where(during_tutorial: false).count
 
-    thursday_offset = (today.wday - 4) % 7
-    thursday = today - thursday_offset.days
+    thursday = wednesday + 1.day
 
     start_time = thursday.in_time_zone.change(hour: 12, min: 4) # 12:05pm
     end_time   = thursday.in_time_zone.change(hour: 15, min: 0) # 3:00pm
@@ -450,8 +444,8 @@ def test_marking_session_csv_data
       )
     end
 
-    summary_without_tutorials = unit.get_tutor_times(start_date: Time.zone.now - 1.week, end_date: Time.zone.now + 1.week, ignore_sessions_during_tutorials: true)
-    summary_with_tutorials = unit.get_tutor_times(start_date: Time.zone.now - 1.week, end_date: Time.zone.now + 1.week, ignore_sessions_during_tutorials: false)
+    summary_without_tutorials = unit.get_tutor_times(start_date: Time.zone.now - 1.week, end_date: Time.zone.now + 1.week, timezone: Time.zone, ignore_sessions_during_tutorials: true)
+    summary_with_tutorials = unit.get_tutor_times(start_date: Time.zone.now - 1.week, end_date: Time.zone.now + 1.week, timezone: Time.zone, ignore_sessions_during_tutorials: false)
 
     # 10 sessions, 30 minutes each
     assert_equal 300, summary_without_tutorials.first[:total_minutes]