fix: respect same order of periods in the requests for meta.dimension…

…s.pe [DHIS2-16265] (#17902) * fix: respect same order of periods in the requests for meta.dimensions.pe [DHIS2-16265] * fix: fixes NPE in integration test [DHIS2-16265]
dhis2 · Jun 27, 2024 · 6bcb4bc · 6bcb4bc
1 parent ee0de2a
commit 6bcb4bc
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 56 deletions.
diff --git a/...cs/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java b/...cs/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java
@@ -40,6 +40,7 @@
 import java.util.List;
 import java.util.Map;
 import org.hisp.dhis.analytics.common.params.CommonParams;
+import org.hisp.dhis.analytics.data.handler.MetadataHandler;
 import org.hisp.dhis.calendar.Calendar;
 import org.hisp.dhis.common.DimensionalItemObject;
 import org.hisp.dhis.common.DimensionalObject;
@@ -76,15 +77,8 @@ Map<String, List<String>> handle(Grid grid, CommonParams commonParams) {
         commonParams.delegate().getPeriodDimensionOrFilterItems();
     List<QueryItem> itemFilters = commonParams.delegate().getItemsAsFilters();
 
-    Calendar calendar = PeriodType.getCalendar();
-
-    List<String> periodUids =
-        calendar.isIso8601()
-            ? getUids(periodDimensionOrFilterItems)
-            : getLocalPeriodIdentifiers(periodDimensionOrFilterItems, calendar);
-
     Map<String, List<String>> dimensionItems = new HashMap<>();
-    dimensionItems.put(PERIOD_DIM_ID, periodUids);
+    dimensionItems.put(PERIOD_DIM_ID, getDistinctPeriodUids(periodDimensionOrFilterItems));
 
     for (DimensionalObject dim : allDimensionalObjects) {
       dimensionItems.put(dim.getDimension(), getDimensionalItemIds(dim.getItems()));
@@ -96,6 +90,20 @@ Map<String, List<String>> handle(Grid grid, CommonParams commonParams) {
     return dimensionItems;
   }
 
+  /**
+   * Returns a list of distinct period UIDs. This method was extracted from the original code to be
+   * used in the {@link MetadataDimensionsHandler} and {@link MetadataHandler} classes.
+   *
+   * @param items the list of {@link DimensionalItemObject} of type period.
+   * @return a list of distinct period UIDs.
+   */
+  public static List<String> getDistinctPeriodUids(List<DimensionalItemObject> items) {
+    Calendar calendar = PeriodType.getCalendar();
+    List<String> periodUids =
+        calendar.isIso8601() ? getUids(items) : getLocalPeriodIdentifiers(items, calendar);
+    return periodUids.stream().distinct().toList();
+  }
+
   /**
    * Adds the list of given {@link QueryItem} into the given dimension items map.
    *

diff --git a/...rvice-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java b/...rvice-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java
@@ -106,7 +106,6 @@
 import org.hisp.dhis.period.DateField;
 import org.hisp.dhis.period.Period;
 import org.hisp.dhis.period.RelativePeriodEnum;
-import org.hisp.dhis.period.comparator.AscendingPeriodComparator;
 import org.hisp.dhis.security.acl.AclService;
 import org.hisp.dhis.setting.SystemSettingManager;
 import org.hisp.dhis.user.UserDetails;
@@ -198,15 +197,11 @@ public BaseDimensionalObject getPeriodDimension(List<String> items, Date relativ
         systemSettingManager.getSystemSetting(
             ANALYTICS_FINANCIAL_YEAR_START, AnalyticsFinancialYearStartKey.class);
 
-    boolean containsRelativePeriods = false;
-
     for (String isoPeriod : items) {
       // Contains isoPeriod and timeField
       IsoPeriodHolder isoPeriodHolder = IsoPeriodHolder.of(isoPeriod);
 
       if (RelativePeriodEnum.contains(isoPeriodHolder.getIsoPeriod())) {
-        containsRelativePeriods = true;
-
         String dateField = isoPeriodHolder.getDateField();
         DateField dateAndField = new DateField(relativePeriodDate, dateField);
         addRelativePeriods(
@@ -223,11 +218,7 @@ public BaseDimensionalObject getPeriodDimension(List<String> items, Date relativ
     }
 
     // Remove duplicates
-    periods = periods.stream().distinct().collect(toList());
-
-    if (containsRelativePeriods) {
-      periods.sort(new AscendingPeriodComparator());
-    }
+    periods = periods.stream().distinct().toList();
 
     overridePeriodAttributes(periods, getCalendar());
 
@@ -393,7 +384,7 @@ public BaseDimensionalObject getOrgUnitDimension(
           levels.stream()
               .map(organisationUnitService::getOrganisationUnitLevelByLevel)
               .filter(Objects::nonNull)
-              .collect(toList()));
+              .toList());
     }
 
     if (!groups.isEmpty()) {
@@ -407,7 +398,7 @@ public BaseDimensionalObject getOrgUnitDimension(
                           group.getUid(),
                           group.getCode(),
                           group.getDisplayProperty(displayProperty)))
-              .collect(toList()));
+              .toList());
     }
 
     // When levels / groups are present, OUs are considered boundaries
@@ -492,7 +483,7 @@ private List<DimensionalItemObject> getOrgUnitDimensionItems(
 
     // Remove duplicates
 
-    return ous.stream().distinct().collect(toList());
+    return ous.stream().distinct().toList();
   }
 
   /**
@@ -580,6 +571,6 @@ private List<DimensionalItemObject> getReadableItems(
       UserDetails userDetails, DimensionalObject object) {
     return object.getItems().stream()
         .filter(o -> aclService.canDataOrMetadataRead(userDetails, o))
-        .collect(toList());
+        .toList();
   }
 }
diff --git a/...service-analytics/src/main/java/org/hisp/dhis/analytics/data/handler/MetadataHandler.java b/...service-analytics/src/main/java/org/hisp/dhis/analytics/data/handler/MetadataHandler.java
@@ -35,6 +35,7 @@
 import static org.hisp.dhis.analytics.AnalyticsMetaDataKey.ORG_UNIT_HIERARCHY;
 import static org.hisp.dhis.analytics.AnalyticsMetaDataKey.ORG_UNIT_NAME_HIERARCHY;
 import static org.hisp.dhis.analytics.OutputFormat.DATA_VALUE_SET;
+import static org.hisp.dhis.analytics.common.processing.MetadataDimensionsHandler.getDistinctPeriodUids;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.getCocNameMap;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.getDimensionMetadataItemMap;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.handleGridForDataValueSet;
@@ -43,8 +44,6 @@
 import static org.hisp.dhis.common.DimensionalObject.PERIOD_DIM_ID;
 import static org.hisp.dhis.common.DimensionalObjectUtils.asTypedList;
 import static org.hisp.dhis.common.DimensionalObjectUtils.getDimensionalItemIds;
-import static org.hisp.dhis.common.IdentifiableObjectUtils.getLocalPeriodIdentifiers;
-import static org.hisp.dhis.common.IdentifiableObjectUtils.getUids;
 import static org.hisp.dhis.organisationunit.OrganisationUnit.getParentGraphMap;
 import static org.hisp.dhis.organisationunit.OrganisationUnit.getParentNameGraphMap;
 
@@ -57,11 +56,9 @@
 import org.hisp.dhis.analytics.DataQueryService;
 import org.hisp.dhis.analytics.orgunit.OrgUnitHelper;
 import org.hisp.dhis.analytics.util.AnalyticsOrganisationUnitUtils;
-import org.hisp.dhis.calendar.Calendar;
 import org.hisp.dhis.common.DimensionalObject;
 import org.hisp.dhis.common.Grid;
 import org.hisp.dhis.organisationunit.OrganisationUnit;
-import org.hisp.dhis.period.PeriodType;
 import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.User;
 import org.hisp.dhis.user.UserService;
@@ -110,15 +107,8 @@ public void addMetaData(DataQueryParams params, Grid grid) {
 
       Map<String, Object> dimensionItems = new HashMap<>();
 
-      Calendar calendar = PeriodType.getCalendar();
-
-      List<String> periodUids =
-          calendar.isIso8601()
-              ? getUids(params.getDimensionOrFilterItems(PERIOD_DIM_ID))
-              : getLocalPeriodIdentifiers(
-                  params.getDimensionOrFilterItems(PERIOD_DIM_ID), calendar);
-
-      dimensionItems.put(PERIOD_DIM_ID, periodUids);
+      dimensionItems.put(
+          PERIOD_DIM_ID, getDistinctPeriodUids(params.getDimensionOrFilterItems(PERIOD_DIM_ID)));
       dimensionItems.put(CATEGORYOPTIONCOMBO_DIM_ID, Sets.newHashSet(cocNameMap.keySet()));
 
       for (DimensionalObject dim : params.getDimensionsAndFilters()) {
@@ -183,10 +173,8 @@ void handleDataValueSet(DataQueryParams params, Grid grid) {
    * @param grid the {@link Grid}.
    */
   void applyIdScheme(DataQueryParams params, Grid grid) {
-    if (!params.isSkipMeta()) {
-      if (params.hasCustomIdSchemeSet()) {
-        grid.substituteMetaData(schemeIdResponseMapper.getSchemeIdResponseMap(params));
-      }
+    if (!params.isSkipMeta() && params.hasCustomIdSchemeSet()) {
+      grid.substituteMetaData(schemeIdResponseMapper.getSchemeIdResponseMap(params));
     }
   }
 }
diff --git a/...lytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java b/...lytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java
@@ -80,6 +80,7 @@
 import org.hisp.dhis.organisationunit.OrganisationUnit;
 import org.hisp.dhis.organisationunit.OrganisationUnitService;
 import org.hisp.dhis.period.Period;
+import org.hisp.dhis.period.comparator.AscendingPeriodComparator;
 import org.hisp.dhis.system.grid.GridUtils;
 import org.hisp.dhis.system.grid.ListGrid;
 import org.hisp.dhis.user.CurrentUserUtil;
@@ -374,6 +375,9 @@ private String getPeriodName(OutlierRequest outlierRequest, Outlier outlier) {
                 .stream()
                 .map(p -> (Period) p);
 
-    return periodStream.map(IdentifiableObject::getName).findFirst().orElse(outlier.getPe());
+    return periodStream
+        .min(AscendingPeriodComparator.INSTANCE)
+        .map(Period::getName)
+        .orElse(outlier.getPe());
   }
 }
diff --git a/...e-analytics/src/test/java/org/hisp/dhis/analytics/data/DimensionalObjectProducerTest.java b/...e-analytics/src/test/java/org/hisp/dhis/analytics/data/DimensionalObjectProducerTest.java
@@ -29,7 +29,6 @@
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.sort;
-import static java.util.stream.Collectors.toUnmodifiableList;
 import static org.apache.commons.lang3.StringUtils.substringAfter;
 import static org.hisp.dhis.DhisConvenienceTest.createCategory;
 import static org.hisp.dhis.DhisConvenienceTest.createDataElement;
@@ -54,6 +53,7 @@
 import static org.hisp.dhis.setting.SettingKey.ANALYTICS_FINANCIAL_YEAR_START;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -62,12 +62,15 @@
 import static org.mockito.Mockito.when;
 
 import java.time.LocalDate;
+import java.time.ZoneId;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Stream;
 import org.hisp.dhis.analytics.AnalyticsFinancialYearStartKey;
+import org.hisp.dhis.analytics.common.processing.MetadataDimensionsHandler;
 import org.hisp.dhis.category.Category;
 import org.hisp.dhis.category.CategoryOption;
 import org.hisp.dhis.common.BaseDimensionalItemObject;
@@ -99,6 +102,9 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
@@ -193,10 +199,7 @@ void testGetDimensionForDataElement() {
 
     DataElement refDataElement =
         (DataElement)
-            dimensionalObject.getItems().stream()
-                .filter(de -> de.getUid() != null)
-                .collect(toUnmodifiableList())
-                .get(0);
+            dimensionalObject.getItems().stream().filter(de -> de.getUid() != null).toList().get(0);
 
     assertBaseDimensionalObjects(dataElement, refDataElement);
 
@@ -422,16 +425,16 @@ void testGetPeriodDimensions() {
     String fiveYearsAgo = Integer.toString(currentYear - 5);
 
     List<DimensionalItemObject> refPeriods = dimensionalObject.getItems();
-    assertDailyPeriod(fiveYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(0));
-    assertDailyPeriod(fourYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(1));
-    assertDailyPeriod(threeYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(2));
-    assertDailyPeriod(twoYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(3));
-    assertDailyPeriod(lastYear, "LAST_UPDATED", (Period) refPeriods.get(4));
+    assertDailyPeriod(lastYear, "LAST_UPDATED", (Period) refPeriods.get(0));
+    assertDailyPeriod(fiveYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(1));
+    assertDailyPeriod(fourYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(2));
+    assertDailyPeriod(threeYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(3));
+    assertDailyPeriod(twoYearsAgo, "SCHEDULED_DATE", (Period) refPeriods.get(4));
     assertDailyPeriod(lastYear, "SCHEDULED_DATE", (Period) refPeriods.get(5));
   }
 
   private void assertDailyPeriod(String year, String dateField, Period period) {
-    assertTrue(period.getPeriodType() instanceof YearlyPeriodType);
+    assertInstanceOf(YearlyPeriodType.class, period.getPeriodType());
     assertEquals(year, period.getIsoDate());
     assertEquals(dateField, period.getDateField());
     assertEquals(year + "-01-01", period.getStartDateString());
@@ -458,7 +461,7 @@ void testGetPeriodDimensionForNonIsoPeriod() {
     assertEquals("2021-05-01_2021-06-01", refDimensionKeywords.getKeywords().get(0).getKey());
 
     List<DimensionalItemObject> refPeriods = dimensionalObject.getItems();
-    assertTrue(((Period) refPeriods.get(0)).getPeriodType() instanceof DailyPeriodType);
+    assertInstanceOf(DailyPeriodType.class, ((Period) refPeriods.get(0)).getPeriodType());
     assertEquals("20210501", ((Period) refPeriods.get(0)).getIsoDate());
     assertEquals("LAST_UPDATED", ((Period) refPeriods.get(0)).getDateField());
     assertEquals("2021-05-01", ((Period) refPeriods.get(0)).getStartDateString());
@@ -509,6 +512,71 @@ void testDynamicFromWithAllItems() {
     assertFalse(dimensionalObject.get().getItems().isEmpty());
   }
 
+  @ParameterizedTest
+  @MethodSource("providePeDimensionsForPeriodOrder")
+  void testOrderOfPeriods(List<String> periods, List<String> expected) {
+
+    // Given
+    Date aDayOfJune2024 =
+        Date.from(LocalDate.of(2024, 6, 15).atStartOfDay(ZoneId.systemDefault()).toInstant());
+
+    when(i18nManager.getI18nFormat()).thenReturn(i18nFormat);
+    when(i18nManager.getI18n()).thenReturn(i18n);
+
+    // When
+    BaseDimensionalObject baseDimensionalObject =
+        target.getPeriodDimension(periods, aDayOfJune2024);
+
+    // Then
+    List<DimensionalItemObject> items = baseDimensionalObject.getItems();
+    // we need to assert that baseDimensionalObject.getItems() is ordered as expected
+    for (int i = 0; i < items.size(); i++) {
+      assertEquals(expected.get(i), items.get(i).getUid());
+    }
+  }
+
+  // This method provides the periods and the expected order of the periods for the
+  // testOrderOfPeriods test
+  private static Stream<Arguments> providePeDimensionsForPeriodOrder() {
+    return Stream.of(
+        Arguments.of(List.of("2024", "LAST_YEAR"), List.of("2024", "2023")),
+        Arguments.of(
+            List.of("THIS_YEAR", "LAST_5_YEARS"),
+            List.of("2024", "2019", "2020", "2021", "2022", "2023")),
+        Arguments.of(
+            List.of("LAST_YEAR", "LAST_5_YEARS"), List.of("2023", "2019", "2020", "2021", "2022")),
+        Arguments.of(
+            List.of("2021", "LAST_5_YEARS"), List.of("2021", "2019", "2020", "2022", "2023")),
+        Arguments.of(
+            List.of("LAST_5_YEARS", "2021"), List.of("2019", "2020", "2021", "2022", "2023")));
+  }
+
+  @Test
+  void testMetadataHandlerGetDistinctPeriodUids() {
+
+    // Given
+    Date aDayOfJune2024 =
+        Date.from(LocalDate.of(2024, 6, 15).atStartOfDay(ZoneId.systemDefault()).toInstant());
+
+    when(i18nManager.getI18nFormat()).thenReturn(i18nFormat);
+    when(i18nManager.getI18n()).thenReturn(i18n);
+
+    // When
+    List<DimensionalItemObject> dimensionalItemObjects =
+        target
+            .getPeriodDimension(
+                List.of("LAST_YEAR:LAST_UPDATED", "LAST_5_YEARS:SCHEDULED_DATE"), aDayOfJune2024)
+            .getItems();
+
+    // And
+    List<String> distinctPeriodUids =
+        MetadataDimensionsHandler.getDistinctPeriodUids(dimensionalItemObjects);
+
+    // Then
+    assertEquals(5, distinctPeriodUids.size());
+    assertEquals(List.of("2023", "2019", "2020", "2021", "2022"), distinctPeriodUids);
+  }
+
   private void assertKeywordForDimensionalObject(
       Keyword keyword, BaseDimensionalItemObject dimensionalItemObject) {
     assertEquals(keyword.getKey(), dimensionalItemObject.getUid());

diff --git a/...tegration/src/test/java/org/hisp/dhis/analytics/event/data/EventDataQueryServiceTest.java b/...tegration/src/test/java/org/hisp/dhis/analytics/event/data/EventDataQueryServiceTest.java
@@ -198,10 +198,8 @@ void testGetPeriods() {
     DimensionalObject pe = params.getDimension("pe");
     assertEquals(3, pe.getItems().size());
     assertTrue(streamOfPeriods(pe).anyMatch(Period::isDefault));
-    assertTrue(
-        streamOfPeriods(pe).map(Period::getDateField).anyMatch(s -> s.equals("LAST_UPDATED")));
-    assertTrue(
-        streamOfPeriods(pe).map(Period::getDateField).anyMatch(s -> s.equals("INCIDENT_DATE")));
+    assertTrue(streamOfPeriods(pe).map(Period::getDateField).anyMatch("LAST_UPDATED"::equals));
+    assertTrue(streamOfPeriods(pe).map(Period::getDateField).anyMatch("INCIDENT_DATE"::equals));
     assertTrue(
         streamOfPeriods(pe)
             .filter(period -> "INCIDENT_DATE".equals(period.getDateField()))