Enhance Export Functionality with PLD and GDU Metrics #2250
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/models/assessment.rb (1)
478-478: Consider making grade boundaries configurableThe grade boundaries are currently hardcoded. Consider making them configurable through course settings or assessment configuration.
- grade_boundaries = { 'A': 90, 'B': 80, 'C': 70, 'D': 60, 'F': 0 } + grade_boundaries = course.grade_boundaries || { 'A': 90, 'B': 80, 'C': 70, 'D': 60, 'F': 0 }app/controllers/assessments_controller.rb (2)
509-515: Fix octal literals and remove trailing whitespace
- Use
0oprefix for octal literals instead of implicit octal- Remove trailing whitespace on line 516
- tar.add_file_simple("PLD.yml", 0644, pld_data.bytesize) do |io| + tar.add_file_simple("PLD.yml", 0o644, pld_data.bytesize) do |io| io.write pld_data end - tar.add_file_simple("GDU.yml", 0644, gdu_data.bytesize) do |io| + tar.add_file_simple("GDU.yml", 0o644, gdu_data.bytesize) do |io| io.write gdu_data end - +🧰 Tools
🪛 rubocop (1.68.0)
[convention] 509-509: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
[convention] 513-513: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
Line range hint
492-516: Consider adding error handling for PLD and GDU calculationsThe export method should handle potential errors from PLD and GDU calculations.
+ begin pld_data = @assessment.calculate_pld.to_yaml gdu_data = @assessment.calculate_gdu.to_yaml + rescue StandardError => e + flash[:error] = "Error calculating assessment metrics: #{e.message}" + redirect_to action: "index" + return + end begin # Update the assessment config YAML file.🧰 Tools
🪛 rubocop (1.68.0)
[convention] 509-509: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
[convention] 513-513: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
[convention] 516-516: Trailing whitespace detected.
(Layout/TrailingWhitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/controllers/assessments_controller.rb(2 hunks)app/models/assessment.rb(1 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
app/models/assessment.rb
[warning] 486-486: Shadowing outer local variable - grade.
(Lint/ShadowingOuterLocalVariable)
app/controllers/assessments_controller.rb
[convention] 509-509: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
[convention] 513-513: Use 0o for octal literals.
(Style/NumericLiteralPrefix)
[convention] 516-516: Trailing whitespace detected.
(Layout/TrailingWhitespace)
| 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 |
There was a problem hiding this comment.
Fix variable shadowing and add error handling
There are several issues to address:
- The
gradevariable on line 486 shadows the outer variable - Need to handle edge case of no submissions
- 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.
| 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)
| # 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling edge cases and enriching the PLD data
The implementation could be enhanced in the following ways:
- Handle edge cases like nil due dates
- Consider assessment extensions in late day calculations
- Include grace days information
- 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
endCommittable suggestion skipped: line range outside the PR's diff.
|
For error handling, could consider adding validation to ensure the late_days calculation doesn't break if a submission or due_at date is nil. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/controllers/assessments_controller.rb (2)
507-516: Reduce code duplication in file writing logic.The file writing logic for PLD.yml and GDU.yml is duplicated. Consider extracting it into a helper method.
+ def add_yaml_to_tar(tar, filename, data) + tar.add_file_simple(filename, 0644, data.bytesize) do |io| + io.write data + end + end + - tar.add_file_simple("PLD.yml", 0o644, pld_data.bytesize) do |io| - io.write pld_data - end - - tar.add_file_simple("GDU.yml", 0o644, gdu_data.bytesize) do |io| - io.write gdu_data - end + add_yaml_to_tar(tar, "PLD.yml", pld_data) + add_yaml_to_tar(tar, "GDU.yml", gdu_data)
517-517: Remove trailing whitespace.There is trailing whitespace on this line that should be removed.
- +🧰 Tools
🪛 rubocop (1.68.0)
[convention] 517-517: Trailing whitespace detected.
(Layout/TrailingWhitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/assessments_controller.rb(2 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
app/controllers/assessments_controller.rb
[convention] 517-517: Trailing whitespace detected.
(Layout/TrailingWhitespace)
|
|
||
| pld_data = @assessment.calculate_pld.to_yaml | ||
| gdu_data = @assessment.calculate_gdu.to_yaml |
There was a problem hiding this comment.
🛠️ 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.
| 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 |
Description
This pull request enhances the export functionality of assessments by including Presumably Late Days (PLD) and Grade Distribution Usage (GDU) metrics. These metrics are now calculated for each assessment and included in the exported tarball as separate YAML files,
PLD.ymlandGDU.yml.Motivation and Context
This change provides educators and administrators with more detailed insights into assessment performance and submission timeliness, aiding in better academic decision-making. This enhancement does not fix a specific issue but adds significant functionality to the existing export capabilities.
How Has This Been Tested?
The new export functionality was tested in a local development environment where:
Attempt for issue: #1733