SAK-52251 Lessons Subpages of a Lessons are imported as pseudo-orphans #14304
Conversation
WalkthroughExpose referenced-page discovery, skip orphaned pages during traversal, and add hierarchy computation and application during merge/import including parent/topParent mapping, toolId synchronization, and related test coverage. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java (1)
1739-1751: Consider adding cycle detection to the parent chain traversal.The
whileloop walks up the parent chain to find the top parent. While cycles should not exist in valid data, a defensive safeguard would prevent an infinite loop in case of corrupted data.🔎 Suggested defensive safeguard
// Calculate top parents by walking up the tree for (Long pageId : calculatedParentMap.keySet()) { Long currentPageId = pageId; Long topParent = null; + Set<Long> visited = new HashSet<>(); - while (calculatedParentMap.containsKey(currentPageId)) { + while (calculatedParentMap.containsKey(currentPageId) && !visited.contains(currentPageId)) { + visited.add(currentPageId); topParent = calculatedParentMap.get(currentPageId); currentPageId = topParent; } if (topParent != null) { calculatedTopParentMap.put(pageId, topParent); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java(6 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:
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
🧠 Learnings (1)
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Applied to files:
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
🧬 Code graph analysis (1)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java (2)
kernel/api/src/main/java/org/sakaiproject/util/MergeConfig.java (1)
MergeConfig(31-41)kernel/api/src/main/java/org/sakaiproject/util/cover/LinkMigrationHelper.java (1)
LinkMigrationHelper(23-52)
⏰ 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 (4)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java (4)
101-134: Import additions are appropriate.The added imports support the new hierarchy reconstruction functionality:
SakaiLTIUtilfor LTI content handlingMergeConfigandLinkMigrationHelperfor merge operationsThese are all used within the file and follow proper organization.
1650-1651: Deferred parent relationship assignment is a reasonable approach.Creating pages with a placeholder
toolIdof "0" and setting parent relationships in a later pass allows proper hierarchy reconstruction after all pages are created. This is cleaner than trying to resolve relationships during initial creation.
1753-1792: Hierarchy update logic looks correct.The code properly:
- Iterates through the page map using old page IDs as keys
- Looks up calculated parent/topparent relationships
- Maps old IDs to new IDs via
pageMap- Updates and persists only when changes are needed
The logging at the end provides good visibility into the operation.
1970-1999: ToolId propagation logic is correct.The code properly propagates the
toolIdfrom top-level pages to their descendants. The checks at line 1986-1987 correctly handle:
- Null pages
- Top parent with null or placeholder "0" toolId
- Avoiding unnecessary updates when toolId already matches
Note: For very large imports, the two
getPage()calls per iteration (lines 1983-1984) could be optimized by caching pages from the earlier pass, but this is acceptable for typical import scenarios.
...er/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
Show resolved
Hide resolved
|
Converted to draft: continues the work from #14300 |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java`:
- Around line 1893-1969: The hierarchy reconstruction loses parent/topparent
info for cross-server imports because findReferencedPagesByItems(fromSiteId)
returns empty when fromSite == null; update the logic after subpageRefs is
computed to fall back to the XML attributes by iterating pageElementMap (the
parsed <page> Elements), reading the "parent" attribute (use NumberUtils.toLong
or equivalent) and populating calculatedParentMap for any parentId > 0 when
!isSameServer and subpageRefs.isEmpty(), then compute calculatedTopParentMap as
before and proceed to apply them to pages; this uses the existing symbols
findReferencedPagesByItems, pageElementMap, calculatedParentMap,
calculatedTopParentMap, and isSameServer.
🧹 Nitpick comments (3)
lessonbuilder/tool/src/test/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducerTest.java (3)
126-164: Tests verify mock configuration rather than production logic.These tests create mocks with
when(subpage1.getToolId()).thenReturn("tool-id-123")and then assertassertNotNull(subpage1.getToolId()). This only validates that Mockito returns configured values, not thatLessonBuilderEntityProducercorrectly reconstructs hierarchy during import.To actually test the hierarchy reconstruction:
`@Test` public void testSubpagesImportedWithCorrectHierarchy() { // Setup: Create XML archive element or mock the merge inputs // ... // Act: Call the actual merge method // producer.mergeInternal(toSiteId, rootElement, archivePath, fromSiteId, mcx, entityMap, false); // Assert: Verify pages in destination site have correct hierarchy // SimplePage importedSubpage = dao.getPage(newPageId); // assertNotNull(importedSubpage.getParent()); // assertEquals(expectedParentId, importedSubpage.getParent()); }Consider refactoring to test actual production code behavior, or add integration tests that exercise the full import flow.
178-220: Test doesn't verify the recovery mechanism it documents.The test comment at lines 214-215 states "the findReferencedPagesByItems method should find this relationship" but the test never calls
findReferencedPagesByItems(which is a private method inLessonBuilderEntityProducer). The test only verifies thatdao.findItemsOnPage()returns the mocked items.To properly test orphaned subpage recovery, consider:
- Making
findReferencedPagesByItemspackage-private or exposing through a testable interface- Testing the public
mergemethod with orphaned pages in the input data- Verifying the resulting pages have correct parent/topParent values after import
225-272: Test serves as documentation but doesn't exercise production code.This test validates expected data structures (PAGE items referencing subpages) but only exercises mock configuration and iteration, not the actual
findReferencedPagesByItemsmethod.The test has value as documentation of the expected data model. To improve test coverage, consider:
- Using reflection or package-private access to test
findReferencedPagesByItemsdirectly- Testing through public
getEntityMap()which callsfindReferencedPagesByItemsinternally
...er/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lessonbuilder/tool/src/test/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducerTest.java`:
- Around line 194-235: The test testFindReferencedPagesByItemsSkipsOrphans
currently only asserts the result is non-null; change it to assert that the
orphan parent (pageId 200L) is not included in the produced mapping from
producer.findReferencedPagesByItems(siteId). Specifically, in
LessonBuilderEntityProducerTest::testFindReferencedPagesByItemsSkipsOrphans add
an assertion that result does not contain key 200L (e.g.
assertFalse(result.containsKey(200L))) or that 200L is not present in any of the
aggregated referenced ID lists, so the orphan parent is explicitly verified as
skipped.
🧹 Nitpick comments (2)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java (1)
1827-1828: Minor: Clarify comment to match deferred assignments.The comment mentions "parent relationships" but the
makePagesignature has toolId as the first parameter. The parent relationships are set via the 4th and 5th parameters which are already null. Consider updating the comment to reflect what's actually being deferred.💡 Suggested clarification
- // Create page with initial toolId, parent relationships will be set later + // Create page with placeholder toolId; toolId and parent relationships will be set by hierarchy calculation page = simplePageToolDao.makePage("0", siteId, title, null, null);lessonbuilder/tool/src/test/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducerTest.java (1)
237-330: Avoid re-implementing hierarchy logic in tests.These two tests re-create the hierarchy algorithm inline, which can drift from production behavior. Consider extracting the hierarchy calculation into a package-private helper in
LessonBuilderEntityProducerand invoking that directly in tests.
...ool/src/test/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java`:
- Around line 1947-1960: The loop that walks parent chains using
calculatedParentMap (iterating pageId, currentPageId, topParent) must guard
against cycles: for each pageId traversal, track visited IDs (e.g., a local
Set<Long>) and break if currentPageId is seen again (or after a reasonable max
iteration count), then skip setting calculatedTopParentMap for that pageId and
optionally log a warning; update the while loop to check the visited set before
advancing currentPageId to prevent an infinite loop caused by cyclic parent
relationships.
🧹 Nitpick comments (2)
lessonbuilder/tool/src/test/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducerTest.java (2)
237-287: Consider testing the actual production code instead of duplicating the algorithm.This test duplicates the hierarchy calculation algorithm inline rather than invoking the production code. While useful as a specification, it won't catch bugs if the production implementation diverges. Consider refactoring to call the actual merge methods with appropriate mocks, or at minimum, extract this algorithm into a shared utility that both test and production code use.
289-330: Same observation as the previous test - algorithm duplication.Like
testHierarchyCalculationFromReferences, this test validates the algorithm logic but doesn't exercise the production code path. The selective import filtering logic appears correct, but changes to the production implementation wouldn't be caught by this test.
...er/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
Outdated
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)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java (1)
850-856: Guard against blank/nullsiteIdin public API.Line 850: this method is now public and is called during merge; a blank
siteIdwill pass through to DAO and can fail. Add a short guard to return an empty map.🛠️ Proposed fix
public Map<Long, List<Long>> findReferencedPagesByItems(String siteId) { Map<Long, List<Long>> pageToReferencedPages = new HashMap<>(); + if (StringUtils.isBlank(siteId)) { + log.warn("findReferencedPagesByItems called with blank siteId"); + return pageToReferencedPages; + } List<SimplePage> allPages = simplePageToolDao.getSitePages(siteId);
🤖 Fix all issues with AI agents
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java`:
- Around line 918-934: The calculateTopParentMap method may write an incorrect
top parent after breaking out of a detected cycle; modify the loop to track when
a cycle is detected (e.g., boolean cycleDetected set when visited.add returns
false) and only put into calculatedTopParentMap for pageId if no cycle was
detected and topParent != null. Update the logic around variables currentPageId,
topParent, visited to avoid storing any top parent when a cycle was encountered.
...er/tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
Show resolved
Hide resolved
1 participant
https://sakaiproject.atlassian.net/browse/SAK-52251
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.