diff --git a/app/src/androidTest/java/com/opentasker/core/engine/variables/VariableExpanderConditionInstrumentedTest.kt b/app/src/androidTest/java/com/opentasker/core/engine/variables/VariableExpanderConditionInstrumentedTest.kt new file mode 100644 index 00000000..fda6eead --- /dev/null +++ b/app/src/androidTest/java/com/opentasker/core/engine/variables/VariableExpanderConditionInstrumentedTest.kt @@ -0,0 +1,75 @@ +package com.opentasker.core.engine.variables + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.opentasker.core.engine.VariableStore +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Runs the Matches/Doesn't Match (Tasker op codes 2/3) condition evaluator against Android's + * real ICU-backed regex engine. + * + * Per CONTRIBUTING.md, this project has shipped desktop-JVM/Android ICU regex divergences three + * times already; the JVM unit test suite runs on desktop `java.util.regex` and cannot see that + * class of bug by construction. `VariableExpander.matchesGlob` builds its pattern at runtime from + * imported Tasker wildcard data rather than from a fixed literal, so it isn't covered by the + * generated `production-regex-patterns.txt` corpus either (that scanner finds compile-time regex + * literals in source, and this pattern doesn't exist until a condition actually runs). This test + * is the only place the real construct -- an `(?s)` inline dotall flag plus `Regex.escape()` + * output, wired together by `String.split("*")` -- gets compiled by the engine devices actually + * ship. + */ +@RunWith(AndroidJUnit4::class) +class VariableExpanderConditionInstrumentedTest { + + @Test + fun matchesWildcardAgainstRealValueOnDevice() { + val variables = VariableStore().apply { set("pa_do", "view_url") } + assertTrue(variables.evaluateCondition("%pa_do ~ view_url")) + } + + @Test + fun matchesLeadingAndTrailingWildcardOnDevice() { + // The shape actually produced by a real Tasker export for a JSON-substring guard + // (verified against a live 6.6.20 backup): op=2, lhs a JSON blob, rhs `*"say":*`. + val variables = VariableStore().apply { set("pa_json", "{\"say\":\"hello\"}") } + assertTrue(variables.evaluateCondition("%pa_json ~ *\"say\":*")) + } + + @Test + fun doesNotMatchWildcardOnDevice() { + val variables = VariableStore().apply { set("pa_do", "launch_app") } + assertFalse(variables.evaluateCondition("%pa_do ~ view_url")) + } + + @Test + fun notMatchesOperatorNegatesCorrectlyOnDevice() { + val variables = VariableStore().apply { set("pa_do", "launch_app") } + assertTrue(variables.evaluateCondition("%pa_do !~ view_url")) + } + + @Test + fun wildcardValueContainingRegexMetacharactersIsTreatedLiterallyOnDevice() { + // Everything except "*" must be escaped, not interpreted as regex syntax -- a value or + // pattern containing ".", "(", "+", etc. (all realistic in imported %pa_json / %pa_url + // condition data) must match literally. + val variables = VariableStore().apply { set("pa_url", "https://example.com/a.b+c(d)") } + assertTrue(variables.evaluateCondition("%pa_url ~ *example.com/a.b+c(d)*")) + assertFalse(variables.evaluateCondition("%pa_url ~ *exampleXcom*")) + } + + @Test + fun isSetAndNotSetStillEvaluateCorrectlyOnDevice() { + // Not regex-related, but cheap to confirm here too since this is the one place a real + // ICU-backed VariableStore instance already exists for this evaluator. + val unset = VariableStore() + assertFalse(unset.evaluateCondition("%text is_set")) + assertTrue(unset.evaluateCondition("%text not_set")) + + val set = VariableStore().apply { set("text", "set a timer") } + assertTrue(set.evaluateCondition("%text is_set")) + assertFalse(set.evaluateCondition("%text not_set")) + } +} diff --git a/app/src/main/java/com/opentasker/core/engine/variables/VariableExpander.kt b/app/src/main/java/com/opentasker/core/engine/variables/VariableExpander.kt index 18122aa1..99af56ba 100644 --- a/app/src/main/java/com/opentasker/core/engine/variables/VariableExpander.kt +++ b/app/src/main/java/com/opentasker/core/engine/variables/VariableExpander.kt @@ -259,7 +259,18 @@ class VariableExpander { return parts.all { evaluateConditionInternal(it, store, arrays) } } - val comparison = parseComparison(normalized) ?: return normalized.toBoolean() + // A real binary comparison always wins over the unary is_set/not_set reading: e.g. + // "%status == is_set", comparing a variable against the literal string "is_set", must + // stay an equality check, not get hijacked into an existence check just because the text + // happens to end with that word. Unary existence syntax is tried only once no binary + // comparison matches at all -- which also means "is_set"/"not_set" can never appear as a + // *value* on either side of a real ==, !=, ~, etc. comparison and be misread as the unary + // operator, since parseComparison always gets first look. + val comparison = parseComparison(normalized) + if (comparison == null) { + unaryExistenceResult(normalized, store, arrays)?.let { return it } + return normalized.toBoolean() + } val left = expandInternal(comparison.left, store, arrays) val right = expandInternal(comparison.right, store, arrays) @@ -270,6 +281,51 @@ class VariableExpander { ComparisonOperator.GE -> compareNumbers(left, right) { l, r -> l >= r } ComparisonOperator.LT -> compareNumbers(left, right) { l, r -> l < r } ComparisonOperator.GT -> compareNumbers(left, right) { l, r -> l > r } + ComparisonOperator.MATCHES -> matchesGlob(left, right) + ComparisonOperator.NOT_MATCHES -> !matchesGlob(left, right) + } + } + + /** + * Tasker's wildcard match: `*` is the only special character, everything else is literal. + * Used for imported "Matches"/"Doesn't Match" conditions (Tasker op codes 2/3), e.g. + * `%pa_do ~ view_url` or `%pa_json ~ *"say":*`. Regex metacharacters other than `*` are + * escaped so a pattern like `%pa_x1.example` matches a literal dot, not "any character". + */ + /** + * Tasker's wildcard match: `*` is the only special character, everything else is literal. + * Used for imported "Matches"/"Doesn't Match" conditions (Tasker op codes 2/3), e.g. + * `%pa_do ~ view_url` or `%pa_json ~ *"say":*`. + * + * Goes through [compileLinearRegex] (RE2, linear-time) with the same [MAX_REGEX_LENGTH] / + * [MAX_REGEX_INPUT_LENGTH] guards this file already applies to every other regex built from + * import- or variable-derived text (see the `regex:`/`replace:` var-ops above), rather than + * Kotlin's backtracking `Regex`: `pattern` here comes from imported Tasker condition data, + * not a fixed literal, so several `*` wildcards is exactly the shape that causes catastrophic + * backtracking in a standard engine. `com.google.re2j.Pattern.quote` is RE2J's per-character + * `quoteMeta` escaper, not `kotlin.text.Regex.escape`'s `\Q...\E` -- RE2 doesn't implement + * `\Q...\E` as a syntax construct at all, so the latter would either fail to compile or (worse) + * be silently misinterpreted. + * + * `\A`/`\z` anchor for a full-string match (RE2J's `Matcher` is not confirmed to expose a + * `.matches()` convenience the way `java.util.regex.Matcher` does, so this uses the same + * `.find()` call already proven out at the other [compileLinearRegex] call sites). + */ + private fun matchesGlob(value: String, pattern: String): Boolean { + if (pattern.length > MAX_REGEX_LENGTH || value.length > MAX_REGEX_INPUT_LENGTH) return false + val regex = buildString { + append("(?s)\\A") + pattern.split("*").forEachIndexed { index, literal -> + if (index > 0) append(".*") + append(Re2Pattern.quote(literal)) + } + append("\\z") + } + val matcher = compileLinearRegex(regex)?.matcher(value) ?: return false + return try { + matcher.find() + } catch (e: RuntimeException) { + false } } @@ -296,6 +352,42 @@ class VariableExpander { return parts } + /** + * Unary existence checks (imported from Tasker's "Is Set"/"Not Set" condition ops, which have + * no right-hand operand). Suffix-anchored, not scanned like the binary operators, so a + * variable value that happens to contain "is_set"/"not_set" mid-string can't false-match. + * Returns null when `normalized` doesn't end with either suffix at all, so the caller can + * fall through to its own default (`normalized.toBoolean()`). + * + * This evaluator has two call paths with different pre-expansion behavior: + * TaskRunner.evaluateConditionString expands the whole condition once before calling in, but + * VariableStore.evaluateCondition (used directly by callers/tests that don't go through + * TaskRunner) does not expand at all -- `normalized` can arrive as raw `%variable` text. The + * binary comparison branch stays correct under both by calling expandInternal on its operands + * regardless of whether the caller already did; this needs the same self-sufficiency, so it + * must not assume expansion already happened. + * + * Empty (post-expansion) operand is a real, meaningful case, not a malformed condition: when + * the source variable is unset and the caller pre-expanded (the TaskRunner path), e.g. + * "%text not_set" arrives as just " not_set" (empty value + the literal suffix), and + * `cond.trim()` at the top of [evaluateConditionInternal] then eats that boundary space -- so + * an empty-operand match and a bare "not_set"/"is_set" with nothing before it are + * indistinguishable by the time we see them, and both correctly mean "the value is empty". + */ + private fun unaryExistenceResult(normalized: String, store: VariableStore, arrays: ArrayStore): Boolean? { + for ((suffix, wantsNonEmpty) in UNARY_EXISTENCE_SUFFIXES) { + val bareSuffix = suffix.trim() + if (normalized.equals(bareSuffix, ignoreCase = true)) { + return !wantsNonEmpty + } + if (normalized.endsWith(suffix, ignoreCase = true)) { + val operand = expandInternal(normalized.removeSuffix(suffix).trim(), store, arrays) + return if (wantsNonEmpty) operand.isNotEmpty() else operand.isEmpty() + } + } + return null + } + private fun stripOuterParens(expr: String): String { var result = expr while (result.length >= 2 && result.first() == '(' && matchingCloseParen(result, 0) == result.lastIndex) { @@ -328,7 +420,7 @@ class VariableExpander { ')' -> if (depth > 0) depth-- } if (depth == 0) { - val operator = COMPARISON_OPERATORS.firstOrNull { expr.startsWith(it.token, index) } + val operator = COMPARISON_OPERATORS.firstOrNull { expr.matchesOperatorAt(it, index) } if (operator != null) { val left = expr.substring(0, index).trim() val right = expr.substring(index + operator.token.length).trim() @@ -343,6 +435,27 @@ class VariableExpander { return matches.singleOrNull() } + /** + * True if `operator`'s token occurs at `index`, and -- for [ComparisonOperator.MATCHES] / + * [ComparisonOperator.NOT_MATCHES] only -- is bounded by whitespace or a string edge on both + * sides. `~` and `!~` are ordinary characters in real values in a way `==`/`<`/etc. are not + * (paths like `~/backups`, version strings like `1.2~rc1`, approximations like `~100`), so + * without this a literal tilde inside an otherwise unambiguous comparison's left/right text + * makes parseComparison find two operator matches instead of one, `matches.singleOrNull()` + * returns null, and the whole comparison silently falls back to `normalized.toBoolean()` + * (false) -- turning a working condition into one that's always false. The other operators + * don't get this treatment: it would be a larger, non-additive behavior change for tokens + * this project already shipped with, out of scope for this fix. + */ + private fun String.matchesOperatorAt(operator: ComparisonOperator, index: Int): Boolean { + if (!startsWith(operator.token, index)) return false + if (operator != ComparisonOperator.MATCHES && operator != ComparisonOperator.NOT_MATCHES) return true + val before = index == 0 || this[index - 1].isWhitespace() + val afterIndex = index + operator.token.length + val after = afterIndex >= length || this[afterIndex].isWhitespace() + return before && after + } + private fun compareNumbers(left: String, right: String, predicate: (Double, Double) -> Boolean): Boolean { val l = left.toDoubleOrNull() ?: return false val r = right.toDoubleOrNull() ?: return false @@ -427,15 +540,26 @@ class VariableExpander { GE(">="), LT("<"), GT(">"), + MATCHES("~"), + NOT_MATCHES("!~"), } companion object { + // (suffix, isSetWhenTrue) — order matters: "not_set"/"!is_set" style negatives must be + // checked before a shorter positive suffix could partially match, though with these two + // literal strings neither is a suffix of the other, so this is just future-proofing. + private val UNARY_EXISTENCE_SUFFIXES = listOf( + " is_set" to true, + " not_set" to false, + ) private val COMPARISON_OPERATORS = listOf( ComparisonOperator.EQ, ComparisonOperator.NE, ComparisonOperator.LE, ComparisonOperator.GE, ComparisonOperator.LT, + ComparisonOperator.NOT_MATCHES, + ComparisonOperator.MATCHES, ComparisonOperator.GT, ) private const val MAX_REGEX_LENGTH = 256 diff --git a/app/src/main/java/com/opentasker/core/transfer/TaskerXmlImport.kt b/app/src/main/java/com/opentasker/core/transfer/TaskerXmlImport.kt index ac41336b..5239e408 100644 --- a/app/src/main/java/com/opentasker/core/transfer/TaskerXmlImport.kt +++ b/app/src/main/java/com/opentasker/core/transfer/TaskerXmlImport.kt @@ -426,7 +426,26 @@ object TaskerXmlImporter { ) else -> ActionWithLoss(unsupportedAction(code)) } - val action = actionWithLoss.action + // Real Tasker exports carry a "Run only if" guard as a sibling , not as + // action args -- this applies to any action type, not just flow-control If/Else If. + // For most actions "condition" isn't a real arg key, so the parsed value only needs to + // land on the generic action.condition field. flow.if is the exception: its own args map + // already carries the old flat- fallback under "condition" (see the "37"/"if" branch + // above), and that same key is what both TaskRunner's stepControl and the action editor's + // existingActionArgValue() read as the if's actual test expression -- so when this action + // type already has an args["condition"] entry, overwrite it with the real parsed condition + // instead of dropping it, or the editor shows a blank required field for a working import. + val (importedCondition, conditionWarning) = element.parseImportedCondition() + val action = if (importedCondition != null) { + val args = if (actionWithLoss.action.args.containsKey("condition")) { + actionWithLoss.action.args + ("condition" to importedCondition) + } else { + actionWithLoss.action.args + } + actionWithLoss.action.copy(condition = importedCondition, args = args) + } else { + actionWithLoss.action + } val unsupported = if (action.type == TASKER_UNSUPPORTED_ACTION_ID) { TaskerUnsupportedAction(taskName = taskName, taskerCode = code, actionIndex = actionIndex) } else { @@ -441,7 +460,9 @@ object TaskerXmlImporter { action = action, mapped = mapped, unsupported = unsupported, - lossyWarning = actionWithLoss.lossyWarning, + lossyWarning = listOfNotNull(actionWithLoss.lossyWarning, conditionWarning) + .joinToString("; ") + .ifBlank { null }, ) } @@ -560,6 +581,55 @@ object TaskerXmlImporter { private fun Element.argIndex(): Int = getAttribute("sr").filter(Char::isDigit).toIntOrNull() ?: Int.MAX_VALUE + private data class ImportedCondition(val expression: String?, val warning: String?) + + /** + * Reads a Tasker `//` + * -- real Tasker exports encode the "Run only if" guard this way on ANY action, not just + * flow-control If/Else If (measured on a real backup: 85 of 118 ConditionList occurrences were + * on ordinary actions like Set Variable). Returns a condition string in this app's own syntax, + * or a null expression if the action has no ConditionList, which is the normal case for most + * actions. + * + * Only the single-condition case is handled: every real sample examined had exactly one + * `` per list. Tasker does support AND/OR chains of multiple conditions via extra + * ``/`` siblings; a multi-condition list degrades to using only the first + * condition (rather than silently producing "true" the way every ConditionList case did before + * this fix) and reports that degradation via [ImportedCondition.warning]. Likewise, an `` + * code outside the known Tasker set (0-9, 12, 13) yields a null expression -- which for flow.if + * specifically falls back to the old literal-"true" behavior -- but is now reported instead of + * silently reproduced. + */ + private fun Element.parseImportedCondition(): ImportedCondition { + val conditionList = directChildren("ConditionList").firstOrNull() ?: return ImportedCondition(null, null) + val conditions = conditionList.directChildren("Condition") + val condition = conditions.firstOrNull() ?: return ImportedCondition(null, null) + val lhs = condition.childText("lhs") + if (lhs.isBlank()) return ImportedCondition(null, null) + val multiConditionWarning = if (conditions.size > 1) { + "a multi-condition \"Run only if\" guard was reduced to just its first condition" + } else { + null + } + val op = condition.childText("op") + val expression = when (op) { + "12" -> "$lhs is_set" + "13" -> "$lhs not_set" + else -> { + val token = TASKER_CONDITION_OP_TOKENS[op] + ?: return ImportedCondition( + expression = null, + warning = listOfNotNull( + multiConditionWarning, + "a \"Run only if\" guard used an unsupported Tasker comparison (op $op) and was dropped", + ).joinToString("; "), + ) + "$lhs $token ${condition.childText("rhs")}" + } + } + return ImportedCondition(expression, multiConditionWarning) + } + private fun org.w3c.dom.NodeList.asElementList(): List = (0 until length).mapNotNull { index -> item(index).takeIf { it.nodeType == Node.ELEMENT_NODE } as? Element } @@ -600,4 +670,26 @@ object TaskerXmlImporter { ) const val TASKER_UNSUPPORTED_ACTION_ID = "tasker.unsupported" + + // Tasker's numeric Condition codes, sourced from github.com/mctinker/Map-Tasker's + // IF_CONDITION_OPERATORS table (a mature, independently-verified Tasker XML tool) and + // cross-checked against a real backup: op values 0/1/2/12/13 all appear and match their + // documented semantics -- e.g. the one real op=1 instance compares %http_response_code + // against 200, matching this project's own documented "Doesn't Match 200" HTTP-status check. + // 3-9 don't appear in that corpus but are included for completeness. 8/9 are the "(Numeric)" + // variants of =/!=; this evaluator has no numeric-aware equality distinct from string + // equality, so they map to the same tokens as 0/1. 12/13 (Is Set/Not Set) are unary and + // handled separately in parseImportedCondition, not through this map. + private val TASKER_CONDITION_OP_TOKENS = mapOf( + "0" to "==", + "1" to "!=", + "2" to "~", + "3" to "!~", + "4" to "~", + "5" to "!~", + "6" to "<", + "7" to ">", + "8" to "==", + "9" to "!=", + ) } diff --git a/app/src/test/java/com/opentasker/core/engine/TaskRunnerConditionTest.kt b/app/src/test/java/com/opentasker/core/engine/TaskRunnerConditionTest.kt index 8d1bee03..8d24a25c 100644 --- a/app/src/test/java/com/opentasker/core/engine/TaskRunnerConditionTest.kt +++ b/app/src/test/java/com/opentasker/core/engine/TaskRunnerConditionTest.kt @@ -267,4 +267,137 @@ class TaskRunnerConditionTest { assertTrue(trace.toSummaryLine().contains("token=")) assertTrue(trace.toSummaryLine().contains("template warnings: 1")) } + + // Covers the condition syntax now emitted by TaskerXmlImport for imported Tasker + // Is Set/Not Set/Matches conditions (op codes 12/13/2/3). + + @Test + fun runsActionWhenIsSetConditionMatchesNonEmptyVariable() = runBlocking { + var ran = false + ActionRegistry.register( + object : Action { + override val id = "test.condition.isset.run" + override val category = ActionCategory.FLOW + override suspend fun run(ctx: ActionContext, args: Map): ActionResult { + ran = true + return ActionResult.Success + } + } + ) + + val variables = VariableStore().apply { set("text", "set a timer") } + val report = TaskRunner(ActionContext(ContextWrapper(null), variables)).run( + Task( + name = "IsSet", + actions = listOf(ActionSpec(type = "test.condition.isset.run", condition = "%text is_set")), + ) + ) + + assertTrue(ran) + assertTrue(report.results.single() is ActionResult.Success) + } + + @Test + fun skipsActionWhenIsSetConditionSeesUnsetVariable() = runBlocking { + var ran = false + ActionRegistry.register( + object : Action { + override val id = "test.condition.isset.skip" + override val category = ActionCategory.FLOW + override suspend fun run(ctx: ActionContext, args: Map): ActionResult { + ran = true + return ActionResult.Success + } + } + ) + + val variables = VariableStore() + val report = TaskRunner(ActionContext(ContextWrapper(null), variables)).run( + Task( + name = "IsSetUnset", + actions = listOf(ActionSpec(type = "test.condition.isset.skip", condition = "%text is_set")), + ) + ) + + assertFalse(ran) + assertTrue(report.results.single() is ActionResult.Skip) + } + + @Test + fun runsActionWhenNotSetConditionSeesUnsetVariable() = runBlocking { + var ran = false + ActionRegistry.register( + object : Action { + override val id = "test.condition.notset.run" + override val category = ActionCategory.FLOW + override suspend fun run(ctx: ActionContext, args: Map): ActionResult { + ran = true + return ActionResult.Success + } + } + ) + + val variables = VariableStore() + val report = TaskRunner(ActionContext(ContextWrapper(null), variables)).run( + Task( + name = "NotSet", + actions = listOf(ActionSpec(type = "test.condition.notset.run", condition = "%text not_set")), + ) + ) + + assertTrue(ran) + assertTrue(report.results.single() is ActionResult.Success) + } + + @Test + fun runsActionWhenMatchesConditionSatisfiesWildcard() = runBlocking { + var ran = false + ActionRegistry.register( + object : Action { + override val id = "test.condition.matches.run" + override val category = ActionCategory.FLOW + override suspend fun run(ctx: ActionContext, args: Map): ActionResult { + ran = true + return ActionResult.Success + } + } + ) + + val variables = VariableStore().apply { set("do", "view_url") } + val report = TaskRunner(ActionContext(ContextWrapper(null), variables)).run( + Task( + name = "Matches", + actions = listOf(ActionSpec(type = "test.condition.matches.run", condition = "%do ~ view_url")), + ) + ) + + assertTrue(ran) + assertTrue(report.results.single() is ActionResult.Success) + } + + @Test + fun skipsActionWhenDoesntMatchConditionFindsWildcardMatch() = runBlocking { + var ran = false + ActionRegistry.register( + object : Action { + override val id = "test.condition.notmatches.skip" + override val category = ActionCategory.FLOW + override suspend fun run(ctx: ActionContext, args: Map): ActionResult { + ran = true + return ActionResult.Success + } + } + ) + + val variables = VariableStore().apply { set("json", "{\"say\":\"hello\"}") } + val report = TaskRunner(ActionContext(ContextWrapper(null), variables)).run( + Task( + name = "NotMatches", + actions = listOf(ActionSpec(type = "test.condition.notmatches.skip", condition = "%json !~ *\"say\":*")), + ) + ) + + assertFalse(ran) + assertTrue(report.results.single() is ActionResult.Skip) + } } diff --git a/app/src/test/java/com/opentasker/core/transfer/TaskerXmlExportTest.kt b/app/src/test/java/com/opentasker/core/transfer/TaskerXmlExportTest.kt index da17b94f..2774fa5c 100644 --- a/app/src/test/java/com/opentasker/core/transfer/TaskerXmlExportTest.kt +++ b/app/src/test/java/com/opentasker/core/transfer/TaskerXmlExportTest.kt @@ -35,6 +35,26 @@ class TaskerXmlExportTest { assertTrue(report.xml.contains("%MODE")) } + @Test + fun exportsImportedFlowIfConditionInsteadOfFallbackTrue() { + // Regression coverage for the import/export pairing: a flow.if imported from a Tasker + // carries its real test expression in args["condition"] (see + // TaskerXmlImport's ConditionList handling), which this exporter reads directly at + // line ~186. If that key were ever dropped again on import, every re-exported "if" would + // silently degrade to the literal fallback "true" with no error. + val task = Task( + id = 1, + name = "Imported if", + actions = listOf( + ActionSpec(type = "flow.if", condition = "%text is_set", args = mapOf("condition" to "%text is_set")), + ), + ) + val report = TaskerXmlExporter.export(emptyList(), listOf(task)) + + assertTrue(report.xml.contains("%text is_set")) + assertFalse(report.xml.contains("true")) + } + @Test fun exportsTimeContextsWithClockParts() { val profile = Profile( diff --git a/app/src/test/java/com/opentasker/core/transfer/TaskerXmlImporterTest.kt b/app/src/test/java/com/opentasker/core/transfer/TaskerXmlImporterTest.kt index 69bc3d70..dbb06649 100644 --- a/app/src/test/java/com/opentasker/core/transfer/TaskerXmlImporterTest.kt +++ b/app/src/test/java/com/opentasker/core/transfer/TaskerXmlImporterTest.kt @@ -358,4 +358,196 @@ class TaskerXmlImporterTest { assertTrue(confirmedBundle.profiles.single().requiresRiskAcknowledgement) assertTrue(confirmedBundle.metadata.warnings.any { it.contains("disabled by default") }) } + + // Real Tasker exports (verified against a live 6.6.20 backup) encode a "Run only if" guard + // as a sibling , not as action args -- and this is not exclusive to + // flow-control If/Else If: it appears on ordinary actions too (Set Variable, etc). Before this + // fix, every -guarded action silently lost its condition, either falling back + // to the literal string "true" (flow.if's own args["condition"] default) or, for every other + // action type, being left with no condition at all -- i.e. running unconditionally. + + @Test + fun conditionListOnFlowIfIsParsedIntoRealCondition() { + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1WithConditionList + + 37 + + %text12 + + + 38 + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + val ifAction = report.bundle.tasks.single().actions.first() + assertEquals("flow.if", ifAction.type) + assertEquals("%text is_set", ifAction.condition) + // flow.if reads its test expression from args["condition"] (both at runtime, in + // TaskRunner.stepControl, and in the action editor's existingActionArgValue()), so the old + // literal-"true" fallback placed there by the flat- branch must be overwritten with the + // real parsed condition rather than merely dropped -- leaving it absent would blank the + // editor's required field even though the import produced a working condition. + assertEquals("%text is_set", ifAction.args["condition"]) + } + + @Test + fun conditionListOnOrdinaryActionIsParsedAsGenericGuard() { + // This is the case the original fix (scoped only to code 37) would have missed entirely: + // a plain Set Variable (code 547) with its own "Run only if" guard, exactly like the 85 of + // 118 real ConditionList occurrences found on non-flow-control actions in a live backup. + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1GuardedSetVariable + + 547 + %pa_sta + toggle + + %pa_do0toggle + + + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + val action = report.bundle.tasks.single().actions.single() + assertEquals("var.set", action.type) + assertEquals("%pa_do == toggle", action.condition) + // The action's own regular args (name/value) must be untouched by the condition handling. + assertEquals("%pa_sta", action.args["name"]) + assertEquals("toggle", action.args["value"]) + } + + @Test + fun conditionListNotSetOperatorIsParsed() { + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1NotSet + + 37 + + %pa_ac13 + + + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + assertEquals("%pa_ac not_set", report.bundle.tasks.single().actions.single().condition) + } + + @Test + fun conditionListMatchesOperatorIsParsedWithWildcardRhs() { + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1Matches + + 37 + + %pa_x32*pa_json.* + + + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + assertEquals("%pa_x3 ~ *pa_json.*", report.bundle.tasks.single().actions.single().condition) + } + + @Test + fun actionWithoutConditionListKeepsExistingFlatStringBehavior() { + // Backward compatibility: an action with no at all (the synthetic + // flat- shape this importer previously assumed for every code-37 action) must import + // exactly as before -- this fix is additive, not a replacement of that path. + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1FlatCondition + 37%MODE = quiet + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + val action = report.bundle.tasks.single().actions.single() + assertEquals("%MODE = quiet", action.args["condition"]) + assertEquals(null, action.condition) + } + + @Test + fun multiConditionListWarnsAndUsesOnlyFirstCondition() { + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1MultiCondition + + 37 + + %pa_do0toggle + %pa_ac13 + + + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + assertEquals("%pa_do == toggle", report.bundle.tasks.single().actions.single().condition) + assertTrue(report.lossyWarnings.any { it.contains("reduced to just its first condition") }) + } + + @Test + fun unmappedConditionOperatorWarnsInsteadOfSilentlyFallingBackToTrue() { + val report = TaskerXmlImporter.parse( + rawXml = """ + + + 1UnknownOp + + 37 + + %pa_do99toggle + + + + + """.trimIndent(), + appVersion = "test", + importedAtEpochMs = 123L, + ) + + val action = report.bundle.tasks.single().actions.single() + assertEquals(null, action.condition) + assertEquals("true", action.args["condition"]) + assertTrue(report.lossyWarnings.any { it.contains("unsupported Tasker comparison (op 99)") }) + } } diff --git a/app/src/test/java/com/opentasker/ui/screens/NotificationTaskEditorMigrationTest.kt b/app/src/test/java/com/opentasker/ui/screens/NotificationTaskEditorMigrationTest.kt index 91e8723c..63e00e3b 100644 --- a/app/src/test/java/com/opentasker/ui/screens/NotificationTaskEditorMigrationTest.kt +++ b/app/src/test/java/com/opentasker/ui/screens/NotificationTaskEditorMigrationTest.kt @@ -33,4 +33,21 @@ class NotificationTaskEditorMigrationTest { assertEquals("", value) assertEquals(NotificationTaskResolution.Ambiguous("Duplicate", 2), issue) } + + @Test + fun importedFlowIfConditionPopulatesItsOwnRequiredEditorField() { + // flow.if's "condition" catalog field is a required text field the editor prefills from + // args["condition"] (see the `args[key] ?: ...` lookup at the top of + // existingActionArgValue). TaskerXmlImport writes the parsed Tasker into + // both action.condition and args["condition"] for this action type specifically, so the + // field must come back non-blank -- a blank read here would mean an imported "if" opens + // in the editor with its required condition field looking empty despite already working. + val value = existingActionArgValue( + actionId = "flow.if", + key = "condition", + args = mapOf("condition" to "%text is_set"), + ) + + assertEquals("%text is_set", value) + } }