diff --git a/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py b/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py new file mode 100644 index 00000000..20739f88 --- /dev/null +++ b/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py @@ -0,0 +1,128 @@ +import json + +import frappe + + +def execute(): + """ + Safe migration patch to relocate Rule.trigger_condition to Entry Action config. + Strictly idempotent and conservative. + """ + rules = frappe.get_all("Rule", fields=["name", "trigger_condition"]) + + for r in rules: + try: + doc = frappe.get_doc("Rule", r.name) + + # 1. Identify Entry Action + entry_actions = [ + a + for a in doc.actions + if (a.get("action_type") == "Entry Action" or a.get("action_id") == "root") + ] + + if not entry_actions: + # Case C: No entry action - should be handled by ensure_start_node but let's be safe + doc.ensure_start_node() + entry_action = doc.get_entry_action() + elif len(entry_actions) > 1: + # Case C: Multiple entry actions + frappe.log_error( + f"Rule {doc.name} has multiple entry actions. Skipping migration.", + "FlexiRule Trigger Condition Migration", + ) + continue + else: + entry_action = entry_actions[0] + + if not entry_action: + continue + + # 2. Parse conditions for comparison + legacy_cond = _parse_json(doc.trigger_condition) + action_cond = _parse_json(entry_action.config) + + # Case D: Malformed JSON check (if they were strings and failed to parse) + if doc.trigger_condition and legacy_cond is None: + frappe.log_error( + f"Rule {doc.name} has malformed legacy trigger_condition. Skipping migration.", + "FlexiRule Trigger Condition Migration", + ) + continue + + if entry_action.config and action_cond is None: + frappe.log_error( + f"Rule {doc.name} has malformed Entry Action config. Skipping migration.", + "FlexiRule Trigger Condition Migration", + ) + continue + + # 3. Decision Logic + if not legacy_cond or _is_effectively_empty(legacy_cond): + # Nothing to migrate + continue + + if not action_cond or _is_effectively_empty(action_cond): + # Case A: Clean Move + _update_action_config(entry_action, doc.trigger_condition) + doc.compile_conditions() + frappe.db.set_value( + "Rule", doc.name, "compiled_expression", doc.compiled_expression, update_modified=False + ) + else: + # Both have data - check for ambiguity + if _is_equal(legacy_cond, action_cond): + # Already migrated (Idempotent) + continue + else: + # Case B: Ambiguous Data + frappe.log_error( + f"Rule {doc.name} has differing conditions in Rule.trigger_condition and EntryAction.config. Skipping migration.", + "FlexiRule Trigger Condition Migration", + ) + continue + + except Exception as e: + import traceback + + frappe.log_error( + f"Migration failed for Rule {r.name}: {e!s}\n\n{traceback.format_exc()}", + "FlexiRule Trigger Condition Migration", + ) + + +def _parse_json(value): + if not value: + return {} + if isinstance(value, dict | list): + return value + try: + return json.loads(value) + except (ValueError, TypeError): + return None + + +def _is_effectively_empty(cond): + if not cond: + return True + if isinstance(cond, list) and not cond: + return True + if isinstance(cond, dict): + if not cond.get("conditions") and not cond.get("collection"): + return True + return False + + +def _is_equal(cond1, cond2): + # Simple stable comparison + return json.dumps(cond1, sort_keys=True) == json.dumps(cond2, sort_keys=True) + + +def _update_action_config(action_doc, config_value): + # Update via frappe.db.set_value to bypass hooks as requested + if isinstance(config_value, dict | list): + config_value = json.dumps(config_value) + + frappe.db.set_value("Rule Action", action_doc.name, "config", config_value, update_modified=False) + # Update in-memory for compile_conditions + action_doc.config = config_value diff --git a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js index 50856467..cfef5cf5 100644 --- a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js +++ b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js @@ -289,7 +289,10 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { doc.is_active = currentIsActive; const startNode = graphStore.nodes.find((el) => el.type === "start"); - doc.trigger_condition = serializeField(startNode?.data?.trigger_condition); + // Dual-write: serialize Start Node config back to root trigger_condition + doc.trigger_condition = serializeField( + startNode?.data?.trigger_condition || startNode?.data?.config + ); doc.compiled_expression = null; if (startNode?.data) { @@ -378,10 +381,11 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { operation: node.data?.operation, }) || {}; const conditionPayload = - action_type === "Condition" + action_type === "Condition" || action_type === "Entry Action" ? getConditionPayload({ config: node.data?.config, condition_json: node.data?.condition_json, + trigger_condition: node.data?.trigger_condition, }) : null; @@ -406,7 +410,7 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { } const finalConfig = - action_type === "Condition" + action_type === "Condition" || action_type === "Entry Action" ? conditionPayload || normalizedConfig : normalizedConfig; diff --git a/flexirule/public/js/flexirule/rule_builder/utils/condition_payload.js b/flexirule/public/js/flexirule/rule_builder/utils/condition_payload.js index 6222c931..c70605df 100644 --- a/flexirule/public/js/flexirule/rule_builder/utils/condition_payload.js +++ b/flexirule/public/js/flexirule/rule_builder/utils/condition_payload.js @@ -46,7 +46,10 @@ export function getConditionPayload(actionLike = {}) { if (fromConfig !== null) return fromConfig; const legacy = safeJsonParse(actionLike.condition_json, null); - return isConditionPayloadShape(legacy) ? legacy : null; + if (isConditionPayloadShape(legacy)) return legacy; + + const trigger = safeJsonParse(actionLike.trigger_condition, null); + return isConditionPayloadShape(trigger) ? trigger : null; } export function hasConditionPayload(actionLike = {}) { diff --git a/flexirule/ruleflow/core/action_handlers/sub_rule.py b/flexirule/ruleflow/core/action_handlers/sub_rule.py index f4cd66c1..46fcd21e 100644 --- a/flexirule/ruleflow/core/action_handlers/sub_rule.py +++ b/flexirule/ruleflow/core/action_handlers/sub_rule.py @@ -236,13 +236,12 @@ def execute(self, action, context, engine): ) # Evaluate trigger condition if not skipped - if not skip_conditions and sub_rule_doc.trigger_condition: + sub_rule_condition = sub_rule_doc.get_entry_condition() + if not skip_conditions and sub_rule_condition: if not sub_rule_doc.compiled_expression: from flexirule.ruleflow.core.compiler import ConditionCompiler - sub_rule_doc.compiled_expression = ConditionCompiler().compile( - sub_rule_doc.trigger_condition - ) + sub_rule_doc.compiled_expression = ConditionCompiler().compile(sub_rule_condition) caller_rule_meta = { "name": engine.rule.name, diff --git a/flexirule/ruleflow/core/coordinator.py b/flexirule/ruleflow/core/coordinator.py index 5f03223d..321afd3b 100644 --- a/flexirule/ruleflow/core/coordinator.py +++ b/flexirule/ruleflow/core/coordinator.py @@ -243,7 +243,12 @@ def _build_runtime_registry() -> dict: "execution_mode": row.get("execution_mode") or "Synchronous", "debug_mode": bool(row.get("debug_mode")), "compiled_expression": row.get("compiled_expression") or "", - "has_trigger_condition": bool(row.get("trigger_condition")), + "has_trigger_condition": bool(row.get("trigger_condition")) + or bool( + frappe.db.get_value( + "Rule Action", {"parent": rule_name, "action_type": "Entry Action"}, "config" + ) + ), # Only use explicitly configured watched fields for dispatch-time pruning. # Trigger-condition dependencies are evaluated in eligibility and are not safe skip keys. "watched_fields": sorted(set(explicit_watched)), @@ -617,11 +622,11 @@ def _has_field(doctype, fieldname): return False, _("Trigger Evaluation Error: {0}").format(str(e)) # Conditions MUST be pre-compiled - no runtime JSON parsing - elif rule_doc.get("trigger_condition"): + elif rule_doc.get_entry_condition(): try: import json - parsed = json.loads(rule_doc.get("trigger_condition")) + parsed = rule_doc.get_entry_condition() if isinstance(parsed, dict) and not parsed.get("conditions"): return True, _("Eligible") if isinstance(parsed, list) and not parsed: diff --git a/flexirule/ruleflow/doctype/rule/rule.py b/flexirule/ruleflow/doctype/rule/rule.py index 83c5a22d..da6e7b86 100644 --- a/flexirule/ruleflow/doctype/rule/rule.py +++ b/flexirule/ruleflow/doctype/rule/rule.py @@ -514,9 +514,17 @@ def resolve_entry_condition(self): if isinstance(self.trigger_condition, str) else self.trigger_condition ) - return legacy_config + # Check if legacy config is non-empty + if legacy_config and ( + isinstance(legacy_config, list) + or ( + isinstance(legacy_config, dict) + and (legacy_config.get("conditions") or legacy_config.get("collection")) + ) + ): + return legacy_config except (ValueError, TypeError): - return None + pass return config diff --git a/flexirule/ruleflow/doctype/rule/test_migration.py b/flexirule/ruleflow/doctype/rule/test_migration.py new file mode 100644 index 00000000..386055aa --- /dev/null +++ b/flexirule/ruleflow/doctype/rule/test_migration.py @@ -0,0 +1,109 @@ +import json + +import frappe +from frappe.tests.utils import FrappeTestCase + + +class TestTriggerConditionMigration(FrappeTestCase): + def setUp(self): + self.rule_name = "Migration Test Rule" + if frappe.db.exists("Rule", self.rule_name): + frappe.delete_doc("Rule", self.rule_name) + + # Create a rule with legacy trigger_condition + self.condition = { + "op": "and", + "conditions": [{"left": {"ref": "doc.status"}, "op": "==", "right": {"value": "Open"}}], + } + + self.rule = frappe.get_doc( + { + "doctype": "Rule", + "rule_name": self.rule_name, + "document_type": "User", + "trigger_type": "DocType Event", + "trigger_event": "Before Save", + "trigger_condition": json.dumps(self.condition), + "actions": [ + { + "action_id": "root", + "action_type": "Entry Action", + "action_label": "Start", + "next_step_if_true": "stop_node", + }, + { + "action_id": "stop_node", + "action_type": "Stop", + "action_label": "Stop", + "operation": "Success", + }, + ], + } + ) + self.rule.insert(ignore_permissions=True) + # Clear config of entry action to simulate Case A + entry_action = self.rule.get_entry_action() + frappe.db.set_value("Rule Action", entry_action.name, "config", None) + + def test_migration_case_a_clean_move(self): + from flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action import execute + + execute() + + # Reload + self.rule.reload() + entry_action = self.rule.get_entry_action() + + # Verify migrated + self.assertEqual(json.loads(entry_action.config), self.condition) + # Verify legacy field is NOT cleared + self.assertEqual(json.loads(self.rule.trigger_condition), self.condition) + # Verify compiled_expression is updated + self.assertTrue(self.rule.compiled_expression) + + def test_migration_idempotency(self): + from flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action import execute + + # Run once + execute() + self.rule.reload() + first_config = self.rule.get_entry_action().config + + # Run again + execute() + self.rule.reload() + second_config = self.rule.get_entry_action().config + + self.assertEqual(first_config, second_config) + + def test_migration_case_b_ambiguity(self): + # Set different config in Entry Action + other_condition = { + "op": "and", + "conditions": [{"left": {"ref": "doc.status"}, "op": "==", "right": {"value": "Closed"}}], + } + entry_action = self.rule.get_entry_action() + frappe.db.set_value("Rule Action", entry_action.name, "config", json.dumps(other_condition)) + + # Mock log_error to verify it was called since Error Log is proving tricky in test transaction + import flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action as patch_mod + from flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action import execute + + original_log = patch_mod.frappe.log_error + log_called = [False] + + def mock_log(msg, title): + log_called[0] = True + + patch_mod.frappe.log_error = mock_log + try: + execute() + finally: + patch_mod.frappe.log_error = original_log + + self.assertTrue(log_called[0]) + + # Verify NOT migrated + self.rule.reload() + entry_action = self.rule.get_entry_action() + self.assertEqual(json.loads(entry_action.config), other_condition)