Integrate Approved Session Tracking Fixes into 10.0.x #566
Conversation
- implement session tracker service with integration test - amend marking session update_session_details method to initalize date time before assigning
b0ink
left a comment
b0ink
left a comment
There was a problem hiding this comment.
Thanks for your contributions, but I don't think we require any of these changes and I have added comments explaining why
If you were looking to optimise how we handle the session duration, it would probably be even better not to expose the duration_minutes from the backend at all, reducing the size of the data sent from the server, and to do the calculation client-side the performance cost is negligible
| t.string :ip_address | ||
| t.datetime :start_time | ||
| t.datetime :end_time | ||
| t.integer :duration_minutes, default: 0 |
There was a problem hiding this comment.
You'll need to start a new migration if you want to make changes to the schema, as this is already shipped in production
| ) | ||
|
|
||
| session.update_session_details | ||
| session.update_session_details if action == 'assessing' |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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
| task_id: task&.id, | ||
| task_definition_id: task&.task_definition_id, | ||
| created_at: Time.zone.now | ||
| created_at: DateTime.now |
3 participants
References:
Approved PR: thoth-tech#83
Unmerged PR: thoth-tech#74
Description
This PR merges the changes from the approved PR #83 into the 10.0.x branch. It ensures the session tracking logic integrated includes schema fixes, environment blockers, and backend improvements previously approved in #83.
This PR directly references the approved work from #83 and makes it available in the target branch for further development and frontend integration.
Fixes identified issues that blocked development and limited the usefulness of the existing schema:
Frontend impact
Persisting duration_minutes enables direct aggregation via the API and removes the need for complex time-delta calculations in the Angular frontend. This change is intended to support(thoth-tech/doubtfire-web#402). By providing this data via the API, we avoid complex time-delta calculations in the Angular frontend, allowing 402 to be refactored to connect directly to this backend logic since the frontend contributions are yet to connect to exisitng backend.
Type of change
How Has This Been Tested?
Test A: Migration Sequence
Executed bundle exec rails db:migrate:redo VERSION=20250922103033.
Verified:
marking_sessionstable is created successfullyduration_minutescolumn existsNo MySQL tablespace errors occur
Test B: Session Tracking Logic
Ran
bundle exec ruby -I test test/services/session_tracker_test.rb.Result: 2 runs, 13 assertions, 0 failures, 0 errors.
Ran
bundle exec rails test test/api/marking_sessions_api_test.rb6 runs, 67 assertions, 0 failures, 0 errors, 0 skips
Confirmed:
15-minute "sticky" session window works
duration_minutesis correctly updated upon activity.Checklist: