Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/expression-parser.api
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public final class org/hisp/dhis/lib/expression/ast/NamedFunction : java/lang/En
public static final field d2_hasUserRole Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_hasValue Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_inOrgUnitGroup Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_inUserGroup Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_lastEventDate Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_left Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
public static final field d2_length Lorg/hisp/dhis/lib/expression/ast/NamedFunction;
Expand Down Expand Up @@ -1383,6 +1384,7 @@ public abstract interface class org/hisp/dhis/lib/expression/spi/ExpressionFunct
public abstract fun d2_hasUserRole (Ljava/lang/String;Ljava/util/List;)Z
public abstract fun d2_hasValue (Lorg/hisp/dhis/lib/expression/spi/VariableValue;)Z
public abstract fun d2_inOrgUnitGroup (Ljava/lang/String;Lorg/hisp/dhis/lib/expression/spi/VariableValue;Ljava/util/Map;)Z
public abstract fun d2_inUserGroup (Ljava/lang/String;Ljava/util/List;)Z
public abstract fun d2_lastEventDate (Lorg/hisp/dhis/lib/expression/spi/VariableValue;)Lkotlinx/datetime/LocalDate;
public abstract fun d2_left (Ljava/lang/String;Ljava/lang/Integer;)Ljava/lang/String;
public abstract fun d2_length (Ljava/lang/String;)I
Expand Down Expand Up @@ -1444,6 +1446,7 @@ public final class org/hisp/dhis/lib/expression/spi/ExpressionFunctions$DefaultI
public static fun d2_hasUserRole (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Ljava/lang/String;Ljava/util/List;)Z
public static fun d2_hasValue (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Lorg/hisp/dhis/lib/expression/spi/VariableValue;)Z
public static fun d2_inOrgUnitGroup (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Ljava/lang/String;Lorg/hisp/dhis/lib/expression/spi/VariableValue;Ljava/util/Map;)Z
public static fun d2_inUserGroup (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Ljava/lang/String;Ljava/util/List;)Z
public static fun d2_lastEventDate (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Lorg/hisp/dhis/lib/expression/spi/VariableValue;)Lkotlinx/datetime/LocalDate;
public static fun d2_left (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Ljava/lang/String;Ljava/lang/Integer;)Ljava/lang/String;
public static fun d2_length (Lorg/hisp/dhis/lib/expression/spi/ExpressionFunctions;Ljava/lang/String;)I
Expand Down
2 changes: 1 addition & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ repositories {
mavenCentral()
}

version = "1.1.11-SNAPSHOT"
version = "1.1.12-SNAPSHOT"
group = "org.hisp.dhis.lib.expression"

