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

Add live feedback storage and statistics #7497

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
019dc23
refactor(main-statistics): add boolean to indicate if live feedback i…
syoopie Sep 3, 2024
83d1f59
feat(live-feedback): add tables for storing live feeedback information
syoopie Aug 15, 2024
7ede67f
feat(live-feedback-stats): add backend
syoopie Sep 3, 2024
c943385
refactor(statistics): classNameUtils
syoopie Aug 15, 2024
d89b6b5
feat(live-feedback-stats): add frontend
syoopie Sep 4, 2024
6b1e38b
style(generate-live-feedback): remove unnecessary commented code
syoopie Aug 16, 2024
940e65f
feat(live-feedback): add live feedback saving
syoopie Sep 3, 2024
00d3ad9
refactor(submission-controller): disable file length restrictions
syoopie Aug 22, 2024
e303b8e
refactor(EditorField): add optional cursorStart variable
syoopie Aug 27, 2024
8501a38
refactor(live-feedback-stats): add retrieval of question ids
syoopie Aug 27, 2024
ba5e6f7
feat(live-feedback-history): add FE and BE
syoopie Sep 3, 2024
0509bd2
test(live-feedback-history): add tests for live feedback history
syoopie Aug 28, 2024
a6e05d6
refactor(assessments-factory)
syoopie Aug 28, 2024
4be98c7
test(live-feedback-statistics) add tests for live feedback statistics
syoopie Aug 28, 2024
6a9f0ea
refactor(assessment-statistics): abstract translations
syoopie Sep 3, 2024
082bd40
style(live-feedback): change translations
syoopie Sep 11, 2024
01fee01
style(live-feedback-history): improve slider styling
syoopie Sep 11, 2024
01ff5f3
style(assessment-statistics): use new custom slider
syoopie Sep 11, 2024
c85a081
perf(live-feedback-stats): reduce N+1
syoopie Sep 12, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# frozen_string_literal: true
module Course::Statistics::LiveFeedbackConcern
private

def find_submitter_course_user(submission)
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
submission.creator.course_users.find { |u| u.course_id == @assessment.course_id }
end

def find_live_feedback_course_user(live_feedback)
live_feedback.creator.course_users.find { |u| u.course_id == @assessment.course_id }
end

def initialize_feedback_count
Array.new(@question_order_hash.size, 0)
end

def find_user_assessment_live_feedback(submitter_course_user, assessment_live_feedbacks)
assessment_live_feedbacks.select do |live_feedback|
find_live_feedback_course_user(live_feedback) == submitter_course_user
end
end

def update_feedback_count(feedback_count, user_assessment_live_feedback)
user_assessment_live_feedback.each do |feedback|
index = @ordered_questions.index(feedback.question_id)
feedback.code.each do |code|
unless code.comments.empty?
feedback_count[index] += 1
break
end
end
end
end

def initialize_student_hash(students)
students.to_h { |student| [student, nil] }
end

def fetch_hash_for_live_feedback_assessment(submissions, assessment_live_feedbacks)
students = @all_students
student_hash = initialize_student_hash(students)

populate_hash(submissions, student_hash, assessment_live_feedbacks)
student_hash
end

def populate_hash(submissions, student_hash, assessment_live_feedbacks)
submissions.each do |submission|
submitter_course_user = find_submitter_course_user(submission)
next unless submitter_course_user&.student?

feedback_count = initialize_feedback_count

user_assessment_live_feedback = find_user_assessment_live_feedback(submitter_course_user,
assessment_live_feedbacks)

update_feedback_count(feedback_count, user_assessment_live_feedback)

student_hash[submitter_course_user] = [submission, feedback_count]
end
end
end
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# frozen_string_literal: true
class Course::Assessment::Submission::SubmissionsController < \
class Course::Assessment::Submission::SubmissionsController < # rubocop:disable Metrics/ClassLength
Course::Assessment::Submission::Controller
include Course::Assessment::Submission::SubmissionsControllerServiceConcern
include Signals::EmissionConcern
Expand Down Expand Up @@ -102,9 +102,41 @@
response_status, response_body = @answer.generate_live_feedback
response_body['feedbackUrl'] = ENV.fetch('CODAVERI_URL')

live_feedback = Course::Assessment::LiveFeedback.create_with_codes(

Check warning on line 105 in app/controllers/course/assessment/submission/submissions_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/assessment/submission/submissions_controller.rb#L105

Added line #L105 was not covered by tests
@submission.assessment_id,
@answer.question_id,
@submission.creator,
response_body['transactionId'],
@answer.actable.files
)

