From 6bcb4bcd951bd00ec722433fe95654efba8e2f59 Mon Sep 17 00:00:00 2001 From: Giuseppe Nespolino Date: Thu, 27 Jun 2024 20:16:20 +0200 Subject: [PATCH] fix: respect same order of periods in the requests for meta.dimensions.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] --- .../processing/MetadataDimensionsHandler.java | 24 +++-- .../data/DimensionalObjectProducer.java | 19 +--- .../data/handler/MetadataHandler.java | 22 +---- .../service/AnalyticsOutlierService.java | 6 +- .../data/DimensionalObjectProducerTest.java | 92 ++++++++++++++++--- .../event/data/EventDataQueryServiceTest.java | 6 +- 6 files changed, 113 insertions(+), 56 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java index 5897411889c3..635941a08177 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java +++ b/dhis-2/dhis-services/dhis-service-analytics/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> handle(Grid grid, CommonParams commonParams) { commonParams.delegate().getPeriodDimensionOrFilterItems(); List itemFilters = commonParams.delegate().getItemsAsFilters(); - Calendar calendar = PeriodType.getCalendar(); - - List periodUids = - calendar.isIso8601() - ? getUids(periodDimensionOrFilterItems) - : getLocalPeriodIdentifiers(periodDimensionOrFilterItems, calendar); - Map> 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> 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 getDistinctPeriodUids(List items) { + Calendar calendar = PeriodType.getCalendar(); + List 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/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java index 271366b52038..a614a18dd319 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java +++ b/dhis-2/dhis-services/dhis-service-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 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 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 getOrgUnitDimensionItems( // Remove duplicates - return ous.stream().distinct().collect(toList()); + return ous.stream().distinct().toList(); } /** @@ -580,6 +571,6 @@ private List getReadableItems( UserDetails userDetails, DimensionalObject object) { return object.getItems().stream() .filter(o -> aclService.canDataOrMetadataRead(userDetails, o)) - .collect(toList()); + .toList(); } } diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/handler/MetadataHandler.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/handler/MetadataHandler.java index 1a56a6a7410f..8ea70c268f46 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/data/handler/MetadataHandler.java +++ b/dhis-2/dhis-services/dhis-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 dimensionItems = new HashMap<>(); - Calendar calendar = PeriodType.getCalendar(); - - List 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/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java index 53614c111c4b..075e89b654fc 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java +++ b/dhis-2/dhis-services/dhis-service-analytics/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/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/data/DimensionalObjectProducerTest.java b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/data/DimensionalObjectProducerTest.java index 0e09a128c906..d35224e77d7e 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/data/DimensionalObjectProducerTest.java +++ b/dhis-2/dhis-services/dhis-service-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 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 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 periods, List 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 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 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 dimensionalItemObjects = + target + .getPeriodDimension( + List.of("LAST_YEAR:LAST_UPDATED", "LAST_5_YEARS:SCHEDULED_DATE"), aDayOfJune2024) + .getItems(); + + // And + List 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/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/analytics/event/data/EventDataQueryServiceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/analytics/event/data/EventDataQueryServiceTest.java index a0da7ddb15b4..9115f8b6d8f9 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/analytics/event/data/EventDataQueryServiceTest.java +++ b/dhis-2/dhis-test-integration/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()))