if (project.hasProperty("removeSnapshotSuffix")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ enum class NamedFunction(
d2_extractDataMatrixValue("d2:extractDataMatrixValue", ValueType.STRING, ValueType.STRING, ValueType.STRING),
d2_floor("d2:floor", ValueType.NUMBER, ValueType.NUMBER),
d2_hasUserRole("d2:hasUserRole", ValueType.BOOLEAN, ValueType.STRING),
d2_inUserGroup("d2:inUserGroup", ValueType.BOOLEAN, ValueType.STRING),
d2_hasValue("d2:hasValue", ValueType.BOOLEAN, ValueType.MIXED),
d2_inOrgUnitGroup("d2:inOrgUnitGroup", ValueType.BOOLEAN, ValueType.STRING),
d2_lastEventDate("d2:lastEventDate", ValueType.DATE, ValueType.STRING),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ internal class Calculator(
NamedFunction.d2_floor -> functions.d2_floor(evalToNumber(fn.child(0)))
NamedFunction.d2_hasUserRole -> functions.d2_hasUserRole(
evalToString(fn.child(0)),
data.supplementaryValues["USER"])
data.supplementaryValues["USER_ROLES"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will introduce a breaking change in the integrations. In the case of Android, this supplementaryValues property is passed by the Android app and it has the key USER (check this line in the Android app). I guess it would happen the same for the backend, @enricocolasante?

This change must be warned in the release notes. Or change, at least, the minor digit of the version instead of the patch one.

@jbee @zubaira

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clients will have to update to support USER_GROUPS this could also be the right opportunity to introduce the breaking change of renaming USER to USER_ROLES for better consistency. But if we all agree otherwise, then we can revert this change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should add some warn somewhere and change the minor digit of the version.
I would also take the opportunity to make the supplementaryValues more explicit and structured and not rely on strings, so the breaking change would fail at compilation and it would be easier for the client fix it.
With the current change, rule-engine will continue to work but some strange behavior will happen around users and it is not really easy for the client to discover and understanding what is happening

NamedFunction.d2_inUserGroup -> functions.d2_inUserGroup(
evalToString(fn.child(0)),
data.supplementaryValues["USER_GROUPS"])
NamedFunction.d2_hasValue -> try {
functions.d2_hasValue(evalToVar(fn.child(0)))
} catch (e: IllegalExpressionException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,11 @@ fun interface ExpressionFunctions {
return roles.contains(role)
}

fun d2_inUserGroup(userGroup: String?, userGroups: List<String?>?): Boolean {
if (userGroups == null) throw IllegalExpressionException("Supplementary data for user needs to be provided")
return userGroups.contains(userGroup)
}

fun d2_hasValue(value: VariableValue?): Boolean {
return value?.value != null
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ object ExpressionGrammar {
fn(NamedFunction.d2_extractDataMatrixValue, expr, expr),
fn(NamedFunction.d2_floor, expr),
fn(NamedFunction.d2_hasUserRole, expr),
fn(NamedFunction.d2_inUserGroup, expr),
fn(NamedFunction.d2_inOrgUnitGroup, expr),
fn(NamedFunction.d2_lastEventDate, expr),
fn(NamedFunction.d2_left, expr, expr),
Expand Down
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to the rest of the PR? Seems the change from "USER_ROLES" to "USER" is another thing?

Copy link
Contributor Author

@zubaira zubaira Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems more aligned with the current update. The USER keyword was already being used for user roles, and now that we’re introducing USER_GROUPS, it makes sense to rename USER to USER_ROLES for better consistency and clarity.

Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal class HasUserRoleTest {

@Test
fun testHasUserRole_Null() {
assertFalse(evaluate("d2:hasUserRole(null)", mapOf("USER" to listOf("admin"))))
assertFalse(evaluate("d2:hasUserRole(null)", mapOf("USER_ROLES" to listOf("admin"))))
}

@Test
Expand All @@ -26,9 +26,9 @@ internal class HasUserRoleTest {

@Test
fun testHasUserRole() {
assertTrue(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER" to listOf("admin"))))
assertFalse(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER" to listOf("guest"))))
assertTrue(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER" to listOf("foo","admin"))))
assertTrue(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER_ROLES" to listOf("admin"))))
assertFalse(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER_ROLES" to listOf("guest"))))
assertTrue(evaluate("d2:hasUserRole(\"admin\")", mapOf("USER_ROLES" to listOf("foo","admin"))))
}

private fun evaluate(expression: String, supplementaryValues: Map<String, List<String>>): Boolean {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.hisp.dhis.lib.expression.function

import org.hisp.dhis.lib.expression.Expression
import org.hisp.dhis.lib.expression.ExpressionMode
import org.hisp.dhis.lib.expression.spi.ExpressionData
import org.hisp.dhis.lib.expression.spi.IllegalExpressionException
import kotlin.test.*

/**
* Test of the `d2:inUserGroup` function.
*
* @author Zubair Asghar
*/
internal class InUserGroupTest {

@Test
fun testInUserGroup_Null() {
assertFalse(evaluate("d2:inUserGroup(null)", mapOf("USER_GROUPS" to listOf("uidusgroup0"))))
}

@Test
fun testInUserGroup_NoData() {
val ex = assertFailsWith(IllegalExpressionException::class) { evaluate("d2:inUserGroup(\"uidougroup1\")", mapOf()) }
assertEquals("Supplementary data for user needs to be provided", ex.message)
}

@Test
fun testInUserGroup() {
assertTrue(evaluate("d2:inUserGroup(\"uidusgroup0\")", mapOf("USER_GROUPS" to listOf("uidusgroup0"))))
assertFalse(evaluate("d2:inUserGroup(\"uidusgroup0\")", mapOf("USER_GROUPS" to listOf("uidusgroup1"))))
}

private fun evaluate(expression: String, supplementaryValues: Map<String, List<String>>): Boolean {
val data: ExpressionData = ExpressionData().copy(supplementaryValues = supplementaryValues)
return Expression(expression, ExpressionMode.RULE_ENGINE_ACTION).evaluate( { _: String -> null }, data) as Boolean
}
}