Merge pull request #2241 from NationalSecurityAgency/t#2237/learning_…

…path_and_approvers T#2237/learning path and approvers
NationalSecurityAgency · May 30, 2023 · 1dbd78e · 1dbd78e
2 parents c8aa036 + fc1e334
commit 1dbd78e
Show file tree

Hide file tree

Showing 3 changed files with 248 additions and 16 deletions.
diff --git 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> badgeAndSkills = []
         Map<String, BadgeAndSkills> badgeAndSkillsByBadgeId = [:]
+        Map<String, List<SkillInfo>> badgesBySkillId = [:]
         void addAll(List<BadgeAndSkills> badgeAndSkillsInput) {
             badgeAndSkills.addAll(badgeAndSkillsInput)
             badgeAndSkills.each {
                 badgeAndSkillsByBadgeId[it.badgeGraphNode.skillId] = it
             }
+            badgeAndSkills.each { BadgeAndSkills badgeAndSkillsItem ->
+                badgeAndSkillsItem.skills.each {
+                    String skillIdLookup = it.skillId
+                    List<SkillInfo> 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<SkillInfo> 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<SkillInfo> checkTheseBadges = badgeAndSkillsLookup.findBadgesThisSkillBelongsTo(checkItem.skillId)
+                if (checkTheseBadges) {
+                    SkillInfo compliantBadge = checkTheseBadges.find {it.skillId == current.circularCheckBadgeLoadedDueToPreviousSkillFollowingRouteOfBadgeId }
+                    sameItem = (compliantBadge == null)
+                }
+            }
+
+            return sameItem
+        }
         if (sameNodeFound) {
             List<SkillInfo> pathCopy = new ArrayList<>(path)
             pathCopy.add(current)
@@ -219,7 +246,10 @@ class CircularLearningPathChecker {
             List<SkillInfo> 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<SkillInfo> pathCopy = new ArrayList<>(path)
-                    pathCopy.add(current)
-                    DependencyCheckResult res = recursiveCircularPrerequisiteCheck(myBadge, pathCopy, currentIter + 1)
-                    if (!res.possible) {
-                        return res
+                List<SkillInfo> 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<SkillInfo> 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<SkillInfo> 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
@@ -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<String> edges(def graph) {
         def idToSkillIdMap = graph.nodes.collectEntries {[it.id, it]}
         return graph.edges.collect {

diff --git a/.../src/test/java/skills/intTests/dependentSkills/LearningPathValidationEndpointSpecs.groovy b/.../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)]"
+    }
 }