Merge pull request #5242 from akvo/5241-cumulative-late-previous-peri…

…od-update [#5241] Fix incorrect cumulative periods aggregation values
akvo · Mar 7, 2023 · d6a1668 · d6a1668
2 parents c6c4484 + fd25785
commit d6a1668
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 10 deletions.
diff --git a/akvo/rsr/models/result/utils.py b/akvo/rsr/models/result/utils.py
@@ -7,7 +7,7 @@
 from decimal import Decimal
 
 from django.apps import apps
-from django.db.models import Max, Q
+from django.db.models import Q
 from akvo.utils import rsr_image_path
 
 PERCENTAGE_MEASURE = '2'
@@ -89,4 +89,6 @@ def get_per_user_latest_indicator_update_ids(period):
         period__indicator=period.indicator,
     ).filter(
         Q(period=period) | Q(period__period_end__lt=period.period_end)
-    ).values('user').annotate(id=Max('id')).values('id')
+    ).order_by(
+        'user_id', '-period__period_end', '-created_at'
+    ).distinct('user_id').values('id')
diff --git a/akvo/rsr/tests/results_framework/test_cumulative_updates.py b/akvo/rsr/tests/results_framework/test_cumulative_updates.py
@@ -42,30 +42,30 @@ class SingleUserCumulativeUnitUpdatesTestCase(CumulativeTestMixin, BaseTestCase)
 
     def setUp(self):
         super().setUp()
-        user = self.create_user('test@akvo.org', 'password', is_admin=True)
+        self.user = self.create_user('test@akvo.org', 'password', is_admin=True)
         self.project = self.setup_project()
 
         self.period1 = self.project.get_period(period_start=self.PERIOD_1_START)
-        self.period1.add_update(user=user, value=1, disaggregations={
+        self.period1.add_update(user=self.user, value=1, disaggregations={
             self.DISAGGREGATION_CATEGORY: {
                 self.DISAGGREGATION_TYPE_1: {'value': 1},
             }
         })
-        self.period1.add_update(user=user, value=2, disaggregations={
+        self.period1.add_update(user=self.user, value=2, disaggregations={
             self.DISAGGREGATION_CATEGORY: {
                 self.DISAGGREGATION_TYPE_1: {'value': 1},
                 self.DISAGGREGATION_TYPE_2: {'value': 1},
             }
         })
 
         self.period2 = self.project.get_period(period_start=self.PERIOD_2_START)
-        self.period2.add_update(user=user, value=3, disaggregations={
+        self.period2.add_update(user=self.user, value=3, disaggregations={
             self.DISAGGREGATION_CATEGORY: {
                 self.DISAGGREGATION_TYPE_1: {'value': 2},
                 self.DISAGGREGATION_TYPE_2: {'value': 1},
             }
         })
-        self.period2.add_update(user=user, value=5, disaggregations={
+        self.period2.add_update(user=self.user, value=5, disaggregations={
             self.DISAGGREGATION_CATEGORY: {
                 self.DISAGGREGATION_TYPE_1: {'value': 3},
                 self.DISAGGREGATION_TYPE_2: {'value': 2},
@@ -97,6 +97,33 @@ def test_period_without_updates(self):
         period3 = self.project.periods.get(id=self.period3.id)
         self.assertEqual(5, Decimal(period3.actual_value))
 
+    def test_late_previous_period_update(self):
+        self.period1.add_update(user=self.user, value=3, disaggregations={
+            self.DISAGGREGATION_CATEGORY: {
+                self.DISAGGREGATION_TYPE_1: {'value': 2},
+                self.DISAGGREGATION_TYPE_2: {'value': 1},
+            }
+        })
+        # aggregation task for all periods of an indicator will be generated automatically for cumulative type
+        aggregate(self.period1.object)
+        aggregate(self.period2.object)
+        aggregate(self.period3.object)
+
+        period1 = self.project.periods.get(id=self.period1.id)
+        self.assertEqual(3, Decimal(period1.actual_value))
+        self.assertEqual(2, period1.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_1).value)
+        self.assertEqual(1, period1.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_2).value)
+
+        period2 = self.project.periods.get(id=self.period2.id)
+        self.assertEqual(5, Decimal(period2.actual_value))
+        self.assertEqual(3, period2.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_1).value)
+        self.assertEqual(2, period2.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_2).value)
+
+        period3 = self.project.periods.get(id=self.period3.id)
+        self.assertEqual(5, Decimal(period3.actual_value))
+        self.assertEqual(3, period3.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_1).value)
+        self.assertEqual(2, period3.disaggregations.get(dimension_value__value=self.DISAGGREGATION_TYPE_2).value)
+
 
 class MultiUserCumulativeUnitUpdatesTestCase(CumulativeTestMixin, BaseTestCase):
 

diff --git a/akvo/rsr/usecases/period_update_aggregation.py b/akvo/rsr/usecases/period_update_aggregation.py
@@ -68,9 +68,6 @@ def sum_updates(period: IndicatorPeriod) -> Tuple[Optional[Decimal], Optional[De
 
 
 def sum_cumulative_updates(period: IndicatorPeriod) -> Tuple[Optional[Decimal], Optional[Decimal], Optional[Decimal]]:
-    '''
-    This method assumes the user will submit cumulative updates in chronological order as it should.
-    '''
     IndicatorPeriodData = apps.get_model('rsr', 'IndicatorPeriodData')
     latest_per_users = get_per_user_latest_indicator_update_ids(period)
     value = IndicatorPeriodData.objects.filter(id__in=latest_per_users)\