Updates from code review

RedHatInsights · Nov 26, 2024 · b02dd64 · b02dd64
1 parent c4df413
commit b02dd64
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 21 deletions.
diff --git a/aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java b/aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java
@@ -154,7 +154,7 @@ List<AggregationCommand> processAggregateEmailsWithOrgPref(LocalDateTime endTime
             if (!differences.isEmpty()) {
                 Log.warnf("Fetching aggregation from legacy way have more record than the events way:");
                 for (AggregationCommand differencesCommand : differences) {
-                    Log.info(differencesCommand.toString() + differencesCommand.getAggregationKey().hashCode());
+                    Log.info(differencesCommand.toString());
                 }
             }
 
@@ -165,7 +165,7 @@ List<AggregationCommand> processAggregateEmailsWithOrgPref(LocalDateTime endTime
             if (!differences.isEmpty()) {
                 Log.warnf("Fetching aggregation from events have more record than the legacy way:");
                 for (AggregationCommand differencesCommand : differences) {
-                    Log.info(differencesCommand.toString() + differencesCommand.getAggregationKey().hashCode());
+                    Log.info(differencesCommand.toString());
                 }
             }
 

diff --git a/aggregator/src/main/java/com/redhat/cloud/notifications/config/AggregatorConfig.java b/aggregator/src/main/java/com/redhat/cloud/notifications/config/AggregatorConfig.java
@@ -1,9 +1,8 @@
 package com.redhat.cloud.notifications.config;
 
 import com.redhat.cloud.notifications.unleash.ToggleRegistry;
-import com.redhat.cloud.notifications.unleash.UnleashConfigSource;
+import com.redhat.cloud.notifications.unleash.UnleashContextBuilder;
 import io.getunleash.Unleash;
-import io.getunleash.UnleashContext;
 import io.quarkus.logging.Log;
 import jakarta.annotation.PostConstruct;
 import jakarta.enterprise.context.ApplicationScoped;
@@ -69,8 +68,7 @@ public boolean isAggregationBasedOnEventEnabled() {
 
     public boolean isAggregationBasedOnEventEnabledByOrgId(String orgId) {
         if (unleashEnabled) {
-            UnleashContext unleashContext = UnleashConfigSource.buildUnleashContextWithOrgId(orgId);
-            return unleash.isEnabled(fetchAggregationBasedOnEvents, unleashContext, false);
+            return unleash.isEnabled(fetchAggregationBasedOnEvents, UnleashContextBuilder.buildUnleashContextWithOrgId(orgId), false);
         } else {
             return false;
         }

diff --git a/aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java b/aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java
@@ -53,7 +53,7 @@ public List<AggregationCommand> getApplicationsWithPendingAggregationAccordingOr
         LocalDateTime currentTimeTwoDaysAgo = now.minusDays(2);
         String query = "SELECT DISTINCT ev.orgId, ev.bundleId, ev.applicationId, acp.lastRun, bu.name, ap.name FROM Event ev " +
             "join Application ap on ev.applicationId = ap.id join Bundle bu on ev.bundleId = bu.id " +
-            "left join AggregationOrgConfig acp on ev.orgId = acp.orgId " +
+            "join AggregationOrgConfig acp on ev.orgId = acp.orgId " +
             "WHERE ev.orgId in (SELECT DISTINCT es.id.orgId FROM EventTypeEmailSubscription es where es.id.subscriptionType='DAILY' and es.subscribed = true) " +
             "AND ev.created > acp.lastRun AND ev.created > :twoDaysAgo AND ev.created <= :now " +
             "AND :nowTime = acp.scheduledExecutionTime";
@@ -66,7 +66,7 @@ public List<AggregationCommand> getApplicationsWithPendingAggregationAccordingOr
         return records.stream()
             .map(emailAggregationRecord -> new AggregationCommand<>(
                 new EventAggregationCriteria((String) emailAggregationRecord[0], (UUID) emailAggregationRecord[1], (UUID) emailAggregationRecord[2], (String) emailAggregationRecord[4], (String) emailAggregationRecord[5]),
-                ((LocalDateTime) emailAggregationRecord[3]).isBefore(currentTimeTwoDaysAgo) ? ((LocalDateTime) emailAggregationRecord[3]) : currentTimeTwoDaysAgo,
+                ((LocalDateTime) emailAggregationRecord[3]).isBefore(currentTimeTwoDaysAgo) ? currentTimeTwoDaysAgo : ((LocalDateTime) emailAggregationRecord[3]),
                 now,
                 DAILY
             ))

diff --git a/aggregator/src/test/java/com/redhat/cloud/notifications/DailyEventAggregationJobTest.java b/aggregator/src/test/java/com/redhat/cloud/notifications/DailyEventAggregationJobTest.java
@@ -187,15 +187,59 @@ void shouldSentTwoAggregationsToKafkaTopic() {
         checkAggCommand(listCommand, "someOrgId", "rhel", "unknown-application");
     }
 
-    private void checkAggCommand(List<AggregationCommand<EventAggregationCriteria>> commands, String orgId, String bundleName, String applicationName) {
-        Application application = resourceHelpers.findApp(bundleName, applicationName);
+    @Test
+    void shouldNotStartBeforeThanTwoDays() {
+        LocalTime now = baseReferenceTime.toLocalTime();
+        addEventEmailAggregation("someOrgId", "rhel", "policies", "somePolicyId", "someHostId");
+        addEventEmailAggregation("someOrgId", "rhel", "unknown-application", "somePolicyId", "someHostId");
+        dailyEmailAggregationJob.setDefaultDailyDigestTime(now);
+        someOrgIdToProceed.setScheduledExecutionTime(baseReferenceTime.toLocalTime());
+        someOrgIdToProceed.setLastRun(baseReferenceTime.minusDays(5));
+        helpers.addAggregationOrgConfig(someOrgIdToProceed);
+
+        dailyEmailAggregationJob.processDailyEmail();
+
+        List<AggregationCommand<EventAggregationCriteria>> listCommand = getRecordsFromKafka();
+        assertEquals(2, listCommand.size());
+
+        final LocalDateTime expectedStartTime = LocalDateTime.now(UTC)
+            .withHour(baseReferenceTime.getHour())
+            .withMinute(baseReferenceTime.getMinute())
+            .withSecond(baseReferenceTime.getSecond())
+            .withNano(baseReferenceTime.getNano())
+            .minusDays(2);
+
+        checkAggCommand(listCommand, "someOrgId", "rhel", "policies", expectedStartTime);
+        checkAggCommand(listCommand, "someOrgId", "rhel", "unknown-application", expectedStartTime);
+    }
+
+    private void checkAggCommand(final List<AggregationCommand<EventAggregationCriteria>> commands, final String orgId, final String bundleName, final String applicationName, final LocalDateTime expectedStartDate) {
+        final Application application = resourceHelpers.findApp(bundleName, applicationName);
+
+        final LocalDateTime expectedEndDate = LocalDateTime.now(UTC)
+            .withHour(baseReferenceTime.getHour())
+            .withMinute(baseReferenceTime.getMinute())
+            .withSecond(baseReferenceTime.getSecond())
+            .withNano(baseReferenceTime.getNano());
 
         assertTrue(commands.stream().anyMatch(
             com -> orgId.equals(com.getAggregationKey().getOrgId()) &&
                 application.getBundleId().equals(((EventAggregationCriteria) com.getAggregationKey()).getBundleId()) &&
                 application.getId().equals(((EventAggregationCriteria) com.getAggregationKey()).getApplicationId()) &&
-                DAILY.equals(com.getSubscriptionType())
-            ));
+                DAILY.equals(com.getSubscriptionType()) &&
+                com.getStart().isEqual(expectedStartDate) &&
+                com.getEnd().isEqual(expectedEndDate)
+        ));
+    }
+
+    private void checkAggCommand(final List<AggregationCommand<EventAggregationCriteria>> commands, final String orgId, final String bundleName, final String applicationName) {
+        final LocalDateTime expectedStartTime = LocalDateTime.now(UTC)
+            .withHour(baseReferenceTime.getHour())
+            .withMinute(baseReferenceTime.getMinute())
+            .withSecond(baseReferenceTime.getSecond())
+            .withNano(baseReferenceTime.getNano())
+            .minusDays(1);
+        checkAggCommand(commands, orgId, bundleName, applicationName, expectedStartTime);
     }
 
     @Test

diff --git a/common-unleash/src/main/java/com/redhat/cloud/notifications/unleash/UnleashConfigSource.java b/common-unleash/src/main/java/com/redhat/cloud/notifications/unleash/UnleashConfigSource.java
@@ -1,6 +1,5 @@
 package com.redhat.cloud.notifications.unleash;
 
-import io.getunleash.UnleashContext;
 import org.eclipse.microprofile.config.spi.ConfigSource;
 
 import java.util.Map;
@@ -44,10 +43,4 @@ public String getValue(String propertyName) {
         return CONFIG.get(propertyName);
     }
 
-    public static UnleashContext buildUnleashContextWithOrgId(String orgId) {
-        UnleashContext unleashContext = UnleashContext.builder()
-            .addProperty("orgId", orgId)
-            .build();
-        return unleashContext;
-    }
 }
diff --git a/...n-unleash/src/main/java/com/redhat/cloud/notifications/unleash/UnleashContextBuilder.java b/...n-unleash/src/main/java/com/redhat/cloud/notifications/unleash/UnleashContextBuilder.java
@@ -0,0 +1,13 @@
+package com.redhat.cloud.notifications.unleash;
+
+import io.getunleash.UnleashContext;
+
+public class UnleashContextBuilder {
+
+    public static UnleashContext buildUnleashContextWithOrgId(String orgId) {
+        UnleashContext unleashContext = UnleashContext.builder()
+            .addProperty("orgId", orgId)
+            .build();
+        return unleashContext;
+    }
+}
diff --git a/engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java b/engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java
@@ -1,7 +1,7 @@
 package com.redhat.cloud.notifications.config;
 
 import com.redhat.cloud.notifications.unleash.ToggleRegistry;
-import com.redhat.cloud.notifications.unleash.UnleashConfigSource;
+import com.redhat.cloud.notifications.unleash.UnleashContextBuilder;
 import io.getunleash.Unleash;
 import io.quarkus.logging.Log;
 import jakarta.annotation.PostConstruct;
@@ -256,7 +256,7 @@ public boolean isSecuredEmailTemplatesEnabled() {
 
     public boolean isAggregationBasedOnEventEnabled(final String orgId) {
         if (unleashEnabled) {
-            return unleash.isEnabled(fetchAggregationBasedOnEvents, UnleashConfigSource.buildUnleashContextWithOrgId(orgId), false);
+            return unleash.isEnabled(fetchAggregationBasedOnEvents, UnleashContextBuilder.buildUnleashContextWithOrgId(orgId), false);
         } else {
             return false;
         }