if response_status == 200
Copy link
Contributor

Choose a reason for hiding this comment

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

in calling the controller generate_live_feedback, when the response_status is 200, you did the calling of save_live_feedback on the Frontend, but also call that functionality here. Why do you need the calling of save_live_feedback twice? Isn't once enough (either call it from frontend, or call it here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because when response status is 200, live feedback is immediately available to be saved. FE wont call save_live_feedback. The live feedback is sent directly to FE from BE and frontend does no polling

Copy link
Contributor

Choose a reason for hiding this comment

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

But when I try to generate the live feedback in the frontend (by clicking Get Help), there will be a network call for save_live_feedback.

If when the response status is 200 the live feedback is immediately available, I think there should not be another call of save_live_feedback from FE; it should be silently saved in BE.

Copy link
Contributor

Choose a reason for hiding this comment

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

@syoopie please attend to this review. Btw, just now I tried to comment out that line save_live_feedback and the API in generating live feedback still runs as expected (when you try to generate live feedback, that generated feedback will be saved accordingly to our DB and the count is increased by 1)

Therefore, please investigate once more if the line 116 is needed here.

params[:live_feedback_id] = live_feedback.id
params[:feedback_files] = response_body['data']['feedbackFiles']
save_live_feedback

Check warning on line 116 in app/controllers/course/assessment/submission/submissions_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/assessment/submission/submissions_controller.rb#L113-L116

Added lines #L113 - L116 were not covered by tests
end

response_body['liveFeedbackId'] = live_feedback.id

Check warning on line 119 in app/controllers/course/assessment/submission/submissions_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/assessment/submission/submissions_controller.rb#L119

Added line #L119 was not covered by tests
render json: response_body, status: response_status
end

def save_live_feedback
live_feedback = Course::Assessment::LiveFeedback.find_by(id: params[:live_feedback_id])
return head :bad_request if live_feedback.nil?

Check warning on line 125 in app/controllers/course/assessment/submission/submissions_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/assessment/submission/submissions_controller.rb#L124-L125

Added lines #L124 - L125 were not covered by tests

feedback_files = params[:feedback_files]
feedback_files.each do |file|
filename = file[:path]
file[:feedbackLines].each do |feedback_line|
Course::Assessment::LiveFeedbackComment.create(

Check warning on line 131 in app/controllers/course/assessment/submission/submissions_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/assessment/submission/submissions_controller.rb#L127-L131

Added lines #L127 - L131 were not covered by tests
code_id: live_feedback.code.find_by(filename: filename).id,
line_number: feedback_line[:linenum],
comment: feedback_line[:feedback]
)
end
end
end

# Reload the current answer or reset it, depending on parameters.
# current_answer has the most recent copy of the answer.
def reload_answer
Expand Down
67 changes: 65 additions & 2 deletions app/controllers/course/statistics/assessments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ class Course::Statistics::AssessmentsController < Course::Statistics::Controller
include Course::UsersHelper
include Course::Statistics::SubmissionsConcern
include Course::Statistics::UsersConcern
include Course::Statistics::LiveFeedbackConcern

def main_statistics
@assessment = Course::Assessment.where(id: assessment_params[:id]).
calculated(:maximum_grade, :question_count).
includes(programming_questions: [:language]).
preload(course: :course_users).first
submissions = Course::Assessment::Submission.where(assessment_id: assessment_params[:id]).
calculated(:grade, :grader_ids).
Expand Down Expand Up @@ -35,8 +37,36 @@ def ancestor_statistics
@student_submissions_hash = fetch_hash_for_ancestor_assessment(submissions, @all_students).compact
end

def live_feedback_statistics
@assessment = Course::Assessment.where(id: assessment_params[:id]).
calculated(:question_count).
preload(course: :course_users).first
submissions = Course::Assessment::Submission.where(assessment_id: assessment_params[:id]).
preload(creator: :course_users)
assessment_live_feedbacks = Course::Assessment::LiveFeedback.where(assessment_id: assessment_params[:id]).
preload(:creator, creator: :course_users, code: :comments)

@course_users_hash = preload_course_users_hash(@assessment.course)

load_course_user_students_info
load_ordered_questions

@student_live_feedback_hash = fetch_hash_for_live_feedback_assessment(submissions,
assessment_live_feedbacks)
end

def live_feedback_history
fetch_live_feedbacks
@live_feedback_details_hash = build_live_feedback_details_hash(@live_feedbacks)
@question = Course::Assessment::Question.find(params[:question_id])
end

private

