SAK-52238 Samigo First question not accessible when part and question are timed in linear access #14288
Conversation
… are timed in linear access
WalkthroughAdjusted SubmitToGradingActionListener to change when placeholder ItemGradingData is created during linear navigation by checking for existing ItemGradingData, ActionEvent presence, and item types; replaced string-concatenated logs with parameterized logging and reorganized imports. Changes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java (2)
56-57: Logging/import setup and debug conversions look good; one small log-message nit
- The added imports and class-level logger wiring are consistent with the existing usage of the
logfield in this class.- The converted debug calls in
submitToGradingService, the EMI section, the ActionEvent handling, andidentifyOrphanedEMIAnswersall use{}placeholders with the correct number and order of arguments, so they should behave as intended and avoid string concatenation overhead.- Minor nit: the message at line 811 reads
"adding new answer answer: {}"; consider simplifying it to"adding new answer: {}"for clarity.Also applies to: 81-82, 94-95, 325-339, 788-812, 895-897, 902-903, 914-914, 969-973
889-940: Linear-navigation placeholder logic looks correct; consider guardingitemTextIdagainst null / -1The new block for linear navigation when
addsis empty and the action is notshowFeedbackappears to address SAK-52238 as intended:
- It now runs even when
aeisnull, ensuring a fakeItemGradingDatais created or reused in linear access for timed/tracked questions.- It avoids recreating placeholders when an
ItemGradingDataalready exists, and forae == nullexplicitly adds the existing grading toaddsto prevent a “blank” question.- It skips creating extra empty records for File Upload/Audio when a grading already exists.
One robustness improvement I’d suggest:
ItemService.getItemTextId(publishedItemId)returns aLong, and the subsequentif(itemTextId != -1)relies on auto‑unboxing and assumes the value is nevernull. If the service ever returnsnullfor an unusual item, this comparison would NPE, and even when non‑null you’re comparing a boxed value to a primitive sentinel.- To make this path safer without changing behavior, you can explicitly guard for
nulland the sentinel-1Lbefore using the value:- ItemService itemService = new ItemService(); - Long itemTextId = itemService.getItemTextId(publishedItemId); - log.debug("itemTextId = {}", itemTextId); - if(itemTextId != -1){ - itemGrading.setPublishedItemTextId(itemTextId); - adds.add(itemGrading); - } + ItemService itemService = new ItemService(); + Long itemTextId = itemService.getItemTextId(publishedItemId); + log.debug("itemTextId = {}", itemTextId); + if (itemTextId != null && itemTextId.longValue() != -1L) { + itemGrading.setPublishedItemTextId(itemTextId); + adds.add(itemGrading); + }If the current
ItemService#getItemTextIdcontract guarantees a non‑null return, this is mostly defensive, but it would harden the code against future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java(8 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:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java
🧠 Learnings (2)
📓 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:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.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: sakai-deploy
- GitHub Check: maven-build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java (1)
906-933: Add explicit else clause to clarify linear navigation logic for non-File Upload/Audio question types.The code structure for
existingItemGrading != nullin linear navigation mode (lines 920-932) intentionally handles File Upload (typeId 6) and Audio (typeId 7) separately from other question types, but lacks documentation for the implicit fallthrough. To improve maintainability, add an explicitelseclause explaining why other question types (1-5, 8-9, 11-16) do not addexistingItemGradingwhenae != null:} else { // For other question types, ItemGradingData is handled within the switch statement above. // If adds is empty here, the question was unanswered or only marked for review; we don't // create a placeholder ItemGradingData when user navigates via button clicks. }This clarifies that the logic is intentional and complete, with File Upload/Audio being special cases because they auto-create ItemGradingData upon user upload/record actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java(8 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:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java
🧠 Learnings (2)
📓 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:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java
🔇 Additional comments (3)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/SubmitToGradingActionListener.java (3)
56-56: LGTM: Lombok logging annotation introduced correctly.The addition of
ItemTextIfcimport and Lombok's@Slf4jannotation aligns with modern Java logging practices and is used consistently throughout the file.Also applies to: 81-81, 94-94
324-324: LGTM: Parameterized logging implemented correctly.All log statements have been properly converted from string concatenation to SLF4J parameterized logging with
{}placeholders, improving performance and following the coding guidelines.Also applies to: 338-338, 799-799, 802-802, 811-811, 894-894, 902-902, 914-914, 968-969, 973-973
905-905: > Likely an incorrect or invalid review comment.
|
This PR has been automatically marked as stale due to 21 days of inactivity. It will be closed in 7 days unless there is new activity. |
1 participant
https://sakaiproject.atlassian.net/browse/SAK-52238
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.