Skip to content

Integrate Approved Session Tracking Fixes into 10.0.x #566

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.

Sign up for GitHub

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

Closed
rammakablecode wants to merge 4 commits into from
+71 −57
Closed

Integrate Approved Session Tracking Fixes into 10.0.x #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16902fe
feature/session-tracker-service
martindolores Aug 25, 2025
2500b0a
Implement session tracking with duration_minutes and activity logging
rammakablecode Jan 26, 2026
6cce7de
Update marking session tests to align with session tracking logic
rammakablecode Jan 29, 2026
f7e3a18
Merge branch 'feature-session-tracker-v2' into resolve-merge-tutor-an…
rammakablecode Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/models/marking_session.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ def duration_minutes

def update_session_details
now = DateTime.now
update(end_time: now)
if start_time.present?
duration = ((now.to_f - start_time.to_f) / 60).to_i
update(
end_time: now,
duration_minutes: duration
)
else
update(end_time: now, duration_minutes: 0)
end
Comment on lines +18 to +26
Copy link
Member

@b0ink b0ink Feb 4, 2026

Choose a reason for hiding this comment

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

duration_minutes is redundant and should not be part of the schema.
The cost of calculating duration from start_time and end_time is negligible, and storing it would duplicate derived data.

Persisting duration_minutes introduces a risk of data inconsistency - for example, if one value is updated without the other - leading to conflicting sources of truth

We dynamically calculate duration_minutes when we expose the MarkingSession entity

end
end
4 changes: 2 additions & 2 deletions app/services/session_tracker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ 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: Time.zone.now
created_at: DateTime.now
Copy link
Member

@b0ink b0ink Feb 4, 2026

Choose a reason for hiding this comment

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

Time.zone.now should be used

)

session.update_session_details
session.update_session_details if action == 'assessing'
Copy link
Member

@b0ink b0ink Feb 4, 2026

Choose a reason for hiding this comment

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

We want various actions to let the backend know the tutor is still active - this includes adding comments, opening different submissions, etc.

We can't limit this to just the assessing action, because a tutor could - for example - spend 2 hours opening and reviewing submissions, leaving comments, and not change the status of any tasks (triggering the assessing action) - this would result in multiple sessions being created back-to-back once the previous session is older than 15 minutes.


activity
end
Expand Down
1 change: 1 addition & 0 deletions db/migrate/20250922103033_create_marking_sessions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ def change
t.string :ip_address
t.datetime :start_time
t.datetime :end_time
t.integer :duration_minutes, default: 0
Copy link
Member

@b0ink b0ink Feb 4, 2026

Choose a reason for hiding this comment

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

You'll need to start a new migration if you want to make changes to the schema, as this is already shipped in production

t.boolean :during_tutorial

t.timestamps
Expand Down
1 change: 1 addition & 0 deletions db/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@
t.string "ip_address"
t.datetime "start_time"
t.datetime "end_time"
t.integer "duration_minutes", default: 0
t.boolean "during_tutorial"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
Expand Down
112 changes: 58 additions & 54 deletions test/api/marking_sessions_api_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,15 @@ def test_marking_sessions
assert_not last_activity.nil?

# > Force the session to be 10 minutes in the past
travel 10.minutes
travel 5.seconds # Ensure we're past the 10 minute mark
travel 10.minutes + 5.seconds # Ensure we're past the 10 minute mark

# Create a comment as the tutor, ensure activity was created
get "/api/projects/#{project.id}/task_def_id/#{td.id}/submission_details"
# Use 'put' (assessing) because SessionTracker only updates end_time if action == 'assessing'
put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'discuss' }
assert_equal 200, last_response.status

last_session = MarkingSession.last
last_activity = SessionActivity.last
last_session.reload

# Now it should be 10 because the end_time was pushed forward
assert_equal 10, last_session.duration_minutes

# Ensure activity points to the same session
Expand Down Expand Up @@ -263,8 +262,8 @@ def test_get_marking_sessions
create(:marking_session,
user: convenor,
unit: unit,
start_time: date.to_time + rand(0..12).hours,
end_time: date.to_time + rand(13..23).hours,
start_time: date.in_time_zone.change(hour: rand(0..12)),
end_time: date.in_time_zone.change(hour: rand(13..23)),
ip_address: '127.0.0.1')
end
end
Expand Down Expand Up @@ -305,49 +304,49 @@ def test_marking_sessions_tutorial_split
activity_type_id: activity_type.id,
activity_type: activity_type
})
tutorial1 = Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "8:00",
meeting_location: "-",
code: "Tutorial1",
unit_role: unit_role,
abbreviation: "Tutorial1",
tutorial_stream: tutorial_stream
})

tutorial2 = Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "11:00",
meeting_location: "-",
code: "Tutorial2",
unit_role: unit_role,
abbreviation: "Tutorial2",
tutorial_stream: tutorial_stream
})

tutorial3 = Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "13:00",
meeting_location: "-",
code: "Tutorial3",
unit_role: unit_role,
abbreviation: "Tutorial3",
tutorial_stream: tutorial_stream
})

tutorial4 = Tutorial.create!({
unit: unit,
meeting_day: "Thursday",
meeting_time: "12:00",
meeting_location: "-",
code: "Tutorial4",
unit_role: unit_role,
abbreviation: "Tutorial4",
tutorial_stream: tutorial_stream
})
Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "8:00",
meeting_location: "-",
code: "Tutorial1",
unit_role: unit_role,
abbreviation: "Tutorial1",
tutorial_stream: tutorial_stream
})

Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "11:00",
meeting_location: "-",
code: "Tutorial2",
unit_role: unit_role,
abbreviation: "Tutorial2",
tutorial_stream: tutorial_stream
})

Tutorial.create!({
unit: unit,
meeting_day: "Wednesday",
meeting_time: "13:00",
meeting_location: "-",
code: "Tutorial3",
unit_role: unit_role,
abbreviation: "Tutorial3",
tutorial_stream: tutorial_stream
})

Tutorial.create!({
unit: unit,
meeting_day: "Thursday",
meeting_time: "12:00",
meeting_location: "-",
code: "Tutorial4",
unit_role: unit_role,
abbreviation: "Tutorial4",
tutorial_stream: tutorial_stream
})

MarkingSession.delete_all

Expand All @@ -360,7 +359,7 @@ def test_marking_sessions_tutorial_split
start_time = wednesday.in_time_zone.change(hour: 5, min: 4) # 11:05am
end_time = wednesday.in_time_zone.change(hour: 18, min: 0) # 3:00pm

current_time = start_time
current_time = start_time + 0.seconds

# unit = Unit.first
project = unit.projects.first
Expand Down Expand Up @@ -409,8 +408,13 @@ def test_marking_sessions_tutorial_split
current_time += 5.minutes
end

start_time = thursday.to_time.change(hour: 17, min: 0) # 12:05pm
end_time = thursday.to_time.change(hour: 18, min: 0) # 3:00pm
# Ensure the next activity MUST create a new one, closing previous one.
MarkingSession.last.update(end_time: thursday.in_time_zone.change(hour: 15, min: 1))

travel 2.hours # ensure testing on a new session

start_time = thursday.in_time_zone.change(hour: 17, min: 0)
end_time = thursday.in_time_zone.change(hour: 18, min: 0)

current_time = start_time

Expand Down
Loading