def load_ordered_questions
@ordered_questions = create_question_order_hash.keys.sort_by { |question_id| @question_order_hash[question_id] }
end

def assessment_params
params.permit(:id)
end
Expand All @@ -59,11 +89,44 @@ def fetch_all_ancestor_assessments
end

def create_question_related_hash
create_question_order_hash
@question_hash = @assessment.questions.to_h do |q|
[q.id, [q.maximum_grade, q.question_type, q.auto_gradable?]]
end
end

def create_question_order_hash
@question_order_hash = @assessment.question_assessments.to_h do |q|
[q.question_id, q.weight]
end
@question_hash = @assessment.questions.to_h do |q|
[q.id, [q.maximum_grade, q.question_type, q.auto_gradable?]]
end

def fetch_live_feedbacks
user = current_course.course_users.find_by(id: params[:course_user_id]).user
@live_feedbacks = Course::Assessment::LiveFeedback.where(assessment_id: assessment_params[:id],
creator: user,
question_id: params[:question_id]).
order(created_at: :asc).includes(:code, code: :comments)
end

def build_live_feedback_details_hash(live_feedbacks)
live_feedbacks.each_with_object({}) do |live_feedback, hash|
hash[live_feedback.id] = live_feedback.code.map do |code|
{
code: {
id: code.id,
filename: code.filename,
content: code.content
},
comments: code.comments.map do |comment|
{
id: comment.id,
line_number: comment.line_number,
comment: comment.comment
}
end
}
end
end
end
end
2 changes: 2 additions & 0 deletions app/models/course/assessment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class Course::Assessment < ApplicationRecord
inverse_of: :assessment, dependent: :destroy
has_one :duplication_traceable, class_name: 'DuplicationTraceable::Assessment',
inverse_of: :assessment, dependent: :destroy
has_many :live_feedbacks, class_name: 'Course::Assessment::LiveFeedback',
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
inverse_of: :assessment, dependent: :destroy

validate :tab_in_same_course
validate :selected_test_type_for_grading
Expand Down
3 changes: 2 additions & 1 deletion app/models/course/assessment/assessment_ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ def allow_read_material
def allow_create_assessment_submission
can :create, Course::Assessment::Submission,
experience_points_record: { course_user: { user_id: user.id } }
can [:update, :generate_live_feedback], Course::Assessment::Submission, assessment_submission_attempting_hash(user)
can [:update, :generate_live_feedback, :save_live_feedback], Course::Assessment::Submission,
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
assessment_submission_attempting_hash(user)
end

def allow_update_own_assessment_answer
Expand Down
38 changes: 38 additions & 0 deletions app/models/course/assessment/live_feedback.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# frozen_string_literal: true
class Course::Assessment::LiveFeedback < ApplicationRecord
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
belongs_to :assessment, class_name: 'Course::Assessment', foreign_key: 'assessment_id', inverse_of: :live_feedbacks
belongs_to :question, class_name: 'Course::Assessment::Question', foreign_key: 'question_id',
inverse_of: :live_feedbacks
has_many :code, class_name: 'Course::Assessment::LiveFeedbackCode', foreign_key: 'feedback_id',
inverse_of: :feedback, dependent: :destroy

validates :assessment, presence: true
validates :question, presence: true
validates :creator, presence: true

def self.create_with_codes(assessment_id, question_id, user, feedback_id, files)
live_feedback = new(
assessment_id: assessment_id,
question_id: question_id,
creator: user,
feedback_id: feedback_id
)

if live_feedback.save
files.each do |file|
live_feedback_code = Course::Assessment::LiveFeedbackCode.new(
feedback_id: live_feedback.id,
filename: file.filename,
content: file.content
)
unless live_feedback_code.save
Rails.logger.error "Failed to save live_feedback_code: #{live_feedback_code.errors.full_messages.join(', ')}"
end
end
live_feedback
else
Rails.logger.error "Failed to save live_feedback: #{live_feedback.errors.full_messages.join(', ')}"
nil
end
end
end
10 changes: 10 additions & 0 deletions app/models/course/assessment/live_feedback_code.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
class Course::Assessment::LiveFeedbackCode < ApplicationRecord
self.table_name = 'course_assessment_live_feedback_code'
belongs_to :feedback, class_name: 'Course::Assessment::LiveFeedback', foreign_key: 'feedback_id', inverse_of: :code
has_many :comments, class_name: 'Course::Assessment::LiveFeedbackComment', foreign_key: 'code_id',
dependent: :destroy, inverse_of: :code

