SAK-51674 grading Add letter grading #14290
Conversation
WalkthroughAdds a GradeType enum (POINTS, PERCENTAGE, LETTER) and migrates grade-type handling project-wide: APIs, persistence, business logic, UI, import/export, charts, and tests; introduces letter-grade support (mappings, validation, serialization), related fields, repository query, and new GradingService APIs. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/LetterGradePercentMapping.java (1)
126-136: Add a null-check ongradeMapinstandardizeInputGradeto avoid NPEUnlike
getValue,getGrade, andgetGradeMapping, this method assumesgradeMapis non-null; if it ever isn’t (e.g., pre-initialization),gradeMap.keySet()will throw aNullPointerException.A small defensive check keeps behavior consistent with the rest of the class:
public String standardizeInputGrade(String inputGrade) { - String standardizedGrade = null; - for (String grade : gradeMap.keySet()) { + String standardizedGrade = null; + if (gradeMap == null) { + return null; + } + for (String grade : gradeMap.keySet()) { if (grade.equalsIgnoreCase(inputGrade)) { standardizedGrade = grade; break; } } return standardizedGrade; }gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/stats/AssignmentStatistics.java (1)
35-67: Guard against missing letter-grade mappings to avoid NPE in stats.In
calculateStatistics():if (grade != null && grade.getGrade() != null) { stats.addValue(gradeType == GradeType.LETTER ? gradeMap.get(grade.getGrade()) : Double.valueOf(grade.getGrade())); }If
gradeType == GradeType.LETTERbutgradeMapisnullor does not contain the key forgrade.getGrade(),gradeMap.get(...)returnsnull, which will NPE when auto-unboxed todouble.Consider a defensive approach, e.g.:
- if (grade != null && grade.getGrade() != null) { - stats.addValue(gradeType == GradeType.LETTER ? gradeMap.get(grade.getGrade()) : Double.valueOf(grade.getGrade())); - } + if (grade != null && grade.getGrade() != null) { + if (gradeType == GradeType.LETTER) { + if (gradeMap != null) { + Double mapped = gradeMap.get(grade.getGrade()); + if (mapped != null) { + stats.addValue(mapped); + } + } + } else { + stats.addValue(Double.valueOf(grade.getGrade())); + } + }Optionally, log or count unmapped letter grades so they’re visible operationally.
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsCategoryPanel.java (1)
787-842: Category weight formatter likely lost the%sign for non-PERCENTAGE gradebooks.In
PercentConverter.convertToString:final Double percentage = value * 100; GradeType gradeType = SettingsCategoryPanel.this.businessService .getGradebookSettings(SettingsCategoryPanel.this.currentGradebookUid, SettingsCategoryPanel.this.currentSiteId) .getGradeType(); return FormatHelper.formatGradeForDisplay(percentage, gradeType);Category weights are always percentages, independent of the course’s grade entry type. Previously this used
FormatHelper.formatDoubleAsPercentage(percentage), which:
- Always rendered a percentage (e.g.,
20%,89.07%), and- Matches how
updateRunningTotalstill formats the total viaformatDoubleAsPercentage(total * 100).Now, when the gradebook is in POINTS mode,
FormatHelper.formatGradeForDisplay(percentage, GradeType.POINTS)will (per your tests) format as a plain number without the%sign, so you likely get20in the cell while the running total still shows20%. That’s a visible regression and inconsistent with the converter’s own Javadoc (“Convert to percentage for display”).To retain previous behavior and consistency with
updateRunningTotal, consider reverting toformatDoubleAsPercentage:- final Double percentage = value * 100; - - GradeType gradeType = SettingsCategoryPanel.this.businessService - .getGradebookSettings(SettingsCategoryPanel.this.currentGradebookUid, - SettingsCategoryPanel.this.currentSiteId) - .getGradeType(); - - return FormatHelper.formatGradeForDisplay(percentage, gradeType); + final Double percentage = value * 100; + return FormatHelper.formatDoubleAsPercentage(percentage);If you prefer to route through the new API, you could instead call
FormatHelper.formatGradeForDisplay(percentage, GradeType.PERCENTAGE)explicitly, but the key point is to decouple this from the gradebook’sgradeTypeso weights always render as percentages.gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Gradebook.java (1)
91-117: Confirm database migration strategy for existingGRADE_TYPEvalues inGB_GRADEBOOK_T.This commit introduces a JPA entity mapping to the existing
GB_GRADEBOOK_Ttable, converting theGRADE_TYPEcolumn from a numeric storage format toEnumType.STRING:@Column(name = "GRADE_TYPE", nullable = false) @Enumerated(EnumType.STRING) private GradeType gradeType = GradeType.POINTS;The table is referenced in production code (e.g., messageforums), indicating existing data. Ensure:
- A database migration script (or Hibernate auto-DDL configuration) will convert existing numeric
GRADE_TYPEvalues (if any) to their corresponding enum names (POINTS,PERCENTAGE,LETTER); otherwise JPA will fail to deserialize legacy rows.- The new display defaults (
courseLetterGradeDisplayedandcourseAverageDisplayedbothTRUE) are compatible with existing gradebook configurations and UX expectations.
🧹 Nitpick comments (14)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GraderPermission.java (1)
18-18: Remove unused import.The
ArrayListimport is no longer needed since line 45 now usesList.of()instead of constructing anArrayList.Apply this diff to remove the unused import:
-import java.util.ArrayList; import java.util.List;gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/LetterGradePercentMapping.java (1)
104-116: Enhanced for-loop ingetGradeMappinglooks good; consider entrySet/values for minor cleanupThe refactor to a typed enhanced for-loop improves readability and type safety. If you want to tighten it further, you could iterate over
entrySet()orvalues()to avoid the extra map lookup inside the loop, but that’s purely cosmetic here given the tiny map size.gradebookng/api/src/main/java/org/sakaiproject/grading/api/Assignment.java (1)
73-74: Consider adding documentation for the new field.The
maxLetterGradefield is added without a Javadoc comment. For consistency with other fields likepoints(line 70-71) anddueDate(line 76-78) that have documentation, consider adding a brief description of this field's purpose.+ /** + * the maximum letter grade for letter-grade entry mode, or null if not applicable. + */ private String maxLetterGrade;gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/repository/AssignmentGradeRecordRepositoryImpl.java (1)
72-82: Consider adding null-safety checks for input parameters.The method follows the established pattern in this repository. However, other repository implementations in the codebase (e.g.,
ValidationAccountRepositoryImpl) include null checks that return early with an empty result when input parameters are null.Optional improvement for defensive null handling:
@Transactional(readOnly = true) public List<AssignmentGradeRecord> findByGradableObject_IdAndGradableObject_Removed(Long gradableObjectId, Boolean removed) { + if (gradableObjectId == null || removed == null) { + return new ArrayList<>(); + } Session session = sessionFactory.getCurrentSession();gradebookng/tool/src/test/org/sakaiproject/gradebookng/business/TestGradebookNgBusinessService.java (1)
56-63: LGTM! Tests updated for new API signature.The test correctly passes
GradeType.POINTSto the updatedformatGradeForDisplaymethod. Consider adding test cases forGradeType.PERCENTAGEandGradeType.LETTERto ensure comprehensive coverage of the new grade type functionality.gradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.java (1)
108-134: Well-implemented grade type aware total points calculation.The method correctly handles the different grade type scenarios:
- For POINTS/PERCENTAGE: sums individual assignment points (returns 0.0 for empty lists)
- For LETTER: calculates based on uniform per-assignment maximum
The validation for LETTER grade type with proper exception handling is appropriate.
Minor simplification using
mapToDoubleinstead ofCollectors.summingDouble:if (gradeType != GradeType.LETTER) { return getAssignmentList().stream() .filter(a -> a.getPoints() != null) - .collect(Collectors.summingDouble(Assignment::getPoints)); + .mapToDouble(Assignment::getPoints) + .sum(); } else {This is slightly more idiomatic for primitive sum operations and avoids boxing overhead.
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java (1)
50-62: Document and constrain the newskipPointsValidationflag.The conditional around points validation looks correct, but the public API now allows callers to bypass a previously mandatory
points > 0check. Please:
- Update the Javadoc to describe when
skipPointsValidationmay betrue(e.g., letter-grade assignments with unused points).- Consider validating that callers only pass
truein supported scenarios (e.g., via higher-level logic tied toGradeType), to avoid accidentally creating zero/negative‑points numeric items.gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java (1)
287-311: Log full rubric association exceptions instead of onlytoString().In the rubric association block, the catch currently logs
e.toString():} catch (Exception e) { log.warn("Failed to get rubric association for gradebook assignment {}: {}", assignment.getId(), e.toString()); }This loses the stacktrace and makes troubleshooting harder. Prefer passing the exception itself:
- } catch (Exception e) { - log.warn("Failed to get rubric association for gradebook assignment {}: {}", assignment.getId(), e.toString()); - } + } catch (Exception e) { + log.warn("Failed to get rubric association for gradebook assignment {}", assignment.getId(), e); + }This keeps SLF4J parameterization and preserves the full cause.
gradebookng/tool/src/test/org/sakaiproject/gradebookng/business/util/TestFormatHelper.java (1)
259-387: Tests correctly exercise GradeType-aware formatting.Updating the tests to pass
GradeType.POINTSand addingtestFormatGradeForDisplayWithLetterGradealign well with the new API and help protect POINTS/LETTER behavior across locales. You might later add a small test forGradeType.PERCENTAGE, but this is not blocking.gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Category.java (1)
94-99: Clean up stray extra semicolon onassignmentList.The field declaration currently has a double semicolon:
@Transient private List<GradebookAssignment> assignmentList = new ArrayList<>();;It’s harmless but noisy; consider dropping the extra
;for clarity.gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsCategoryPanel.java (1)
139-153: Consider caching maxPoints when iterating assignments in LETTER grade mode.In the category parsing loop:
for (Assignment a : category.getAssignmentList()) { if (settings.getGradeType() != GradeType.LETTER) { points.add(new BigDecimal(a.getPoints()).setScale(2, RoundingMode.HALF_DOWN)); } else { points.add(BigDecimal.valueOf( gradingService.getMaxPoints(settings.getSelectedGradingScaleBottomPercents()).orElse(0D))); } }
gradingService.getMaxPoints(...)will return the same value for every assignment in the loop. You could call it once persettings(or per category) and reuse the result to avoid repeated service lookups:- for (Assignment a : category.getAssignmentList()) { - if (settings.getGradeType() != GradeType.LETTER) { + Double maxPoints = null; + for (Assignment a : category.getAssignmentList()) { + if (settings.getGradeType() != GradeType.LETTER) { points.add(new BigDecimal(a.getPoints()).setScale(2, RoundingMode.HALF_DOWN)); - } else { - points.add(BigDecimal.valueOf(gradingService.getMaxPoints(settings.getSelectedGradingScaleBottomPercents()).orElse(0D))); + } else { + if (maxPoints == null) { + maxPoints = gradingService + .getMaxPoints(settings.getSelectedGradingScaleBottomPercents()) + .orElse(0D); + } + points.add(BigDecimal.valueOf(maxPoints)); } }Not critical, but slightly more efficient and readable.
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java (1)
4736-4753: GradeType switch correctly normalizes grading mode flagsSwitching on
GradeTypeto derivegradeByLetter/gradeByPoints/gradeByPercentis logically correct and ensures only one mode is active at a time.LETTERandPERCENTAGEare handled explicitly, and thedefaultfalls back to points, which matches current semantics.If there is (now or in future) a dedicated
GradeType.POINTSconstant, consider adding an explicitcase POINTS:(falling through to the default block) and optionally logging or asserting on unknown values to make behavior clearer and safer against future enum expansion.gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java (1)
151-155: Empty catch block in test setup.The empty catch block silently swallows any exceptions from
siteService.getSite(). While acceptable in test setup, consider logging or using a fail-fast approach if the mock setup is critical.Site site = mock(Site.class); try { when(siteService.getSite(siteId)).thenReturn(site); } catch (Exception e) { + // Site mock setup - exception not expected in test context }gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/chart/AssignmentGradeChart.java (1)
76-76: Raw type usage for grade collection.Using a raw
Listwithout generics loses compile-time type safety. While the logic correctly handles the type divergence between LETTER (String) and numeric (Double) grades, consider using a more type-safe approach.One alternative is to use
List<Comparable<?>>or separate the logic paths earlier:- final List allGrades = new ArrayList(); + final List<Comparable<?>> allGrades = new ArrayList<>();However, the current approach is pragmatic given the different data types needed for letter vs. numeric grades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/Assignment.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.java(3 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradeDefinition.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradeType.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookInformation.java(2 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GraderPermission.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradingConstants.java(0 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradingService.java(5 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/AbstractGradeRecord.java(2 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/AssignmentGradeRecord.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Category.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Gradebook.java(5 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/GradebookAssignment.java(3 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/LetterGradePercentMapping.java(2 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/repository/AssignmentGradeRecordRepository.java(1 hunks)gradebookng/bundle/src/main/bundle/gradebookng.properties(2 hunks)gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/GradingAuthzImpl.java(0 hunks)gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/GradingPersistenceManagerImpl.java(2 hunks)gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/repository/AssignmentGradeRecordRepositoryImpl.java(1 hunks)gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java(9 hunks)gradebookng/tool/pom.xml(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.java(9 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/util/CourseGradeFormatter.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/util/FormatHelper.java(4 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/framework/GradebookNgEntityProducer.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/actions/GradeUpdateAction.java(3 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/chart/AssignmentGradeChart.java(6 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.java(3 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java(14 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/ImportWizardModel.java(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/QuickEntryPage.java(4 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java(7 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AssignmentStatisticsPanel.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/BasePanel.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/BulkGradePanel.java(3 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/GradeSummaryTablePanel.java(4 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/RubricGradePanel.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsCategoryPanel.java(12 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.html(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/StudentGradeSummaryPanel.html(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/UpdateUngradedItemsPanel.java(4 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/ZeroUngradedItemsPanel.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/ExportPanel.java(6 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/GradeImportConfirmationStep.java(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/GradeImportUploadStep.java(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/PreviewImportedGradesPanel.java(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/stats/AssignmentStatistics.java(4 hunks)gradebookng/tool/src/test/org/sakaiproject/gradebookng/business/TestGradebookNgBusinessService.java(4 hunks)gradebookng/tool/src/test/org/sakaiproject/gradebookng/business/util/TestFormatHelper.java(3 hunks)gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js(1 hunks)msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java(3 hunks)msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/MessageForumStatisticsBean.java(3 hunks)webapi/src/main/java/org/sakaiproject/webapi/controllers/GradesController.java(2 hunks)webcomponents/bundle/src/main/bundle/grader.properties(1 hunks)
💤 Files with no reviewable changes (2)
- gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/GradingAuthzImpl.java
- gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradingConstants.java
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/StudentGradeSummaryPanel.htmlgradebookng/tool/pom.xmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.htmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.html
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/StudentGradeSummaryPanel.htmlgradebookng/tool/pom.xmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.htmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.html
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/StudentGradeSummaryPanel.htmlgradebookng/tool/pom.xmlgradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.jsgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.htmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.html
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/StudentGradeSummaryPanel.htmlgradebookng/tool/pom.xmlgradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.jsgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.htmlgradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.html
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/RubricGradePanel.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradeType.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsGradeEntryPanel.javagradebookng/impl/src/main/java/org/sakaiproject/grading/impl/repository/AssignmentGradeRecordRepositoryImpl.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/UpdateUngradedItemsPanel.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/BasePanel.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/GradeImportUploadStep.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradeDefinition.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AssignmentStatisticsPanel.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/GradeImportConfirmationStep.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookInformation.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/GradebookAssignment.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsCategoryPanel.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/GradesController.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/MessageForumStatisticsBean.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/LetterGradePercentMapping.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GraderPermission.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/QuickEntryPage.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Category.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/Assignment.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/framework/GradebookNgEntityProducer.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/PreviewImportedGradesPanel.javagradebookng/tool/src/test/org/sakaiproject/gradebookng/business/TestGradebookNgBusinessService.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/business/util/FormatHelper.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/AbstractGradeRecord.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/business/util/CourseGradeFormatter.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/ImportWizardModel.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/repository/AssignmentGradeRecordRepository.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/ZeroUngradedItemsPanel.javagradebookng/tool/src/test/org/sakaiproject/gradebookng/business/util/TestFormatHelper.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/chart/AssignmentGradeChart.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/GradeSummaryTablePanel.javagradebookng/impl/src/main/java/org/sakaiproject/grading/impl/GradingPersistenceManagerImpl.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/Gradebook.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/ExportPanel.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/BulkGradePanel.javagradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradingService.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/model/AssignmentGradeRecord.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/actions/GradeUpdateAction.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/stats/AssignmentStatistics.java
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.js: Use clean, standard modern JavaScript in Sakai frontends
Update jQuery code to modern JavaScript when making changes, if the changes are minimal
Prefer ES6+ features (arrow functions, template literals, destructuring, etc.) in JavaScript
Write modular, reusable JavaScript components in Sakai frontends
Minimize use of global variables and functions (Avoid Global Scope) in JavaScript
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs likefetchkeepalive are present; avoid legacy branches, UA sniffing, or fallbacks unless a specific evergreen gap is documented
Replace jQuery with modern DOM APIs when touching code; new work should not add jQuery dependencies
Compose Lit components, ES modules, and encapsulated helpers; keep state local and explicit with modular code
Prefer module scope or class fields; expose intentional APIs instead of incidental globals; avoid global side channels
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Internal reactive state in Lit components should stay prefixed with
_and is only surfaced through getters/setters when required
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
📚 Learning: 2025-09-27T15:41:26.287Z
Learnt from: csev
Repo: sakaiproject/sakai PR: 14094
File: lti/lti-api/src/java/org/sakaiproject/lti/beans/LtiContentBean.java:1-19
Timestamp: 2025-09-27T15:41:26.287Z
Learning: csev prefers to use Apache 2.0 license headers for new code he writes, even when the project standard is ECL 2.0, as the licenses are compatible.
Applied to files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/SettingsCategoryPanel.java
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java
🧬 Code graph analysis (4)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java (1)
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (1)
points(846-846)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/ImportWizardModel.java (2)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookInformation.java (1)
Data(30-127)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java (1)
Data(93-108)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java (1)
lessonbuilder/api/src/java/org/sakaiproject/lessonbuildertool/api/LessonBuilderConstants.java (1)
LessonBuilderConstants(18-21)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/actions/GradeUpdateAction.java (2)
kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java (1)
NumberUtil(35-143)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java (1)
GbGradebookData(59-759)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
gradebookng/api/src/main/java/org/sakaiproject/grading/api/model/AssignmentGradeRecord.java
Show resolved
Hide resolved
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java
Show resolved
Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.java
Outdated
Show resolved
Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java
Outdated
Show resolved
Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.java (1)
151-161: Potential NPE from autounboxingdropKeepEnabledingetPointsForCategory
dropKeepEnabledis aBoolean, but Line 152 uses it inif (!dropKeepEnabled), which triggers autounboxing. IfdropKeepEnabledis ever null (which is plausible, given you’ve added a null-safegetDropKeepEnabled()earlier), this will throw aNullPointerException.Consider switching this check to use the null-safe getter:
- public Double getPointsForCategory() { - if (!dropKeepEnabled) { + public Double getPointsForCategory() { + if (!getDropKeepEnabled()) { return null; }This aligns behavior with the rest of the class and removes the NPE risk.
🧹 Nitpick comments (4)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java (1)
158-182: OnChangeAjaxBehavior attached for PERCENTAGE but has no effect.The OnChangeAjaxBehavior is added for both POINTS and PERCENTAGE grade types (line 158:
gradingType != GradeType.LETTER), but the scaling logic inside only executes for POINTS (line 167). For PERCENTAGE, the behavior is attached but does nothing, which adds a small overhead.Consider either:
- Moving the behavior attachment inside the
if (gradingType == GradeType.POINTS)check to avoid unnecessary behavior for PERCENTAGE, or- Documenting why PERCENTAGE needs the behavior (e.g., for future extensibility)
Example refactor to eliminate the overhead:
- if (gradingType != GradeType.LETTER) { - // onchange, might want to scale - points.add(new OnChangeAjaxBehavior() { - private static final long serialVersionUID = 1L; - - @Override - protected void onUpdate(final AjaxRequestTarget target) { - - // conditional option to scale - if (gradingType == GradeType.POINTS) { - - final Double existing = AddOrEditGradeItemPanelContent.this.existingPoints; - final Double current = points.getModelObject(); - - log.debug("existingPoints: {}", existing); - log.debug("currentPoints: {}", current); - AddOrEditGradeItemPanelContent.this.scaleGradesTriggered = existing != null && !existing.equals(current); - - log.debug("scaleGradesTriggered: {}", AddOrEditGradeItemPanelContent.this.scaleGradesTriggered); - - target.add(AddOrEditGradeItemPanelContent.this.scaleGradesContainer); - } - } - }); - } + if (gradingType == GradeType.POINTS) { + // onchange, might want to scale + points.add(new OnChangeAjaxBehavior() { + private static final long serialVersionUID = 1L; + + @Override + protected void onUpdate(final AjaxRequestTarget target) { + final Double existing = AddOrEditGradeItemPanelContent.this.existingPoints; + final Double current = points.getModelObject(); + + log.debug("existingPoints: {}", existing); + log.debug("currentPoints: {}", current); + AddOrEditGradeItemPanelContent.this.scaleGradesTriggered = existing != null && !existing.equals(current); + + log.debug("scaleGradesTriggered: {}", AddOrEditGradeItemPanelContent.this.scaleGradesTriggered); + + target.add(AddOrEditGradeItemPanelContent.this.scaleGradesContainer); + } + }); + }gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.java (3)
610-705: GradeType-based handling and concurrency checks look correct; minor cleanup possibleUsing
GradeType gradingType = gradebook.getGradeType();and branching onGradeType.PERCENTAGEfor the conversion logic is consistent with the enum migration, and the reintroduced letter-grade concurrency check restores protection forGradeType.LETTER. The numeric branch usingNumberFormatandBigDecimalalso preserves the SAK-42001 behavior.You could slightly simplify and avoid an extra service call by reusing the local variable in the concurrency block:
if (gradingType != GradeType.LETTER) { ... } else { ... }Functionally it’s equivalent but a bit clearer.
927-982: Grade comparison matrix now correctly driven by GradeTypeAccepting a
GradeType gradingTypeparameter and appending%only whengradingType == GradeType.PERCENTAGEkeeps the comparison display consistent with the configured entry type while leaving points and letter grades unchanged. The security advisor usage and mapping overGbStudentGradeInforemain intact.If you find more formatting logic cropping up, consider centralizing the
"%"decision into a helper (e.g., inFormatHelper) to avoid scattered grade-type branching.
2264-2272: Clarify intent of null category-type argument to calculateCategoryScoreThe new call:
final Optional<CategoryScoreData> result = gradingService.calculateCategoryScore(gradebook.getId(), studentUuid, categoryId, isInstructor, null);compiles, but it’s not obvious what
nullrepresents, especially since related bulk APIs here passgradebook.getCategoryType()as the last parameter.For consistency and to avoid surprises in the service implementation, consider either:
- Passing
gradebook.getCategoryType()instead ofnull, if that’s what the service expects, or- Adding a brief comment that
nullis intentional and means “let the service infer the category type from the gradebook”.This will make the behavior clearer to future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/Assignment.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.java(3 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java(1 hunks)gradebookng/api/src/main/java/org/sakaiproject/grading/api/GraderPermission.java(1 hunks)gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/repository/AssignmentGradeRecordRepositoryImpl.java(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.java(9 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java(14 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- gradebookng/api/src/main/java/org/sakaiproject/grading/api/Assignment.java
- gradebookng/api/src/main/java/org/sakaiproject/grading/api/GraderPermission.java
- gradebookng/impl/src/main/java/org/sakaiproject/grading/impl/repository/AssignmentGradeRecordRepositoryImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.javagradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.javagradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
🧬 Code graph analysis (2)
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java (1)
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (1)
points(846-846)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java (1)
lessonbuilder/api/src/java/org/sakaiproject/lessonbuildertool/api/LessonBuilderConstants.java (1)
LessonBuilderConstants(18-21)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (16)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/AddOrEditGradeItemPanelContent.java (6)
52-52: LGTM: Import additions support enum migration.The GradeType enum and LessonBuilderConstants imports are properly used throughout the file.
Also applies to: 55-55
93-93: LGTM: Enum migration improves type safety.Replacing Integer with the GradeType enum provides better type safety and code clarity.
102-102: LGTM: Diamond operator improves readability.The diamond operator syntax is a standard Java modernization with no functional impact.
126-132: LGTM: Label logic correctly handles all grade types.The conditional logic appropriately shows labels for PERCENTAGE and POINTS, while hiding the label for LETTER grade type where no numeric input is needed.
197-197: LGTM: Scale grades correctly hidden for letter grading.Setting scaleGradesContainer visibility based on grade type ensures the scaling option is never shown for LETTER grades, where scaling doesn't apply.
295-311: Code correctly follows all guidelines and properly handles LETTER grade incompatibility.The implementation:
- Uses explicit type declarations (
Optional<AssociationTransferBean>) instead ofvar- Employs parameterized logging:
log.warn("Failed to get rubric association for gradebook assignment {}", assignment.getId(), e)- Uses null-safe comparison with
StringUtils.equals- Consistently hides rubric UI for LETTER grades (matching pattern on lines 156 and 197 where points and scaling are similarly hidden)
The LETTER grade restriction is an intentional architectural pattern that prevents point-based features (including rubrics) from being configured for letter-graded assignments, which is sensible design.
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java (5)
327-329: LGTM! Conditional serialization based on grade type.The serialization logic correctly branches to use
getLetter()for letter grades andgetScore()for numeric grades, ensuring appropriate data handling for each grade type.
418-420: LGTM! Clean enum-based grade type detection.The settings serialization correctly uses the
GradeTypeenum for grade entry type detection and includes letter grades in the settings map for UI consumption.Also applies to: 439-439
475-475: LGTM! Grade type propagated through formatting.The signature change correctly adds
GradeTypeparameter and propagates it toFormatHelper.formatGradeForDisplay(), ensuring consistent grade formatting based on the configured grade type.Also applies to: 485-485, 489-489
636-636: Method signatures and API usage are correct.The code correctly calls
category.getTotalPoints(settings.getGradeType(), gradingService.getMaxPoints(courseGradeMap).orElse(0D)):
CategoryDefinition.getTotalPoints(GradeType gradeType, Double maxPoints)signature matches the usage, accepting grade type and a Double value for maximum points.GradingService.getMaxPoints(Map<String, Double> gradeMap)correctly returnsOptional<Double>, which is properly unwrapped withorElse(0D)to provide a Double fallback.- The implementation validates maxPoints when gradeType is LETTER (throws IllegalArgumentException if maxPoints ≤ 0), which aligns with the requirement for letter grade calculations.
No issues identified.
578-578: No action needed. ThecourseGradeMapparameter is correct.The method signature confirms
getMaxLetterGrade(Map<String, Double> gradeMap)expects a map with String keys (grade letters) and Double values (percentage thresholds). ThecourseGradeMapvariable is correctly typed asMap<String, Double>and represents the active grading scale. The implementation finds the highest percentage threshold and returns its associated letter grade, making this the appropriate usage.gradebookng/api/src/main/java/org/sakaiproject/grading/api/CategoryDefinition.java (1)
52-135: Grade-type-aware total points and non-nullassignmentListlook correct
- Initializing
assignmentListtonew ArrayList<>()is a good defensive move and avoids null-stream issues by default.- In
getTotalPoints(GradeType gradeType, Double maxPoints), the split between non-LETTER (sum of non-null assignment points) and LETTER (validatedmaxPoints > 0D, thenassignmentList.size() * maxPoints) is clear and matches the Javadoc.- Argument validation via
IllegalArgumentExceptionfor invalidmaxPointsin LETTER mode is appropriate and fails fast.No issues from a correctness perspective here.
gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradebookHelper.java (1)
51-63: LETTER-aware points validation is consistent and preserves name checksThe updated
validateAssignmentNameAndPoints(Assignment assignmentDefinition, GradeType gradeType):
- Correctly skips the points
> 0validation whengradeType == GradeType.LETTER, allowing letter-only assignments.- Preserves existing behavior for POINTS/PERCENTAGE (non-LETTER) by still enforcing
points != null && points > 0.- Leaves
validateGradeItemName(assignmentDefinition.getName())untouched, so title validation semantics remain the same.This change cleanly aligns validation with the new grade type model.
gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/GradebookNgBusinessService.java (3)
83-83: GradeType import is appropriateImporting
GradeTypehere matches its later usage in this class and keeps references concise; no issues.
770-788: GradeType usage in saveExcuse aligns with percentage semanticsSwitching the excuse logic to branch on
GradeType.PERCENTAGEwhile leaving other types untouched preserves the existing “stored as points, convert to percentage for the service call” behavior and matches howsaveGradetreats percentage entry. No functional concerns here.
1724-1745: Updated GradingService calls with siteId look consistentUsing the new signatures
gradingService.addAssignment(gradebookUid, siteId, assignment)andgradingService.getAssignments(gradebookUid, siteId, SortType.SORT_BY_NONE)insideaddAssignmentmatches the rest of this class’s service usage and ensures assignments are resolved in the correct site context. The surrounding sort-order logic is unchanged and remains correct.
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbGradebookData.java
Show resolved
Hide resolved
…est/GradingServiceTests.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java (5)
141-155: User mock IDs and baseSitemock insetup()Stubbing
getId()foruser1User/user2Userand adding a defaultSitemock forsiteService.getSite(siteId)should support the newer grading/section logic that depends on user IDs and site lookup. Swallowing the exception aroundgetSiteis effectively harmless here (Mockito won’t throw), but if this ever did fail it might be clearer to callAssert.fail(...)as increateSiteMockso the test fails loudly rather than silently skipping the stub.
474-490: GradeType integration in grade definitions looks correct; assertion style nitUsing
GradeType.POINTSwhen constructing theGradeDefinitions and then verifying thegradeEntryTypeviagetGradesWithoutCommentsForStudentsForItemsaligns with the new enum-based API and keeps the tests realistic. For enums, it’s a bit more idiomatic to useassertEquals(GradeType.POINTS, defs.get(0).getGradeEntryType());rather thanassertTrue(GradeType.POINTS == ...), purely for readability.Also applies to: 780-781
657-704:testPercentageCalculationWithWeightis mostly tautological and comments are out of sync with the dataThis test currently computes the percentage manually, writes it back into
gradeDefwithsetGrade(String.valueOf((int) percentage)), and then asserts on that same value, so it doesn’t actually validate whatgradingServicereturns in percentage mode. The explanatory comments also still refer to10/20while the data used is15/30. Consider asserting on the value coming fromgradingServicebefore mutatinggradeDef, and updating the comments to match the actual numbers, so the test genuinely exercises the new percentage-grade behavior rather than confirming its own calculation.
784-858: Letter-grading flow test is solid; a couple of optional assertions could deepen coverageThe
letterGradingtest does a nice job of wiring up gradebook settings, per-user permissions, section membership, letter grades on an assignment, and the resultingCourseGradeTransferBeanpoints mapping (83/73 out of 100), which directly exercises the new letter-grade support. If you want to push coverage a bit further, you might also:
- Assert that
gradingService.getGradebookInformation(...).getGradeType()isGradeType.LETTERafter the settings update, and/or- Assert that the
GradeDefinitions returned for this assignment haveGradeType.LETTERasgradeEntryType,to fully exercise the enum across both assignment-level and course-grade flows.
879-902: Localvarusage conflicts with Sakai Java style guidelinesThis helper method still uses
varfor theCategoryDefinitioninstances and thecatslist, which conflicts with the stated guideline to avoid local variable type inference and use explicit types. As per coding guidelines, consider cleaning this up the next time you touch this block, e.g.:- var cd1 = new CategoryDefinition(); + CategoryDefinition cd1 = new CategoryDefinition(); @@ - var cd2 = new CategoryDefinition(); + CategoryDefinition cd2 = new CategoryDefinition(); @@ - var cats = new ArrayList<CategoryDefinition>(); + List<CategoryDefinition> cats = new ArrayList<CategoryDefinition>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (1)
gradebookng/impl/src/test/java/org/sakaiproject/grading/impl/test/GradingServiceTests.java (1)
31-45: New wiring for configuration, permissions, and section awareness looks consistentInjecting
ServerConfigurationService,GradingPermissionService, andSectionAwareness(plus the related model types) into this test class matches their later usage in the letter-grading and course-grade/permissions flows, so the additional imports and@Autowiredfields look correct from a test wiring perspective.Also applies to: 55-57, 83-89
|
Simple cypress fix after mergin |
|
1 participant
https://sakaiproject.atlassian.net/browse/SAK-51674
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.