diff --git a/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy b/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy index b28e28529f..8e68b655f9 100644 --- a/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy +++ b/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy @@ -36,6 +36,7 @@ class CircularLearningPathChecker { // private private BadgeAndSkillsLookup badgeAndSkillsLookup = new BadgeAndSkillsLookup() private PrerequisiteNodeLookup prerequisiteNodeLookup = new PrerequisiteNodeLookup() + private Set allItemIdsOnFinalLearningPath private SkillInfo start // contains all of the badges by following start node in the opposite direction of prerequisite path @@ -182,6 +183,7 @@ class CircularLearningPathChecker { SkillInfo skillInfo = new SkillInfo(projectId: skillDef.projectId, skillId: skillDef.skillId, name: skillDef.name, type: skillDef.type) SkillInfo prereqSkillInfo = new SkillInfo(projectId: prereqSkillDef.projectId, skillId: prereqSkillDef.skillId, name: prereqSkillDef.name, type: prereqSkillDef.type) List path = [skillInfo] + allItemIdsOnFinalLearningPath = constructAllItemIdsOnFinalLearningPath(edgePairs, skillInfo, prereqSkillInfo) startNodeBadgesOnParentPath = [] collectParentBadges(skillInfo, startNodeBadgesOnParentPath, 0) @@ -274,7 +276,9 @@ class CircularLearningPathChecker { badgesOnPath = badgesOnPath.findAll { it.type == SkillDef.ContainerType.Badge } for (SkillInfo badgeOnPathSkillInfo : badgesOnPath) { BadgeAndSkills badge = badgeAndSkillsLookup.getBadgeByBadgeId(badgeOnPathSkillInfo.skillId) - if (badge.badgeHasSkillId(current.skillId) && badge.badgeGraphNode.skillId != current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId) { + if (isBadgeOnCurrentLearningPath(badge) && + badge.badgeHasSkillId(current.skillId) && + badge.badgeGraphNode.skillId != current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId) { return new DependencyCheckResult(possible: false, failureType: DependencyCheckResult.FailureType.BadgeSkillIsAlreadyOnPath, violatingSkillId: current.skillId, @@ -327,21 +331,36 @@ class CircularLearningPathChecker { private DependencyCheckResult checkBadgesForSkillOverlap(BadgeAndSkills badge, List checkAgainst) { for (SkillInfo badgeOnPathSkillInfo : checkAgainst) { BadgeAndSkills badgeOnPath = badgeAndSkillsLookup.getBadgeByBadgeId(badgeOnPathSkillInfo.skillId) - SkillInfo found = badgeOnPath.skills.find { SkillInfo searchFor -> badge.skills.find { searchFor.skillId == it.skillId } } - if (found) { - return new DependencyCheckResult(possible: false, - failureType: DependencyCheckResult.FailureType.BadgeOverlappingSkills, - violatingSkillInBadgeId: badge.badgeGraphNode.skillId, - violatingSkillInBadgeName: badge.badgeGraphNode.name, - violatingSkillId: found.skillId, - violatingSkillName: found.name, - reason: "Multiple badges on the same Learning path cannot have overlapping skills. Both badge [${badge.badgeGraphNode.name}] and [${badgeOnPath.badgeGraphNode.name}] badge have [${found.name}] skill.") + if (isBadgeOnCurrentLearningPath(badgeOnPath)) { + SkillInfo found = badgeOnPath.skills.find { SkillInfo searchFor -> badge.skills.find { searchFor.skillId == it.skillId } } + if (found) { + return new DependencyCheckResult(possible: false, + failureType: DependencyCheckResult.FailureType.BadgeOverlappingSkills, + violatingSkillInBadgeId: badge.badgeGraphNode.skillId, + violatingSkillInBadgeName: badge.badgeGraphNode.name, + violatingSkillId: found.skillId, + violatingSkillName: found.name, + reason: "Multiple badges on the same Learning path cannot have overlapping skills. Both badge [${badge.badgeGraphNode.name}] and [${badgeOnPath.badgeGraphNode.name}] badge have [${found.name}] skill.") + } } } return new DependencyCheckResult() } + private Set constructAllItemIdsOnFinalLearningPath(List edgePairs, SkillInfo skillInfo, SkillInfo prereqSkillInfo) { + Set res = edgePairs.collect { + ["${it.node.projectId}-${it.node.skillId}".toString(), "${it.prerequisite.projectId}-${it.prerequisite.skillId}".toString()] + }.flatten().toSet() + res.add("${skillInfo.projectId}-${skillInfo.skillId}".toString()) + res.add("${prereqSkillInfo.projectId}-${prereqSkillInfo.skillId}".toString()) + return res + } + private boolean isBadgeOnCurrentLearningPath(BadgeAndSkills badge) { + boolean isBadgeOnPath = allItemIdsOnFinalLearningPath.contains("${badge.badgeGraphNode.projectId}-${badge.badgeGraphNode.skillId}".toString()) + return isBadgeOnPath + } + private void collectParentBadges(SkillInfo start, List badges, int currentIteration) { if (currentIteration > 1000) { throw new IllegalStateException("Number of [1000] iterations exceeded when checking for circular dependency for [${start.skillId}]") diff --git a/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy b/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy index 10525a2bdc..88017fc6c8 100644 --- a/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy +++ b/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy @@ -742,4 +742,29 @@ class LearningPathValidationEndpointSpecs extends DefaultIntSpec { !result.violatingSkillName result.reason == "Discovered circular prerequisite [Skill:skill1 -> Skill:skill3 -> Badge:badge1(Skill:skill1)]" } + + def "skills can be belong to multiple badges"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(4, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge1 = SkillsFactory.createBadge(1, 1) + skillsService.createBadge(badge1) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge1.badgeId, skillId: p1Skills[2].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge1.badgeId, skillId: p1Skills[3].skillId]) + + def badge2 = SkillsFactory.createBadge(1, 2) + skillsService.createBadge(badge2) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge2.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge2.badgeId, skillId: p1Skills[3].skillId]) + + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[3].skillId, p1Skills[2].skillId) + + when: + def result = skillsService.vadlidateLearningPathPrerequisite(p1.projectId, p1Skills[1].skillId, p1.projectId, p1Skills[0].skillId) + + then: + result.possible == true + } }