diff --git a/src/main/java/no/ndla/taxonomy/domain/GradeAverage.java b/src/main/java/no/ndla/taxonomy/domain/GradeAverage.java index 74a47da4..2ce44577 100644 --- a/src/main/java/no/ndla/taxonomy/domain/GradeAverage.java +++ b/src/main/java/no/ndla/taxonomy/domain/GradeAverage.java @@ -11,23 +11,23 @@ import java.util.Optional; public class GradeAverage { - public GradeAverage(double averageValue, int count) { - this.averageValue = averageValue; + public GradeAverage(int averageSum, int count) { + this.averageSum = averageSum; this.count = count; } - double averageValue; + int averageSum; int count; public static GradeAverage fromGrades(Collection> grades) { var existing = grades.stream().flatMap(Optional::stream).toList(); var count = existing.size(); - var avg = existing.stream().mapToInt(Grade::toInt).average().orElse(0.0); - return new GradeAverage(avg, count); + var sum = existing.stream().mapToInt(Grade::toInt).sum(); + return new GradeAverage(sum, count); } - public double getAverageValue() { - return averageValue; + public int getAverageSum() { + return averageSum; } public int getCount() { @@ -44,11 +44,15 @@ public boolean equals(Object obj) { } GradeAverage that = (GradeAverage) obj; - return Double.compare(that.averageValue, averageValue) == 0 && count == that.count; + return averageSum == that.averageSum && count == that.count; } @Override public String toString() { - return "GradeAverage{averageValue=" + averageValue + ", count=" + count + '}'; + return "GradeAverage{averageSum=" + averageSum + ", count=" + count + '}'; + } + + public double getAverageValue() { + return (double) averageSum / count; } } diff --git a/src/main/java/no/ndla/taxonomy/domain/Node.java b/src/main/java/no/ndla/taxonomy/domain/Node.java index eaafc08f..bd6f971d 100644 --- a/src/main/java/no/ndla/taxonomy/domain/Node.java +++ b/src/main/java/no/ndla/taxonomy/domain/Node.java @@ -83,8 +83,8 @@ public class Node extends DomainObject implements EntityWithMetadata { @Column(name = "quality_evaluation_comment") private String qualityEvaluationComment; - @Column(name = "child_quality_evaluation_average") - private Double childQualityEvaluationAverage; + @Column(name = "child_quality_evaluation_sum") + private int childQualityEvaluationSum; @Column(name = "child_quality_evaluation_count") private int childQualityEvaluationCount; @@ -147,25 +147,33 @@ public Optional getQualityEvaluationNote() { } public Optional getChildQualityEvaluationAverage() { - return Optional.ofNullable(this.childQualityEvaluationAverage) - .map(avg -> new GradeAverage(avg, this.childQualityEvaluationCount)); + if (this.childQualityEvaluationSum == 0) { + return Optional.empty(); + } + var gradeAverage = new GradeAverage(this.childQualityEvaluationSum, this.childQualityEvaluationCount); + return Optional.of(gradeAverage); } public void addGradeAverageTreeToAverageCalculation(GradeAverage newGradeAverage) { var childAvg = getChildQualityEvaluationAverage(); if (childAvg.isEmpty()) { - this.childQualityEvaluationAverage = newGradeAverage.getAverageValue(); + this.childQualityEvaluationSum = newGradeAverage.getAverageSum(); this.childQualityEvaluationCount = newGradeAverage.getCount(); return; } - var oldSum = childAvg.get().getAverageValue() * childAvg.get().getCount(); - var sumToAdd = newGradeAverage.getAverageValue() * newGradeAverage.getCount(); + var oldSum = childAvg.get().getAverageSum(); + var sumToAdd = newGradeAverage.getAverageSum(); var newSum = oldSum + sumToAdd; var newCount = childAvg.get().getCount() + newGradeAverage.getCount(); - this.childQualityEvaluationAverage = newSum / newCount; - this.childQualityEvaluationCount = newCount; + if (newCount > 0) { + this.childQualityEvaluationSum = newSum; + this.childQualityEvaluationCount = newCount; + } else { + this.childQualityEvaluationSum = 0; + this.childQualityEvaluationCount = 0; + } } public void removeGradeAverageTreeFromAverageCalculation(GradeAverage previousGradeAverage) { @@ -178,17 +186,17 @@ public void removeGradeAverageTreeFromAverageCalculation(GradeAverage previousGr return; } - var totalSum = childAvg.get().getAverageValue() * childAvg.get().getCount(); - var sumToRemove = previousGradeAverage.getAverageValue() * previousGradeAverage.getCount(); + var totalSum = childAvg.get().getAverageSum(); + var sumToRemove = previousGradeAverage.getAverageSum(); var newSum = totalSum - sumToRemove; var newCount = childAvg.get().getCount() - previousGradeAverage.getCount(); if (newCount == 0) { - this.childQualityEvaluationAverage = null; + this.childQualityEvaluationSum = 0; this.childQualityEvaluationCount = 0; } else { - this.childQualityEvaluationAverage = newSum / newCount; + this.childQualityEvaluationSum = newSum; this.childQualityEvaluationCount = newCount; } } @@ -197,42 +205,30 @@ public void updateChildQualityEvaluationAverage(Optional previousGrade, O var childAvg = getChildQualityEvaluationAverage(); if (childAvg.isEmpty()) { newGrade.ifPresent(ng -> { - this.childQualityEvaluationAverage = (double) ng.toInt(); + this.childQualityEvaluationSum = ng.toInt(); this.childQualityEvaluationCount = 1; }); return; } var avg = childAvg.get(); - - if (Double.isNaN(avg.averageValue)) { - logger.warn( - "Child quality evaluation average of node '{}' is NaN. Recalculating entire tree.", - this.getPublicId()); - updateEntireAverageTree(); - return; - } - if (previousGrade.isEmpty() && newGrade.isEmpty()) return; else if (previousGrade.isEmpty()) { // New grade is present var newCount = avg.getCount() + 1; - var newSum = ((avg.averageValue * avg.getCount()) + newGrade.get().toInt()); - var newAverage = newSum / newCount; + var newSum = avg.averageSum + newGrade.get().toInt(); this.childQualityEvaluationCount = newCount; - this.childQualityEvaluationAverage = newAverage; + this.childQualityEvaluationSum = newSum; } else if (newGrade.isEmpty()) { // Previous grade is present var newCount = avg.getCount() - 1; - var oldSum = (avg.averageValue * avg.getCount()); + var oldSum = avg.getAverageSum(); var newSum = oldSum - previousGrade.get().toInt(); - var newAverage = newCount > 0 ? newSum / newCount : null; this.childQualityEvaluationCount = newCount; - this.childQualityEvaluationAverage = newAverage; + this.childQualityEvaluationSum = newSum; } else { // Both grades are present - var oldSum = avg.averageValue * avg.getCount(); + var oldSum = avg.getAverageSum(); var newSum = oldSum - previousGrade.get().toInt() + newGrade.get().toInt(); - var newAverage = newSum / avg.getCount(); this.childQualityEvaluationCount = avg.getCount(); - this.childQualityEvaluationAverage = newAverage; + this.childQualityEvaluationSum = newSum; } } @@ -246,10 +242,10 @@ public void updateEntireAverageTree() { gradeAverage); if (gradeAverage.count == 0) { - this.childQualityEvaluationAverage = null; + this.childQualityEvaluationSum = 0; this.childQualityEvaluationCount = 0; } else if (gradeAverage.count > 0) { - this.childQualityEvaluationAverage = gradeAverage.averageValue; + this.childQualityEvaluationSum = gradeAverage.getAverageSum(); this.childQualityEvaluationCount = gradeAverage.count; } } diff --git a/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java b/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java index 7e827389..56d7f9d1 100644 --- a/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java +++ b/src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java @@ -28,6 +28,10 @@ public static UpdateOrDelete Default() { return new UpdateOrDelete<>(); } + public static UpdateOrDelete Update(T value) { + return new UpdateOrDelete<>(value, false); + } + public boolean isDelete() { return delete; } diff --git a/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java b/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java index 609b7fbb..8dedcd07 100644 --- a/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java +++ b/src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java @@ -47,11 +47,11 @@ public interface NodeRepository extends TaxonomyRepository { """) Stream findNodesWithQualityEvaluation(); - @Modifying + @Modifying(flushAutomatically = true, clearAutomatically = true) @Query( """ UPDATE Node n - SET n.childQualityEvaluationAverage = NULL, + SET n.childQualityEvaluationSum = 0, n.childQualityEvaluationCount = 0 """) void wipeQualityEvaluationAverages(); 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 63b81884..03c800ee 100644 --- a/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java +++ b/src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java @@ -76,7 +76,7 @@ protected void deleteEntity(@PathVariable("id") URI id) { if (parents.isPresent()) { var p = parents.get(); - qualityEvaluationService.updateQualityEvaluationOf(p, oldGrade, Optional.empty()); + qualityEvaluationService.updateQualityEvaluationOfRecursive(p, oldGrade, Optional.empty()); } } diff --git a/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java b/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java index 5aaa6308..2b032c03 100644 --- a/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java +++ b/src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java @@ -96,17 +96,11 @@ protected void updateQualityEvaluationOfParents(Node node, Optional oldGr return; } - updateQualityEvaluationOf(node.getParentNodes(), oldGrade, newGrade); + updateQualityEvaluationOfRecursive(node.getParentNodes(), oldGrade, newGrade); } @Transactional - public void updateQualityEvaluationOf( - Collection parents, Optional oldGrade, Optional newGrade) { - updateQualityEvaluationOfRecursive(parents, oldGrade, newGrade); - } - - @Transactional - protected void updateQualityEvaluationOfRecursive( + public void updateQualityEvaluationOfRecursive( Collection parents, Optional oldGrade, Optional newGrade) { var updatedParents = parents.stream() .peek(p -> { diff --git a/src/main/java/no/ndla/taxonomy/service/dtos/GradeAverageDTO.java b/src/main/java/no/ndla/taxonomy/service/dtos/GradeAverageDTO.java index 31918104..705be99d 100644 --- a/src/main/java/no/ndla/taxonomy/service/dtos/GradeAverageDTO.java +++ b/src/main/java/no/ndla/taxonomy/service/dtos/GradeAverageDTO.java @@ -29,7 +29,8 @@ public static double roundToSingleDecimal(double value) { public static Optional fromNode(Node node) { return node.getChildQualityEvaluationAverage().map(ga -> { - var roundedAvg = roundToSingleDecimal(ga.getAverageValue()); + var avg = (double) ga.getAverageSum() / ga.getCount(); + var roundedAvg = roundToSingleDecimal(avg); var count1 = ga.getCount(); return new GradeAverageDTO(roundedAvg, count1); }); diff --git a/src/main/resources/db-master-changelog.xml b/src/main/resources/db-master-changelog.xml index 83d71440..724dde88 100644 --- a/src/main/resources/db-master-changelog.xml +++ b/src/main/resources/db-master-changelog.xml @@ -1602,4 +1602,11 @@ + + + + + + + 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 336da488..530793c8 100644 --- a/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java +++ b/src/test/java/no/ndla/taxonomy/rest/v1/NodesTest.java @@ -13,10 +13,7 @@ import jakarta.persistence.EntityManager; import java.net.URI; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Optional; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; import no.ndla.taxonomy.TestSeeder; @@ -28,6 +25,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.transaction.TestTransaction; public class NodesTest extends RestTest { @Autowired @@ -1128,7 +1126,7 @@ public void update_and_deletion_of_quality_evaluation_does_not_break_calculation var dbTopic = nodeRepository.getByPublicId(URI.create(topicId)); var dbSuject = nodeRepository.getByPublicId(URI.create(subjectId)); assertEquals(dbResource.getQualityEvaluationGrade(), Optional.of(Grade.Five)); - var expectedFive = Optional.of(new GradeAverage(5.0, 1)); + var expectedFive = Optional.of(new GradeAverage(5, 1)); assertEquals(dbSuject.getChildQualityEvaluationAverage(), expectedFive); assertEquals(dbTopic.getChildQualityEvaluationAverage(), expectedFive); } @@ -1292,6 +1290,14 @@ public void connect(Node parent, Node child) throws Exception { testUtils.createResource("/v1/node-connections/", connectBody); } + public void connectId(URI parentId, URI childId) throws Exception { + var connectBody = new NodeConnectionPOST(); + connectBody.parentId = parentId; + connectBody.childId = childId; + + 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(); @@ -1438,6 +1444,168 @@ public void that_updating_after_quality_evaluation_works_as_expected() throws Ex testQualityEvaluationAverage(t1, 1, 3.0); } + public URI createNode(String name, NodeType nodeType, Optional qualityEvaluation) { + var n = new NodePostPut(); + n.name = Optional.of(name); + n.nodeType = nodeType; + qualityEvaluation.ifPresent(x -> { + var comment = Optional.of("La til karakter " + x + " her da..."); + var qe = new QualityEvaluationDTO(Grade.fromInt(x), comment); + n.qualityEvaluation = UpdateOrDelete.Update(qe); + }); + + try { + return getId(testUtils.createResource("/v1/nodes", n)); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + public List getQualityEvaluationGradesOf(List ids) { + return ids.stream() + .map(id -> nodeRepository + .getByPublicId(id) + .getQualityEvaluationGrade() + .orElseThrow() + .toInt()) + .toList(); + } + + public List getQEAverageGradesOf(List ids) { + return ids.stream() + .map(id -> nodeRepository + .getByPublicId(id) + .getChildQualityEvaluationAverage() + .orElse(null)) + .toList(); + } + + @Test + public void update_super_nested_quality_evaluation() throws Exception { + // Create subjects + var s1id = createNode("S1", NodeType.SUBJECT, Optional.of(5)); + var s2id = createNode("S2", NodeType.SUBJECT, Optional.of(4)); + var s3id = createNode("S3", NodeType.SUBJECT, Optional.of(3)); + + // Create topics + var t1id = createNode("S1T1", NodeType.TOPIC, Optional.of(5)); + var t2id = createNode("S1T2", NodeType.TOPIC, Optional.of(5)); + var t3id = createNode("S2T3", NodeType.TOPIC, Optional.of(5)); + var t4id = createNode("S1T1T4", NodeType.TOPIC, Optional.of(2)); + var t5id = createNode("S1T1T5", NodeType.TOPIC, Optional.of(3)); + var t6id = createNode("S1T2T6", NodeType.TOPIC, Optional.of(4)); + var t7id = createNode("S1T4T7", NodeType.TOPIC, Optional.of(3)); + var t8id = createNode("S1T4T8", NodeType.TOPIC, Optional.of(2)); + var t9id = createNode("S1T6T9", NodeType.TOPIC, Optional.of(3)); + var t10id = createNode("S1T5T10", NodeType.TOPIC, Optional.of(2)); + var t11id = createNode("S1T4T7T11", NodeType.TOPIC, Optional.of(5)); + var t12id = createNode("S1T4T7T12", NodeType.TOPIC, Optional.of(4)); + + // Hook topics to subjects + connectId(s1id, t1id); + connectId(s1id, t2id); + connectId(s2id, t3id); + connectId(t1id, t4id); + connectId(t1id, t5id); + connectId(t2id, t6id); + connectId(t4id, t7id); + connectId(t4id, t8id); + connectId(t6id, t9id); + connectId(t5id, t10id); + connectId(t7id, t11id); + connectId(t7id, t12id); + + // Create resources + var r1id = createNode("S1T1R1", NodeType.RESOURCE, Optional.of(5)); + var r2id = createNode("S1T1R2", NodeType.RESOURCE, Optional.of(4)); + var r3id = createNode("S1T2R3", NodeType.RESOURCE, Optional.of(3)); + var r4id = createNode("S1T2T6R4", NodeType.RESOURCE, Optional.of(5)); + var r5id = createNode("S2T3R5", NodeType.RESOURCE, Optional.of(4)); + var r6id = createNode("S1T1T4R6", NodeType.RESOURCE, Optional.of(2)); + var r7id = createNode("S1T6R7", NodeType.RESOURCE, Optional.of(2)); + var r8id = createNode("S1T4T7R8", NodeType.RESOURCE, Optional.of(3)); + var r9id = createNode("S1T4T7R9", NodeType.RESOURCE, Optional.of(1)); + var r10id = createNode("S1T4T8R10", NodeType.RESOURCE, Optional.of(1)); + var r11id = createNode("S1T5T10R11", NodeType.RESOURCE, Optional.of(4)); + var r12id = createNode("S1T5T10R12", NodeType.RESOURCE, Optional.of(2)); + var r13id = createNode("S1T5T10R13", NodeType.RESOURCE, Optional.of(3)); + var r14id = createNode("S1T5T10R14", NodeType.RESOURCE, Optional.of(5)); + var r15id = createNode("S1T4T7T11R15", NodeType.RESOURCE, Optional.of(1)); + var r16id = createNode("S1T4T7T11R16", NodeType.RESOURCE, Optional.of(2)); + var r17id = createNode("S1T4T7T11R17", NodeType.RESOURCE, Optional.of(3)); + var r18id = createNode("S1T4T7T11R18", NodeType.RESOURCE, Optional.of(2)); + var r19id = createNode("S1T4T7T12R19", NodeType.RESOURCE, Optional.of(4)); + var r20id = createNode("S1T4T7T12R20", NodeType.RESOURCE, Optional.of(1)); + + // Hook resources to topics + connectId(t1id, r1id); + connectId(t1id, r2id); + connectId(t2id, r3id); + connectId(t6id, r4id); + connectId(t3id, r5id); + connectId(t4id, r6id); + connectId(t6id, r7id); + connectId(t7id, r8id); + connectId(t7id, r9id); + connectId(t8id, r10id); + connectId(t10id, r11id); + connectId(t10id, r12id); + connectId(t10id, r13id); + connectId(t10id, r14id); + connectId(t11id, r15id); + connectId(t11id, r16id); + connectId(t11id, r17id); + connectId(t11id, r18id); + connectId(t12id, r19id); + connectId(t12id, r20id); + + var allIds = List.of( + s1id, s2id, s3id, t1id, t2id, t3id, t4id, t5id, t6id, t7id, t8id, t9id, t10id, t11id, t12id, r1id, r2id, + r3id, r4id, r5id, r6id, r7id, r8id, r9id, r10id, r11id, r12id, r13id, r14id, r15id, r16id, r17id, r18id, + r19id, r20id); + + var originalGrades = getQualityEvaluationGradesOf(allIds); + var originalAvgs = getQEAverageGradesOf(allIds); + + testUtils.createResource("/v1/admin/buildAverageTree"); + nodeRepository.flush(); + TestTransaction.flagForCommit(); + TestTransaction.end(); + TestTransaction.start(); + + var originalAfterRebuild = getQualityEvaluationGradesOf(allIds); + var originalAvgsAfterRebuild = getQEAverageGradesOf(allIds); + assertEquals(originalAfterRebuild, originalGrades); + assertEquals(originalAvgsAfterRebuild, originalAvgs); + + // Update all quality evaluations + var updateBody = new NodePostPut(); + updateBody.qualityEvaluation = UpdateOrDelete.Update(getRandomGrade()); + for (var id : allIds) { + testUtils.updateResource("/v1/nodes/" + id, updateBody); + } + + var updatedGrades = getQualityEvaluationGradesOf(allIds); + var updatedAvgs = getQEAverageGradesOf(allIds); + + testUtils.createResource("/v1/admin/buildAverageTree"); + nodeRepository.flush(); + TestTransaction.flagForCommit(); + TestTransaction.end(); + TestTransaction.start(); + + var updatedAfterRebuild = getQualityEvaluationGradesOf(allIds); + var updatedAvgsAfterRebuild = getQEAverageGradesOf(allIds); + assertEquals(updatedGrades, updatedAfterRebuild); + assertEquals(updatedAvgs, updatedAvgsAfterRebuild); + } + + public QualityEvaluationDTO getRandomGrade() { + var x = new Random().nextInt(5) + 1; + var dto = new QualityEvaluationDTO(Grade.fromInt(x), Optional.of("Random grade " + x)); + return dto; + } + public void testQualityEvaluationAverage(Node inputNode, int expectedCount, double expectedAverage) { var node = nodeRepository.findFirstByPublicId(inputNode.getPublicId()).orElseThrow(); var qe = node.getChildQualityEvaluationAverage().orElseThrow();