Skip to content
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

[#11843] Create FeedbackSessionLog entity and cron job action #12895

Merged
merged 12 commits into from
Mar 18, 2024

Conversation

dishenggg
Copy link
Contributor

@dishenggg dishenggg commented Mar 13, 2024

Part of #11843

Outline of Solution

  • Create FeedbackSessionLog Entity

  • Create UpdateFeedbackSessionLogsAction

    • Processes all logs from the past hour similar to UsageStatistics
    • Ensure that logs of the same type by the same student in the same session are only passed to the logic layer if they are at least 2 seconds apart

@cedricongjh cedricongjh added the s.Ongoing The PR is being worked on by the author(s) label Mar 13, 2024
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

Great and fast work, thanks!

Comment on lines 34 to 35
List<FeedbackSessionLogEntry> logEntries = logsProcessor.getFeedbackSessionLogs(null, null,
startTime.toEpochMilli(), endTime.toEpochMilli(), null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness of this action relies on the ordering added in this method. Would it be better if there's some indication about needing the ordering? I'm thinking getOrderedFeedbackSessionLogs, it's not great, open if there's better suggestions also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm thats a good point, I think your suggestion should work fine since there is no downside to having it be ordered and in the future if there is a need to retrieve ordered logs the method can be easily found

@dishenggg dishenggg marked this pull request as ready for review March 14, 2024 09:30
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM

@dishenggg dishenggg added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 16, 2024
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@cedricongjh cedricongjh merged commit 17866eb into TEAMMATES:student-activity-logs Mar 18, 2024
1 check passed
@ziqing26 ziqing26 added this to the V9.0.0-beta.5 milestone Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToReview The PR is waiting for review(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants