diff --git a/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy b/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy index e17de2ab66..b28e28529f 100644 --- a/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy +++ b/service/src/main/java/skills/services/admin/CircularLearningPathChecker.groovy @@ -44,20 +44,33 @@ class CircularLearningPathChecker { static class BadgeAndSkillsLookup { List badgeAndSkills = [] Map badgeAndSkillsByBadgeId = [:] + Map> badgesBySkillId = [:] void addAll(List badgeAndSkillsInput) { badgeAndSkills.addAll(badgeAndSkillsInput) badgeAndSkills.each { badgeAndSkillsByBadgeId[it.badgeGraphNode.skillId] = it } + badgeAndSkills.each { BadgeAndSkills badgeAndSkillsItem -> + badgeAndSkillsItem.skills.each { + String skillIdLookup = it.skillId + List badges = badgesBySkillId[skillIdLookup] + if (badges) { + if (!badges.find { it.skillId == badgeAndSkillsItem.badgeGraphNode.skillId }) { + badges.add(badgeAndSkillsItem.badgeGraphNode) + } + } else { + badgesBySkillId[it.skillId] = [badgeAndSkillsItem.badgeGraphNode] + } + } + } } BadgeAndSkills getBadgeByBadgeId(String badgeId) { return badgeAndSkillsByBadgeId[badgeId]; } - BadgeAndSkills findBadgeThisSkillBelongsTo(String skillId) { - BadgeAndSkills result = badgeAndSkills.find { it.badgeHasSkillId(skillId) } - return result; + List findBadgesThisSkillBelongsTo(String skillId) { + return badgesBySkillId[skillId] } boolean isEmpty() { return badgeAndSkills.isEmpty() @@ -128,6 +141,8 @@ class CircularLearningPathChecker { Boolean circularCheckProvidedBecauseFollowingSkillsUnderBadge = false Boolean circularCheckBadgeLoadedDueToPreviousSkill = false + // when following all of badges for a given skill, keep track which badge Id is follow + String circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId = null @Override protected Object clone() throws CloneNotSupportedException { @@ -185,7 +200,19 @@ class CircularLearningPathChecker { throw new IllegalStateException("Number of [1000] iterations exceeded when checking for circular dependency for [${start.skillId}]") } - SkillInfo sameNodeFound = path.find { it.skillId == current.skillId && it.projectId == current.projectId && it.type == current.type } + SkillInfo sameNodeFound = path.find { SkillInfo checkItem -> + boolean sameItem = checkItem.skillId == current.skillId && checkItem.projectId == current.projectId && checkItem.type == current.type; + boolean skillLoadedDueToBadge = current.type == SkillDef.ContainerType.Skill && current.circularCheckBadgeLoadedDueToPreviousSkill + if (sameItem && skillLoadedDueToBadge) { + List checkTheseBadges = badgeAndSkillsLookup.findBadgesThisSkillBelongsTo(checkItem.skillId) + if (checkTheseBadges) { + SkillInfo compliantBadge = checkTheseBadges.find {it.skillId == current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId } + sameItem = (compliantBadge == null) + } + } + + return sameItem + } if (sameNodeFound) { List pathCopy = new ArrayList<>(path) pathCopy.add(current) @@ -219,7 +246,10 @@ class CircularLearningPathChecker { List pathCopy = new ArrayList<>(path) pathCopy.add(current) for ( SkillInfo pNode : skillsUnderBadgeToCheck ) { - DependencyCheckResult res = recursiveCircularPrerequisiteCheck(pNode, pathCopy, currentIter+1) + SkillInfo skillCopy = pNode.clone() + skillCopy.circularCheckBadgeLoadedDueToPreviousSkill = current.circularCheckBadgeLoadedDueToPreviousSkill + skillCopy.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId = current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId + DependencyCheckResult res = recursiveCircularPrerequisiteCheck(skillCopy, pathCopy, currentIter+1) if (!res.possible) { return res } @@ -244,7 +274,7 @@ 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)) { + if (badge.badgeHasSkillId(current.skillId) && badge.badgeGraphNode.skillId != current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId) { return new DependencyCheckResult(possible: false, failureType: DependencyCheckResult.FailureType.BadgeSkillIsAlreadyOnPath, violatingSkillId: current.skillId, @@ -254,15 +284,22 @@ class CircularLearningPathChecker { } if (!current.circularCheckProvidedBecauseFollowingSkillsUnderBadge) { - BadgeAndSkills badgeIBelongTo = badgeAndSkillsLookup.findBadgeThisSkillBelongsTo(current.skillId) - if (badgeIBelongTo) { - SkillInfo myBadge = badgeIBelongTo.badgeGraphNode.clone() - myBadge.circularCheckBadgeLoadedDueToPreviousSkill = true - List pathCopy = new ArrayList<>(path) - pathCopy.add(current) - DependencyCheckResult res = recursiveCircularPrerequisiteCheck(myBadge, pathCopy, currentIter + 1) - if (!res.possible) { - return res + List badgesIBelongTo = badgeAndSkillsLookup.findBadgesThisSkillBelongsTo(current.skillId) + if (badgesIBelongTo) { + // do not follow try to walk down badge that is currently being checked + if (current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId) { + badgesIBelongTo = badgesIBelongTo.findAll( { it.skillId != current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId }) + } + for (SkillInfo badgeIBelongTo : badgesIBelongTo) { + SkillInfo myBadge = badgeIBelongTo.clone() + myBadge.circularCheckBadgeLoadedDueToPreviousSkill = true + myBadge.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId = myBadge.skillId + List pathCopy = new ArrayList<>(path) + pathCopy.add(current) + DependencyCheckResult res = recursiveCircularPrerequisiteCheck(myBadge, pathCopy, currentIter + 1) + if (!res.possible) { + return res + } } } } @@ -275,7 +312,10 @@ class CircularLearningPathChecker { List pathCopy = new ArrayList<>(path) pathCopy.add(current) for ( SkillInfo pNode : prereqNodes ) { - DependencyCheckResult res = recursiveCircularPrerequisiteCheck(pNode, pathCopy, currentIter+1) + SkillInfo nodeCopy = pNode.clone() + nodeCopy.circularCheckBadgeLoadedDueToPreviousSkill = current.circularCheckBadgeLoadedDueToPreviousSkill + nodeCopy.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId = current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId + DependencyCheckResult res = recursiveCircularPrerequisiteCheck(nodeCopy, pathCopy, currentIter+1) if (!res.possible) { return res } diff --git a/service/src/test/java/skills/intTests/dependentSkills/AdminLearningPathBadgeSpecs.groovy b/service/src/test/java/skills/intTests/dependentSkills/AdminLearningPathBadgeSpecs.groovy index 02f8be5a00..ac72af0821 100644 --- a/service/src/test/java/skills/intTests/dependentSkills/AdminLearningPathBadgeSpecs.groovy +++ b/service/src/test/java/skills/intTests/dependentSkills/AdminLearningPathBadgeSpecs.groovy @@ -386,6 +386,88 @@ class AdminLearningPathBadgeSpecs extends DefaultIntSpec { ].sort() } + def "able to add skill1->skill2 when both skills are under the same badge"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + when: + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[1].skillId) + + def graph = skillsService.getDependencyGraph(p1.projectId) + then: + edges(graph) == [ + "[Skill:${p1Skills[1].skillId}] prerequisite for [Skill:${p1Skills[0].skillId}]", + ].sort() + } + + def "able to add skill1->skill2->skill3 when both skills are under the same badge"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[2].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[3].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + when: + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[1].skillId, p1.projectId, p1Skills[0].skillId) + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[2].skillId, p1.projectId, p1Skills[1].skillId) + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[3].skillId, p1.projectId, p1Skills[2].skillId) + + def graph = skillsService.getDependencyGraph(p1.projectId) + then: + edges(graph) == [ + "[Skill:${p1Skills[0].skillId}] prerequisite for [Skill:${p1Skills[1].skillId}]", + "[Skill:${p1Skills[1].skillId}] prerequisite for [Skill:${p1Skills[2].skillId}]", + "[Skill:${p1Skills[2].skillId}] prerequisite for [Skill:${p1Skills[3].skillId}]", + ].sort() + } + + def "able to add skill1->skill2 when both skills are under 2 separate badges"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + 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[1].skillId]) + badge2.enabled = true + skillsService.createBadge(badge2) + + when: + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[1].skillId) + + def graph = skillsService.getDependencyGraph(p1.projectId) + then: + edges(graph) == [ + "[Skill:${p1Skills[1].skillId}] prerequisite for [Skill:${p1Skills[0].skillId}]", + ].sort() + } + private List edges(def graph) { def idToSkillIdMap = graph.nodes.collectEntries {[it.id, it]} return graph.edges.collect { diff --git a/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy b/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy index 3cee9cfb87..10525a2bdc 100644 --- a/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy +++ b/service/src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy @@ -632,4 +632,114 @@ class LearningPathValidationEndpointSpecs extends DefaultIntSpec { result.violatingSkillName == p1Skills[0].name result.reason == "Badge [Test Badge 1] has skill [Test Skill 1] which already exists on the Learning Path." } + + def "not able to add badge(skill1)-> skill3 -> skill1"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[2].skillId, p1.projectId, badge.badgeId) + when: + def result = skillsService.vadlidateLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[2].skillId) + + then: + result.possible == false + result.failureType == DependencyCheckResult.FailureType.CircularLearningPath.toString() + result.violatingSkillInBadgeId == badge.badgeId + result.violatingSkillInBadgeName == badge.name + !result.violatingSkillId + !result.violatingSkillName + result.reason == "Discovered circular prerequisite [Skill:skill1 -> Skill:skill3 -> Badge:badge1(Skill:skill1)]" + } + + def "not able to add badge(skill1)-> skill3 -> skill1; badge->skill learning path item is added second"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[2].skillId) + when: + def result = skillsService.vadlidateLearningPathPrerequisite(p1.projectId, p1Skills[2].skillId, p1.projectId, badge.badgeId) + + then: + result.possible == false + result.failureType == DependencyCheckResult.FailureType.CircularLearningPath.toString() + !result.violatingSkillInBadgeId + !result.violatingSkillInBadgeName + !result.violatingSkillId + !result.violatingSkillName + result.reason == "Discovered circular prerequisite [Skill:skill3 -> Badge:badge1(Skill:skill1) -> Skill:skill3]" + } + + def "not able to add skill1 -> skill3 -> badge[skill1]"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[2].skillId) + + when: + def result = skillsService.vadlidateLearningPathPrerequisite(p1.projectId, p1Skills[2].skillId, p1.projectId, badge.badgeId) + + then: + result.possible == false + result.failureType == DependencyCheckResult.FailureType.CircularLearningPath.toString() + !result.violatingSkillInBadgeId + !result.violatingSkillInBadgeName + !result.violatingSkillId + !result.violatingSkillName + result.reason == "Discovered circular prerequisite [Skill:skill3 -> Badge:badge1(Skill:skill1) -> Skill:skill3]" + } + + def "not able to add skill1 -> skill3 -> badge(skill1); skill1->skill3 route is added second"() { + def p1 = createProject(1) + def p1subj1 = createSubject(1, 1) + def p1Skills = createSkills(5, 1, 1, 100) + skillsService.createProjectAndSubjectAndSkills(p1, p1subj1, p1Skills) + + def badge = SkillsFactory.createBadge() + skillsService.createBadge(badge) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[0].skillId]) + skillsService.assignSkillToBadge([projectId: p1.projectId, badgeId: badge.badgeId, skillId: p1Skills[1].skillId]) + badge.enabled = true + skillsService.createBadge(badge) + + skillsService.addLearningPathPrerequisite(p1.projectId, p1Skills[2].skillId, p1.projectId, badge.badgeId) + + when: + def result = skillsService.vadlidateLearningPathPrerequisite(p1.projectId, p1Skills[0].skillId, p1.projectId, p1Skills[2].skillId) + + then: + result.possible == false + result.failureType == DependencyCheckResult.FailureType.CircularLearningPath.toString() + result.violatingSkillInBadgeId == badge.badgeId + result.violatingSkillInBadgeName == badge.name + !result.violatingSkillId + !result.violatingSkillName + result.reason == "Discovered circular prerequisite [Skill:skill1 -> Skill:skill3 -> Badge:badge1(Skill:skill1)]" + } }