diff --git a/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java b/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java index bf36664b..393b91e3 100644 --- a/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java +++ b/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 getValue() { return value; } diff --git a/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java b/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java index c2600300..5eb980c7 100644 --- a/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java +++ b/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 { @@ -38,12 +40,21 @@ public interface NodeRepository extends TaxonomyRepository { @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 findNodesWithQualityEvaluation(); + + @Modifying + @Query( + """ + UPDATE Node n + SET n.childQualityEvaluationAverage = NULL, + n.childQualityEvaluationCount = 0 """) - List findIdsWithChildren(); + void wipeQualityEvaluationAverages(); @Query( """ diff --git a/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java b/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java index 9ba7c6ca..63b81884 100644 --- a/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java +++ b/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java @@ -102,8 +102,7 @@ protected T updateEntity(URI id, UpdatableDto 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 createEntity(T entity, UpdatableDto 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(); diff --git a/src/main/java/no/ndla/taxonomy/rest/v1/NodeTranslations.java b/src/main/java/no/ndla/taxonomy/rest/v1/NodeTranslations.java index 06d2b7a8..c556d2e3 100644 --- a/src/main/java/no/ndla/taxonomy/rest/v1/NodeTranslations.java +++ b/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); }); } } diff --git a/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java b/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java index b7bec73e..5aaa6308 100644 --- a/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java +++ b/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 oldGrade, UpdatableDto command) { + public void updateQualityEvaluationOfParents(Node node, Optional oldGrade, UpdatableDto command) { if (!(command instanceof NodePostPut nodeCommand)) { return; } - Optional 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 oldGrade, Optional newGrade) { - if (!shouldBeIncludedInQualityEvaluationAverage(nodeType)) { + protected void updateQualityEvaluationOfParents(Node node, Optional oldGrade, Optional 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 parents, Optional oldGrade, Optional newGrade) { - var parentIds = parents.stream().map(DomainEntity::getPublicId).toList(); - updateQualityEvaluationOfRecursive(parentIds, oldGrade, newGrade); + updateQualityEvaluationOfRecursive(parents, oldGrade, newGrade); } @Transactional protected void updateQualityEvaluationOfRecursive( - List parentIds, Optional oldGrade, Optional 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 parents, Optional oldGrade, Optional 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())); } } diff --git a/src/test/java/no/ndla/taxonomy/TestUtils.java b/src/test/java/no/ndla/taxonomy/TestUtils.java index 196e6983..af567586 100644 --- a/src/test/java/no/ndla/taxonomy/TestUtils.java +++ b/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(); diff --git a/src/test/java/no/ndla/taxonomy/rest/v1/AdminTest.java b/src/test/java/no/ndla/taxonomy/rest/v1/AdminTest.java new file mode 100644 index 00000000..959922a2 --- /dev/null +++ b/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); + } +} diff --git a/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java b/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java index f7f8ceac..3ff4206b 100644 --- a/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java +++ b/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java @@ -1321,21 +1321,97 @@ public void that_adding_quality_evaluated_trees_works_recursively() throws Excep connect(t1, t4); - { - var subject = nodeRepository.findFirstByPublicId(URI.create("urn:subject:1")); - var qe = subject.get().getChildQualityEvaluationAverage().get(); - assertEquals(16, qe.getCount()); - assertEquals(3.75, qe.getAverageValue()); - } + 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); + } + + @Test + public void that_deleting_a_bunch_of_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 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)); + + connect(s1, t1); + connect(t1, t2); + + connect(t1, r1); + connect(t1, r2); + connect(t1, r3); + connect(t1, r4); + + testQualityEvaluationAverage(t1, 4, 4.25); + + disconnect(t1, t2); + { - var subject = nodeRepository.findFirstByPublicId(URI.create("urn:subject:1")); - var qe = subject.get().getChildQualityEvaluationAverage().get(); - assertEquals(12, qe.getCount()); - assertEquals(3.8333333333333335, qe.getAverageValue()); + var node = nodeRepository.findFirstByPublicId(t2.getPublicId()); + var qe = node.get().getChildQualityEvaluationAverage(); + assertEquals(false, qe.isPresent()); } + + connect(t2, r5); + connect(t2, r6); + connect(t2, r7); + connect(t2, r8); + + testQualityEvaluationAverage(t2, 4, 3.75); + testQualityEvaluationAverage(t1, 4, 4.25); + + disconnect(s1, t1); + connect(t1, t2); + + testQualityEvaluationAverage(t1, 8, 4.0); + + connect(s1, t1); + + testQualityEvaluationAverage(s1, 8, 4.0); + } + + @Test + public void that_updating_after_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 r1 = builder.node(NodeType.RESOURCE, n -> n.name("R1").qualityEvaluation(Grade.Three)); + + connect(s1, t1); + connect(t1, r1); + + testQualityEvaluationAverage(s1, 1, 3.0); + testQualityEvaluationAverage(t1, 1, 3.0); + + var updateBody = new NodePostPut(); + updateBody.name = Optional.of("Resource 1"); + testUtils.updateResource("/v1/nodes/" + r1.getPublicId(), updateBody); + + testQualityEvaluationAverage(s1, 1, 3.0); + testQualityEvaluationAverage(t1, 1, 3.0); + } + + 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()); } private static class ConnectionTypeCounter { diff --git a/src/test/java/no/ndla/taxonomy/service/ContextUpdaterServiceImplTest.java b/src/test/java/no/ndla/taxonomy/service/ContextUpdaterServiceImplTest.java index 4d11081f..9901a337 100644 --- a/src/test/java/no/ndla/taxonomy/service/ContextUpdaterServiceImplTest.java +++ b/src/test/java/no/ndla/taxonomy/service/ContextUpdaterServiceImplTest.java @@ -36,6 +36,9 @@ void setup(@Autowired NodeRepository nodeRepository, @Autowired NodeConnectionRe this.nodeRepository = nodeRepository; this.nodeConnectionRepository = nodeConnectionRepository; + this.nodeRepository.deleteAllAndFlush(); + this.nodeConnectionRepository.deleteAllAndFlush(); + service = new ContextUpdaterServiceImpl(); }