Code review fixes

Author: enricocolasante

src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineAnalyzer.kt

@@ -1,5 +1,6 @@
 package org.hisp.dhis.rules.engine
 
+import org.hisp.dhis.lib.expression.Expression
 import org.hisp.dhis.rules.api.RuleContextRequirements
 import org.hisp.dhis.rules.models.Rule
 import org.hisp.dhis.rules.models.RuleVariable
@@ -14,7 +15,6 @@ import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_ENROLLMENT_DATE
 import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_ENROLLMENT_ID
 import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_ENROLLMENT_STATUS
 import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_EVENT_COUNT
-import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_INCIDENT_DATE
 import org.hisp.dhis.rules.utils.RuleEngineUtils.ENV_VAR_TEI_COUNT
 
 internal object RuleEngineAnalyzer {
@@ -24,7 +24,6 @@ internal object RuleEngineAnalyzer {
         ENV_VAR_ENROLLMENT_DATE,
         ENV_VAR_ENROLLMENT_ID,
         ENV_VAR_ENROLLMENT_STATUS,
-        ENV_VAR_INCIDENT_DATE,
         ENV_VAR_TEI_COUNT,
     )
 
@@ -34,47 +33,62 @@ internal object RuleEngineAnalyzer {
     ): RuleContextRequirements {
         val byName = variables.associateBy { it.name }
         val hasNonCalculatedVariables = variables.any { it !is RuleVariableCalculatedValue }
+        val acc = RequirementsAccumulator(byName)
 
+        for (rule in rules) {
+            if (acc.isComplete) break
+            for (expr in rule.expressions()) {
+                acc.processEnvVars(expr)
+                if (hasNonCalculatedVariables) acc.processVarNames(expr)
+                acc.processOrgUnitGroups(expr)
+            }
+        }
+
+        return acc.toRequirements()
+    }
+
+    private fun Rule.expressions(): List<Expression> = buildList {
+        conditionExpression.getOrNull()?.let { add(it) }
+        actions.forEach { action -> action.dataExpression.getOrNull()?.let { add(it) } }
+    }
+
+    private class RequirementsAccumulator(private val byName: Map<String, RuleVariable>) {
         var needsAllEvents = false
         var needsEnrollment = false
         var needsDataValues = false
         var needsAttributes = false
         var needsOrgUnitGroups = false
 
-        for (rule in rules) {
-            if (needsAllEvents && needsEnrollment && needsDataValues && needsAttributes && needsOrgUnitGroups) break
+        val isComplete get() = needsAllEvents && needsEnrollment && needsDataValues && needsAttributes && needsOrgUnitGroups
 
-            val expressions = buildList {
-                rule.conditionExpression.getOrNull()?.let { add(it) }
-                rule.actions.forEach { action -> action.dataExpression.getOrNull()?.let { add(it) } }
+        fun processEnvVars(expr: Expression) {
+            if (needsAllEvents && needsEnrollment) return
+            for (envVar in expr.collectProgramVariablesNames()) {
+                needsAllEvents = needsAllEvents || envVar in MULTI_EVENT_ENV_VARS
+                needsEnrollment = needsEnrollment || envVar in ENROLLMENT_ENV_VARS
+                if (needsAllEvents && needsEnrollment) break
             }
+        }
 
-            for (expr in expressions) {
-                if (!needsAllEvents || !needsEnrollment) {
-                    for (envVar in expr.collectProgramVariablesNames()) {
-                        needsAllEvents = needsAllEvents || envVar in MULTI_EVENT_ENV_VARS
-                        needsEnrollment = needsEnrollment || envVar in ENROLLMENT_ENV_VARS
-                        if (needsAllEvents && needsEnrollment) break
-                    }
-                }
-
-                if (hasNonCalculatedVariables && (!needsAllEvents || !needsDataValues || !needsAttributes)) {
-                    for (name in expr.collectProgramRuleVariableNames()) {
-                        val v = byName[name]
-                        val isMultiEvent = v is RuleVariableNewestEvent || v is RuleVariableNewestStageEvent || v is RuleVariablePreviousEvent
-                        needsAllEvents = needsAllEvents || isMultiEvent
-                        needsDataValues = needsDataValues || isMultiEvent || v is RuleVariableCurrentEvent
-                        needsAttributes = needsAttributes || v is RuleVariableAttribute
-                        needsEnrollment = needsEnrollment || needsAttributes
-                        if (needsAllEvents && needsDataValues && needsAttributes) break
-                    }
-                }
-
-
-                needsOrgUnitGroups = needsOrgUnitGroups || expr.collectInOrgUnitGroups().isNotEmpty()
+        fun processVarNames(expr: Expression) {
+            if (needsAllEvents && needsDataValues && needsAttributes) return
+            for (name in expr.collectProgramRuleVariableNames()) {
+                val v = byName[name]
+                val isMultiEvent = v is RuleVariableNewestEvent || v is RuleVariableNewestStageEvent || v is RuleVariablePreviousEvent
+                needsAllEvents = needsAllEvents || isMultiEvent
+                needsDataValues = needsDataValues || isMultiEvent || v is RuleVariableCurrentEvent
+                needsAttributes = needsAttributes || v is RuleVariableAttribute
+                needsEnrollment = needsEnrollment || needsAttributes
+                if (needsAllEvents && needsDataValues && needsAttributes) break
             }
         }
 
-        return RuleContextRequirements(needsAllEvents, needsEnrollment, needsDataValues, needsAttributes, needsOrgUnitGroups)
+        fun processOrgUnitGroups(expr: Expression) {
+            if (!needsOrgUnitGroups) needsOrgUnitGroups = expr.collectInOrgUnitGroups().isNotEmpty()
+        }
+
+        fun toRequirements() = RuleContextRequirements(
+            needsAllEvents, needsEnrollment, needsDataValues, needsAttributes, needsOrgUnitGroups,
+        )
     }
 }

src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineAnalyzerTest.kt

@@ -220,11 +220,6 @@ class RuleEngineAnalyzerTest {
         assertTrue(analyze(listOf(rule("V{enrollment_status} == 'ACTIVE'"))).needsEnrollment)
     }
 
-    @Test
-    fun incidentDateEnvVarTriggersNeedsEnrollment() {
-        assertTrue(analyze(listOf(rule("V{incident_date} > '2020-01-01'"))).needsEnrollment)
-    }
-
     @Test
     fun teiCountEnvVarTriggersNeedsEnrollment() {
         assertTrue(analyze(listOf(rule("V{tei_count} > 0"))).needsEnrollment)