validates :filename, presence: true
validates :content, presence: true
end
7 changes: 7 additions & 0 deletions app/models/course/assessment/live_feedback_comment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
class Course::Assessment::LiveFeedbackComment < ApplicationRecord
belongs_to :code, class_name: 'Course::Assessment::LiveFeedbackCode', foreign_key: 'code_id', inverse_of: :comments

validates :line_number, presence: true
validates :comment, presence: true
end
4 changes: 3 additions & 1 deletion app/models/course/assessment/question.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class Course::Assessment::Question < ApplicationRecord
has_many :question_bundle_questions, class_name: 'Course::Assessment::QuestionBundleQuestion',
foreign_key: :question_id, dependent: :destroy, inverse_of: :question
has_many :question_bundles, through: :question_bundle_questions, class_name: 'Course::Assessment::QuestionBundle'
has_many :live_feedbacks, class_name: 'Course::Assessment::LiveFeedback',
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
dependent: :destroy, inverse_of: :question

delegate :to_partial_path, to: :actable
delegate :question_type, to: :actable
Expand All @@ -38,7 +40,7 @@ class Course::Assessment::Question < ApplicationRecord
#
# @return [Boolean] True if the question supports auto grading.
def auto_gradable?
actable.present? && actable.self_respond_to?(:auto_gradable?) ? actable.auto_gradable? : false
(actable.present? && actable.self_respond_to?(:auto_gradable?)) ? actable.auto_gradable? : false
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
end

# Gets an instance of the auto grader suitable for use with this question.
Expand Down
4 changes: 2 additions & 2 deletions app/models/course/assessment/question/programming.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,8 @@ def validate_codaveri_question

# TODO: Move this validation logic to frontend, to prevent user from submitting in the first place.
if !CodaveriAsyncApiService.language_valid_for_codaveri?(language)
errors.add(:base, 'Language type must be Python 3 and above to activate either codaveri '\
'evaluator or get help')
errors.add(:base, 'Language type must be Python 3 and above to activate either codaveri ' \
'evaluator or live feedback')
elsif !question_assessments.empty? &&
!question_assessments.first.assessment.course.component_enabled?(Course::CodaveriComponent)
errors.add(:base,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,14 @@ def self.language_from_locale(locale)
# @param [Course::Assessment::Answer] answer The answer to be graded.
# @return [Course::Assessment::Answer] The graded answer. Note that this answer is not persisted
# yet.
def construct_feedback_object # rubocop:disable Metrics/AbcSize
def construct_feedback_object
bivanalhar marked this conversation as resolved.
Show resolved Hide resolved
return unless @question.codaveri_id

@answer_object[:problemId] = @question.codaveri_id

@answer_object[:languageVersion][:language] = @question.polyglot_language_name
@answer_object[:languageVersion][:version] = @question.polyglot_language_version

# @answer_object[:is_only_itsp] = true if @course.codaveri_itsp_enabled?

@answer_files.each do |file|
file_template = default_codaveri_student_file_template
file_template[:path] = file.filename
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true
json.files @live_feedback_details_hash[live_feedback_id].each do |live_feedback_details|
json.id live_feedback_details[:code][:id]
json.filename live_feedback_details[:code][:filename]
json.content live_feedback_details[:code][:content]
json.language @question.specific.language[:name]
json.comments live_feedback_details[:comments].map do |comment|
json.lineNumber comment[:line_number]
json.comment comment[:comment]
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# frozen_string_literal: true
json.liveFeedbackHistory do
json.array! @live_feedbacks.map do |live_feedback|
json.id live_feedback.id
json.createdAt live_feedback.created_at&.iso8601
json.partial! 'live_feedback_history_details', live_feedback_id: live_feedback.id
end
end

json.question do
json.id @question.id
json.title @question.title
json.description format_ckeditor_rich_text(@question.description)
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true
json.array! @student_live_feedback_hash.each do |course_user, (submission, live_feedback_count)|
json.partial! 'course_user', course_user: course_user
if submission.nil?
json.workflowState 'unstarted'
else
json.workflowState submission.workflow_state
end

json.groups @group_names_hash[course_user.id] do |name|
json.name name
end

json.liveFeedbackCount live_feedback_count
json.questionIds(@question_order_hash.keys.sort_by { |key| @question_order_hash[key] })
end
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ json.assessment do
json.partial! 'assessment', assessment: @assessment, course: current_course
json.isAutograded @assessment_autograded
json.questionCount @assessment.question_count
json.liveFeedbackEnabled @assessment.programming_questions.any?(&:live_feedback_enabled)
end

json.submissions @student_submissions_hash.each do |course_user, (submission, answers, end_at)|
Expand Down
Loading