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.

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

Enhance Export Functionality with PLD and GDU Metrics #2250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enhance Export Functionality with PLD and GDU Metrics #2250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
580adc2
PLD+GDU_try0
RongYYYY Dec 8, 2024
bd58ac2
Update assessments_controller.rb
RongYYYY Dec 9, 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
14 changes: 14 additions & 0 deletions app/controllers/assessments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,10 @@ def getAssessmentVariable(key)
def export
dir_path = @course.directory_path.to_s
asmt_dir = @assessment.name

pld_data = @assessment.calculate_pld.to_yaml
gdu_data = @assessment.calculate_gdu.to_yaml
Comment on lines +492 to +494
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for PLD and GDU calculations.

The calls to calculate_pld and calculate_gdu should be wrapped in error handling to gracefully handle potential calculation failures.

-    pld_data = @assessment.calculate_pld.to_yaml
-    gdu_data = @assessment.calculate_gdu.to_yaml
+    begin
+      pld_data = @assessment.calculate_pld.to_yaml
+      gdu_data = @assessment.calculate_gdu.to_yaml
+    rescue StandardError => e
+      flash[:error] = "Unable to calculate PLD/GDU metrics -- #{e.message}"
+      redirect_to action: "index"
+      return
+    end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pld_data = @assessment.calculate_pld.to_yaml
gdu_data = @assessment.calculate_gdu.to_yaml
begin
pld_data = @assessment.calculate_pld.to_yaml
gdu_data = @assessment.calculate_gdu.to_yaml
rescue StandardError => e
flash[:error] = "Unable to calculate PLD/GDU metrics -- #{e.message}"
redirect_to action: "index"
return
end


begin
# Update the assessment config YAML file.
@assessment.dump_yaml
Expand All @@ -500,6 +504,16 @@ def export
tar.mkdir asmt_dir, File.stat(File.join(dir_path, asmt_dir)).mode
filter = [@assessment.handin_directory_path]
@assessment.load_dir_to_tar(dir_path, asmt_dir, tar, filter)

# Add PLD and GDU data to the tarball as separate files
tar.add_file_simple("PLD.yml", 0644, pld_data.bytesize) do |io|
io.write pld_data
end

tar.add_file_simple("GDU.yml", 0644, gdu_data.bytesize) do |io|
io.write gdu_data
end

end
tarStream.rewind
tarStream.close
Expand Down
27 changes: 27 additions & 0 deletions app/models/assessment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,33 @@ def date_to_s(date)
date.strftime("%a, %b %e at %l:%M%P")
end

# Public method to access the PLD data
def calculate_pld
submissions.includes(:scores).map do |submission|
late_days = [(submission.created_at.to_date - due_at.to_date).to_i, 0].max
{ submission_id: submission.id, late_days: late_days }
end
end
Comment on lines +467 to +473
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider handling edge cases and enriching the PLD data

The implementation could be enhanced in the following ways:

  1. Handle edge cases like nil due dates
  2. Consider assessment extensions in late day calculations
  3. Include grace days information
  4. Add submission metadata like student info and timestamps
 def calculate_pld
-  submissions.includes(:scores).map do |submission|
-    late_days = [(submission.created_at.to_date - due_at.to_date).to_i, 0].max
-    { submission_id: submission.id, late_days: late_days }
+  raise "Assessment due date not set" if due_at.nil?
+  
+  submissions.includes(:scores, :course_user_datum).map do |submission|
+    extension = extensions.find_by(course_user_datum_id: submission.course_user_datum_id)
+    effective_due_at = extension ? extension.due_at : due_at
+    late_days = [(submission.created_at.to_date - effective_due_at.to_date).to_i, 0].max
+    {
+      submission_id: submission.id,
+      student_email: submission.course_user_datum.user.email,
+      submitted_at: submission.created_at,
+      late_days: late_days,
+      grace_days_used: submission.grace_days_used || 0,
+      extension_days: extension ? (extension.due_at.to_date - due_at.to_date).to_i : 0
+    }
   end
 end

Committable suggestion skipped: line range outside the PR's diff.


# Public method to access the GDU data
def calculate_gdu
# Define grade boundaries
grade_boundaries = { 'A': 90, 'B': 80, 'C': 70, 'D': 60, 'F': 0 }
# Initialize distribution
distribution = Hash.new(0)

total_submissions = submissions.joins(:scores).count
submissions.includes(:scores).each do |submission|
# Assuming 'score' is a method that sums up scores for a submission
score = submission.scores.sum(&:points)
grade = grade_boundaries.keys.reverse.detect { |grade| score >= grade_boundaries[grade] }
distribution[grade] += 1
end
Comment on lines +482 to +488
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix variable shadowing and add error handling

There are several issues to address:

  1. The grade variable on line 486 shadows the outer variable
  2. Need to handle edge case of no submissions
  3. Need to handle nil scores
-    total_submissions = submissions.joins(:scores).count
-    submissions.includes(:scores).each do |submission|
-      # Assuming 'score' is a method that sums up scores for a submission
-      score = submission.scores.sum(&:points)
-      grade = grade_boundaries.keys.reverse.detect { |grade| score >= grade_boundaries[grade] }
-      distribution[grade] += 1
+    submissions_with_scores = submissions.includes(:scores).select { |s| s.scores.any? }
+    total_submissions = submissions_with_scores.length
+    
+    return {} if total_submissions.zero?
+    
+    submissions_with_scores.each do |submission|
+      score = submission.scores.sum(&:points)
+      letter_grade = grade_boundaries.keys.reverse.detect { |g| score >= grade_boundaries[g] }
+      distribution[letter_grade] += 1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
total_submissions = submissions.joins(:scores).count
submissions.includes(:scores).each do |submission|
# Assuming 'score' is a method that sums up scores for a submission
score = submission.scores.sum(&:points)
grade = grade_boundaries.keys.reverse.detect { |grade| score >= grade_boundaries[grade] }
distribution[grade] += 1
end
submissions_with_scores = submissions.includes(:scores).select { |s| s.scores.any? }
total_submissions = submissions_with_scores.length
return {} if total_submissions.zero?
submissions_with_scores.each do |submission|
score = submission.scores.sum(&:points)
letter_grade = grade_boundaries.keys.reverse.detect { |g| score >= grade_boundaries[g] }
distribution[letter_grade] += 1
🧰 Tools
🪛 rubocop (1.68.0)

[warning] 486-486: Shadowing outer local variable - grade.

(Lint/ShadowingOuterLocalVariable)


# Convert counts to percentages
distribution.transform_values { |count| (count.to_f / total_submissions * 100).round(2) }
end

def load_dir_to_tar(dir_path, asmt_dir, tar, filters = [], export_dir = "")
Dir[File.join(dir_path, asmt_dir, "**")].each do |file|
mode = File.stat(file).mode
Expand Down
Loading