Merge pull request #267 from NDLANO/taxonomy-investigation-quality-ev…

…aluation

Fix quality evaluation bug when connecting evaluated resources

src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java

@@ -33,6 +33,10 @@ public boolean isDelete() {
         return delete;
     }
 
+    public boolean isChanged() {
+        return delete || value.isPresent();
+    }
+
     public Optional<T> getValue() {
         return value;
     }

src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java

@@ -11,10 +11,12 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Stream;
 import no.ndla.taxonomy.domain.Node;
 import no.ndla.taxonomy.domain.NodeType;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
+import org.springframework.data.jpa.repository.Modifying;
 import org.springframework.data.jpa.repository.Query;
 
 public interface NodeRepository extends TaxonomyRepository<Node> {
@@ -38,12 +40,21 @@ public interface NodeRepository extends TaxonomyRepository<Node> {
 
     @Query(
             """
-            SELECT n.id
+            SELECT n
             FROM Node n
-            LEFT JOIN n.childConnections cc
-            WHERE cc IS NOT NULL
+            LEFT JOIN FETCH n.parentConnections pc
+            WHERE n.qualityEvaluation IS NOT NULL
+            """)
+    Stream<Node> findNodesWithQualityEvaluation();
+
+    @Modifying
+    @Query(
+            """
+            UPDATE Node n
+            SET n.childQualityEvaluationAverage = NULL,
+                n.childQualityEvaluationCount = 0
             """)
-    List<Integer> findIdsWithChildren();
+    void wipeQualityEvaluationAverages();
 
     @Query(
             """

src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java

@@ -102,8 +102,7 @@ protected T updateEntity(URI id, UpdatableDto<T> command) {
         if (entity instanceof Node node) {
             if (contextUpdaterService != null) contextUpdaterService.updateContexts(node);
             if (qualityEvaluationService != null)
-                qualityEvaluationService.updateQualityEvaluationOfParents(
-                        node.getPublicId(), node.getNodeType(), oldGrade, command);
+                qualityEvaluationService.updateQualityEvaluationOfParents(node, oldGrade, command);
         }
 
         return entity;
@@ -129,8 +128,7 @@ protected ResponseEntity<Void> createEntity(T entity, UpdatableDto<T> command) {
             if (entity instanceof Node node) {
                 if (contextUpdaterService != null) contextUpdaterService.updateContexts(node);
                 if (qualityEvaluationService != null)
-                    qualityEvaluationService.updateQualityEvaluationOfParents(
-                            node.getPublicId(), node.getNodeType(), oldGrade, command);
+                    qualityEvaluationService.updateQualityEvaluationOfParents(node, oldGrade, command);
             }
 
             return ResponseEntity.created(location).build();

src/main/java/no/ndla/taxonomy/rest/v1/NodeTranslations.java

@@ -78,7 +78,7 @@ public void createUpdateNodeTranslation(
                     TranslationPUT command) {
         Node node = nodeRepository.getByPublicId(id);
         node.addTranslation(command.name, language);
-        entityManager.persist(node);
+        nodeRepository.save(node);
     }
 
     @DeleteMapping("/{language}")
@@ -96,7 +96,7 @@ public void deleteNodeTranslation(
         Node node = nodeRepository.getByPublicId(id);
         node.getTranslation(language).ifPresent(translation -> {
             node.removeTranslation(language);
-            entityManager.persist(node);
+            nodeRepository.save(node);
         });
     }
 }

src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java

@@ -9,10 +9,7 @@
 
 import java.net.URI;
 import java.util.Collection;
-import java.util.List;
 import java.util.Optional;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.stream.Collectors;
 import no.ndla.taxonomy.domain.*;
 import no.ndla.taxonomy.repositories.NodeRepository;
 import no.ndla.taxonomy.rest.v1.commands.NodePostPut;
@@ -39,23 +36,23 @@ private boolean shouldBeIncludedInQualityEvaluationAverage(NodeType nodeType) {
     }
 
     @Transactional
-    public void updateQualityEvaluationOfParents(
-            URI nodeId, NodeType nodeType, Optional<Grade> oldGrade, UpdatableDto<?> command) {
+    public void updateQualityEvaluationOfParents(Node node, Optional<Grade> oldGrade, UpdatableDto<?> command) {
         if (!(command instanceof NodePostPut nodeCommand)) {
             return;
         }
 
-        Optional<QualityEvaluationDTO> qe =
-                nodeCommand.qualityEvaluation.isDelete() ? Optional.empty() : nodeCommand.qualityEvaluation.getValue();
-        var newGrade = qe.map(QualityEvaluationDTO::getGrade);
+        if (!nodeCommand.qualityEvaluation.isChanged()) {
+            return;
+        }
 
-        updateQualityEvaluationOfParents(nodeId, nodeType, oldGrade, newGrade);
+        var newGrade = nodeCommand.qualityEvaluation.getValue().map(QualityEvaluationDTO::getGrade);
+
+        updateQualityEvaluationOfParents(node, oldGrade, newGrade);
     }
 
     public void updateQualityEvaluationOfNewConnection(Node parent, Node child) {
         // Update parents quality evaluation average with the newly linked one.
-        updateQualityEvaluationOfParents(
-                child.getPublicId(), child.getNodeType(), Optional.empty(), child.getQualityEvaluationGrade());
+        updateQualityEvaluationOfParents(child, Optional.empty(), child.getQualityEvaluationGrade());
 
         child.getChildQualityEvaluationAverage()
                 .ifPresent(childAverage -> addGradeAverageTreeToParents(parent, childAverage));
@@ -79,8 +76,7 @@ public void removeQualityEvaluationOfDeletedConnection(NodeConnection connection
         var child = connectionToDelete.getChild().get();
 
         if (shouldBeIncludedInQualityEvaluationAverage(child.getNodeType())) {
-            updateQualityEvaluationOfParents(
-                    child.getPublicId(), child.getNodeType(), child.getQualityEvaluationGrade(), Optional.empty());
+            updateQualityEvaluationOfParents(child, child.getQualityEvaluationGrade(), Optional.empty());
             return;
         }
 
@@ -92,39 +88,35 @@ public void removeQualityEvaluationOfDeletedConnection(NodeConnection connection
     }
 
     @Transactional
-    public void updateQualityEvaluationOfParents(
-            URI nodeId, NodeType nodeType, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
-        if (!shouldBeIncludedInQualityEvaluationAverage(nodeType)) {
+    protected void updateQualityEvaluationOfParents(Node node, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
+        if (!shouldBeIncludedInQualityEvaluationAverage(node.getNodeType())) {
             return;
         }
         if (oldGrade.isEmpty() && newGrade.isEmpty() || oldGrade.equals(newGrade)) {
             return;
         }
 
-        var node = nodeRepository
-                .findFirstByPublicId(nodeId)
-                .orElseThrow(() -> new NotFoundServiceException("Node was not found"));
-
         updateQualityEvaluationOf(node.getParentNodes(), oldGrade, newGrade);
     }
 
     @Transactional
     public void updateQualityEvaluationOf(
             Collection<Node> parents, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
-        var parentIds = parents.stream().map(DomainEntity::getPublicId).toList();
-        updateQualityEvaluationOfRecursive(parentIds, oldGrade, newGrade);
+        updateQualityEvaluationOfRecursive(parents, oldGrade, newGrade);
     }
 
     @Transactional
     protected void updateQualityEvaluationOfRecursive(
-            List<URI> parentIds, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
-        parentIds.forEach(pid -> nodeRepository.findFirstByPublicId(pid).ifPresent(p -> {
-            p.updateChildQualityEvaluationAverage(oldGrade, newGrade);
-            nodeRepository.save(p);
-            var parentsParents =
-                    p.getParentNodes().stream().map(Node::getPublicId).toList();
-            updateQualityEvaluationOfRecursive(parentsParents, oldGrade, newGrade);
-        }));
+            Collection<Node> parents, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
+        var updatedParents = parents.stream()
+                .peek(p -> {
+                    p.updateChildQualityEvaluationAverage(oldGrade, newGrade);
+                    var parentsParents = p.getParentNodes();
+                    updateQualityEvaluationOfRecursive(parentsParents, oldGrade, newGrade);
+                })
+                .toList();
+
+        nodeRepository.saveAll(updatedParents);
     }
 
     @Transactional
@@ -136,17 +128,11 @@ public void updateEntireAverageTreeForNode(URI publicId) {
         nodeRepository.save(node);
     }
 
+    @Transactional
     public void updateQualityEvaluationOfAllNodes() {
-        var ids = nodeRepository.findIdsWithChildren();
-        final var counter = new AtomicInteger();
-        ids.stream()
-                .collect(Collectors.groupingBy(i -> counter.getAndIncrement() / 1000))
-                .values()
-                .forEach(idChunk -> {
-                    var toSave = nodeRepository.findByIds(idChunk).stream()
-                            .peek(Node::updateEntireAverageTree)
-                            .toList();
-                    nodeRepository.saveAll(toSave);
-                });
+        nodeRepository.wipeQualityEvaluationAverages();
+        var nodeStream = nodeRepository.findNodesWithQualityEvaluation();
+        nodeStream.forEach(
+                node -> updateQualityEvaluationOfParents(node, Optional.empty(), node.getQualityEvaluationGrade()));
     }
 }

src/test/java/no/ndla/taxonomy/TestUtils.java

@@ -66,6 +66,11 @@ public MockHttpServletResponse createResource(String path, Object command) throw
         return createResource(path, command, status().isCreated());
     }
 
+    public MockHttpServletResponse createResource(String path) throws Exception {
+        entityManager.flush();
+        return mockMvc.perform(post(path)).andReturn().getResponse();
+    }
+
     public MockHttpServletResponse createResource(String path, Object command, ResultMatcher resultMatcher)
             throws Exception {
         entityManager.flush();

src/test/java/no/ndla/taxonomy/rest/v1/AdminTest.java

@@ -0,0 +1,152 @@
+/*
+ * Part of NDLA taxonomy-api
+ * Copyright (C) 2021 NDLA
+ *
+ * See LICENSE
+ */
+
+package no.ndla.taxonomy.rest.v1;
+
+import static no.ndla.taxonomy.TestUtils.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import jakarta.persistence.EntityManager;
+import no.ndla.taxonomy.TestSeeder;
+import no.ndla.taxonomy.domain.*;
+import no.ndla.taxonomy.rest.v1.dtos.NodeConnectionPOST;
+import no.ndla.taxonomy.service.dtos.*;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.test.context.transaction.TestTransaction;
+
+public class AdminTest extends RestTest {
+    @Autowired
+    EntityManager entityManager;
+
+    @Autowired
+    private TestSeeder testSeeder;
+
+    @Value(value = "${new.url.separator:false}")
+    private boolean newUrlSeparator;
+
+    @BeforeEach
+    void clearAllRepos() {
+        nodeRepository.deleteAllAndFlush();
+        nodeConnectionRepository.deleteAllAndFlush();
+        resourceTypeRepository.deleteAllAndFlush();
+        resourceResourceTypeRepository.deleteAllAndFlush();
+    }
+
+    public void connect(Node parent, Node child) throws Exception {
+        var connectBody = new NodeConnectionPOST();
+        connectBody.parentId = parent.getPublicId();
+        connectBody.childId = child.getPublicId();
+
+        testUtils.createResource("/v1/node-connections/", connectBody);
+    }
+
+    public void disconnect(Node parent, Node child) throws Exception {
+        var connection = nodeConnectionRepository.findByParentIdAndChildId(parent.getId(), child.getId());
+        var connectionId = connection.getPublicId();
+        testUtils.deleteResource("/v1/node-connections/" + connectionId);
+    }
+
+    public void testQualityEvaluationAverage(Node inputNode, int expectedCount, double expectedAverage) {
+        var node = nodeRepository.findFirstByPublicId(inputNode.getPublicId()).orElseThrow();
+        var qe = node.getChildQualityEvaluationAverage().orElseThrow();
+        assertEquals(expectedCount, qe.getCount());
+        assertEquals(expectedAverage, qe.getAverageValue());
+    }
+
+    public void assertMissingQualityEvaluation(Node inputNode) {
+        var node = nodeRepository.findFirstByPublicId(inputNode.getPublicId()).orElseThrow();
+        var qe = node.getChildQualityEvaluationAverage();
+        assertTrue(qe.isEmpty());
+    }
+
+    @Test
+    public void rebuilding_quality_evaluation_works_as_expected() throws Exception {
+        var s1 = builder.node(NodeType.SUBJECT, s -> s.name("S1").publicId("urn:subject:1"));
+
+        var t1 = builder.node(
+                NodeType.TOPIC, n -> n.name("T1").publicId("urn:topic:1").qualityEvaluation(Grade.Four));
+        var t2 = builder.node(NodeType.TOPIC, n -> n.name("T2").qualityEvaluation(Grade.One));
+        var t3 = builder.node(NodeType.TOPIC, n -> n.name("T3").qualityEvaluation(Grade.Two));
+        var t4 = builder.node(
+                NodeType.TOPIC, n -> n.name("T4").publicId("urn:topic:4").qualityEvaluation(Grade.One));
+
+        var r1 = builder.node(NodeType.RESOURCE, n -> n.name("R1").qualityEvaluation(Grade.Five));
+        var r2 = builder.node(NodeType.RESOURCE, n -> n.name("R2").qualityEvaluation(Grade.Five));
+        var r3 = builder.node(NodeType.RESOURCE, n -> n.name("R3").qualityEvaluation(Grade.Three));
+        var r4 = builder.node(NodeType.RESOURCE, n -> n.name("R4").qualityEvaluation(Grade.Four));
+        var r5 = builder.node(NodeType.RESOURCE, n -> n.name("R5").qualityEvaluation(Grade.One));
+        var r6 = builder.node(NodeType.RESOURCE, n -> n.name("R6").qualityEvaluation(Grade.Five));
+        var r7 = builder.node(NodeType.RESOURCE, n -> n.name("R7").qualityEvaluation(Grade.Five));
+        var r8 = builder.node(NodeType.RESOURCE, n -> n.name("R8").qualityEvaluation(Grade.Four));
+        var r9 = builder.node(NodeType.RESOURCE, n -> n.name("R9").qualityEvaluation(Grade.Four));
+        var r10 = builder.node(NodeType.RESOURCE, n -> n.name("R10").qualityEvaluation(Grade.Five));
+        var r11 = builder.node(NodeType.RESOURCE, n -> n.name("R11").qualityEvaluation(Grade.Three));
+        var r12 = builder.node(NodeType.RESOURCE, n -> n.name("R12").qualityEvaluation(Grade.Two));
+        var r13 = builder.node(NodeType.RESOURCE, n -> n.name("R13").qualityEvaluation(Grade.Five));
+        var r14 = builder.node(NodeType.RESOURCE, n -> n.name("R14").qualityEvaluation(Grade.One));
+        var r15 = builder.node(NodeType.RESOURCE, n -> n.name("R15").qualityEvaluation(Grade.Four));
+        var r16 = builder.node(NodeType.RESOURCE, n -> n.name("R16").qualityEvaluation(Grade.Four));
+
+        connect(s1, t1);
+        connect(t1, t2);
+        connect(t2, t3);
+
+        connect(t1, r1);
+        connect(t1, r2);
+        connect(t1, r3);
+        connect(t1, r4);
+
+        connect(t2, r5);
+        connect(t2, r6);
+        connect(t2, r7);
+        connect(t2, r8);
+
+        connect(t3, r9);
+        connect(t3, r10);
+        connect(t3, r11);
+        connect(t3, r12);
+
+        connect(t4, r13);
+        connect(t4, r14);
+        connect(t4, r15);
+        connect(t4, r16);
+
+        connect(t1, t4);
+
+        nodeRepository.wipeQualityEvaluationAverages();
+        nodeRepository.flush();
+        TestTransaction.flagForCommit();
+        TestTransaction.end();
+        TestTransaction.start();
+
+        assertMissingQualityEvaluation(s1);
+        assertMissingQualityEvaluation(t1);
+        assertMissingQualityEvaluation(t2);
+        assertMissingQualityEvaluation(t3);
+        assertMissingQualityEvaluation(t4);
+
+        testUtils.createResource("/v1/admin/buildAverageTree");
+
+        TestTransaction.flagForCommit();
+        TestTransaction.end();
+        TestTransaction.start();
+
+        testQualityEvaluationAverage(s1, 16, 3.75);
+        testQualityEvaluationAverage(t1, 16, 3.75);
+        testQualityEvaluationAverage(t2, 8, 3.625);
+        testQualityEvaluationAverage(t3, 4, 3.5);
+        testQualityEvaluationAverage(t4, 4, 3.5);
+
+        disconnect(t1, t4);
+
+        testQualityEvaluationAverage(s1, 12, 3.8333333333333335);
+        testQualityEvaluationAverage(t1, 12, 3.8333333333333335);
+    }
+}