SAK-52134 Import Implement unique title generation for assignments to avoid gradebook conflicts #14224
Conversation
… avoid gradebook conflicts
WalkthroughWraps the call to Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.javaTip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java (1)
5184-5199: Consider Gradebook names when generating uniques
generateUniqueAssignmentTitleonly filters against existing assignment titles. If the Gradebook already has manual items namedFoo,Foo-1, etc., we’ll still hit the sameConflictingAssignmentNameExceptionon retry and fall back to disabling integration. Could we extend the uniqueness check to include existing Gradebook item names (viagradingService) so we keep searching until we find a name that actually clears the Gradebook collision?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Commit messages should follow: <issue key> <component> <brief description> (e.g., SAK-12345 Assignments add option x)
⏰ 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
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java
Show resolved
Hide resolved
|
This PR has been automatically marked as stale due to 10 days of inactivity. It will be closed in 4 days unless there is new activity. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java (2)
4559-4622: Graceful conflict handling looks solid; consider making rename logic gradebook‑awareWrapping
gradingService.addExternalAssessmentin atry/catchforConflictingAssignmentNameExceptionand then renaming + retrying is a good improvement, and switching toassignmentRepository.merge(nAssignment)in this path avoids the previous doubleupdateAssignmentside‑effects.One behavioural gap:
generateUniqueAssignmentTitlecurrently only checks against existing Assignment titles in the site. If the conflict originates from a gradebook‑only item (a column created directly in Gradebook with no corresponding Assignment), the first retry will still use the same title (because it’s not in the assignment list), leading immediately to the fallback path (draft +GRADEBOOK_INTEGRATION_NO) instead of resolving via a suffixed title. To fully meet the intent of SAK‑52134, you might want to extend the uniqueness check to also include gradebook assignment names in that context (internal + external) viagradingService, so the renamed title is highly likely to be non‑conflicting on the Gradebook side as well.Additionally, if the retry fails again specifically with another
ConflictingAssignmentNameException, we currently do not attempt a second suffix (e.g.-2) but instead disable gradebook integration. That’s acceptable as a safe fallback, but if you expect frequent name clashes, a small loop with a capped number of attempts (probing Gradebook names per candidate) would avoid unnecessarily dropping the link.
5182-5206: Defensively handle null/blank titles ingenerateUniqueAssignmentTitleThe helper will throw a
NullPointerExceptioniforiginalTitleis evernull(e.g. via bad import data), sinceuniqueTitlestarts asnulland the loop concatenatesoriginalTitle + "-" + suffix. While callers should normally guarantee a non‑blank title, a small guard would make this more robust against malformed input:- private String generateUniqueAssignmentTitle(String originalTitle, String context) { - Collection<Assignment> existingAssignments = getAssignmentsForContext(context); + private String generateUniqueAssignmentTitle(String originalTitle, String context) { + if (StringUtils.isBlank(originalTitle)) { + return originalTitle; + } + Collection<Assignment> existingAssignments = getAssignmentsForContext(context); @@ - while (existingTitles.contains(uniqueTitle)) { - uniqueTitle = originalTitle + "-" + suffix; + while (existingTitles.contains(uniqueTitle)) { + uniqueTitle = originalTitle + "-" + suffix; suffix++; }(Optionally, you could also switch to the already‑imported
Collectors.toSet()instead of the fully‑qualifiedjava.util.stream.Collectors.toSet()for consistency with the rest of the class.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Commit messages should follow: <issue key> <component> <brief description> (e.g., SAK-12345 Assignments add option x)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.