From 458544a202e62d52e442c2fb4c4d1f669f3267a7 Mon Sep 17 00:00:00 2001 From: ruzaqiarkan-eng <289478799+ruzaqiarkan-eng@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:02:19 +0000 Subject: [PATCH 1/2] feat: relocate rule trigger condition to entry action config (Release N) - Implemented `get_entry_action` and `get_entry_condition` in `Rule` class. - Added dual-read compatibility layer with priority: Entry Action config > trigger_condition. - Updated `Rule.compile_conditions` and runtime components (`RuleCoordinator`, `SubRuleHandler`) to use encapsulated APIs. - Implemented conservative migration patch for relocating legacy conditions. - Updated Rule Builder frontend to support dual-writing Start Node conditions. - Added unit tests for migration and validated all existing rule tests. --- ...grate_trigger_condition_to_entry_action.py | 124 ++++++++++++++++++ .../rule_builder/stores/useRuleStore.js | 8 +- .../rule_builder/utils/condition_payload.js | 5 +- .../ruleflow/core/action_handlers/sub_rule.py | 5 +- flexirule/ruleflow/core/coordinator.py | 6 +- flexirule/ruleflow/doctype/rule/rule.py | 12 +- .../ruleflow/doctype/rule/test_migration.py | 107 +++++++++++++++ 7 files changed, 256 insertions(+), 11 deletions(-) create mode 100644 flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py create mode 100644 flexirule/ruleflow/doctype/rule/test_migration.py 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..58522cd0 --- /dev/null +++ b/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py @@ -0,0 +1,124 @@ +import frappe +import json + +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}: {str(e)}\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..2d29fa3a 100644 --- a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js +++ b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js @@ -289,7 +289,8 @@ 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 +379,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 +408,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..4208e635 100644 --- a/flexirule/ruleflow/core/action_handlers/sub_rule.py +++ b/flexirule/ruleflow/core/action_handlers/sub_rule.py @@ -236,12 +236,13 @@ 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_condition ) caller_rule_meta = { diff --git a/flexirule/ruleflow/core/coordinator.py b/flexirule/ruleflow/core/coordinator.py index 5f03223d..3819605f 100644 --- a/flexirule/ruleflow/core/coordinator.py +++ b/flexirule/ruleflow/core/coordinator.py @@ -243,7 +243,7 @@ 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 +617,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..f1d8f564 --- /dev/null +++ b/flexirule/ruleflow/doctype/rule/test_migration.py @@ -0,0 +1,107 @@ +import frappe +import json +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)) + + from flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action import execute + # 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 + 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) From 7e34e6e57f3da750eb59a4d2925b851314515469 Mon Sep 17 00:00:00 2001 From: ruzaqiarkan-eng <289478799+ruzaqiarkan-eng@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:07:23 +0000 Subject: [PATCH 2/2] chore: fix linting and formatting issues from previous commit --- ...grate_trigger_condition_to_entry_action.py | 36 ++++++----- .../rule_builder/stores/useRuleStore.js | 4 +- .../ruleflow/core/action_handlers/sub_rule.py | 4 +- flexirule/ruleflow/core/coordinator.py | 7 ++- .../ruleflow/doctype/rule/test_migration.py | 62 ++++++++++--------- 5 files changed, 62 insertions(+), 51 deletions(-) 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 index 58522cd0..20739f88 100644 --- a/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py +++ b/flexirule/patches/v1_0/migrate_trigger_condition_to_entry_action.py @@ -1,6 +1,8 @@ -import frappe import json +import frappe + + def execute(): """ Safe migration patch to relocate Rule.trigger_condition to Entry Action config. @@ -14,7 +16,8 @@ def execute(): # 1. Identify Entry Action entry_actions = [ - a for a in doc.actions + a + for a in doc.actions if (a.get("action_type") == "Entry Action" or a.get("action_id") == "root") ] @@ -26,7 +29,7 @@ def execute(): # Case C: Multiple entry actions frappe.log_error( f"Rule {doc.name} has multiple entry actions. Skipping migration.", - "FlexiRule Trigger Condition Migration" + "FlexiRule Trigger Condition Migration", ) continue else: @@ -43,14 +46,14 @@ def execute(): 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" + "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" + "FlexiRule Trigger Condition Migration", ) continue @@ -63,7 +66,9 @@ def execute(): # 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) + 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): @@ -73,17 +78,19 @@ def execute(): # 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" + "FlexiRule Trigger Condition Migration", ) continue except Exception as e: import traceback + frappe.log_error( - f"Migration failed for Rule {r.name}: {str(e)}\n\n{traceback.format_exc()}", - "FlexiRule Trigger Condition Migration" + 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 {} @@ -94,6 +101,7 @@ def _parse_json(value): except (ValueError, TypeError): return None + def _is_effectively_empty(cond): if not cond: return True @@ -104,21 +112,17 @@ def _is_effectively_empty(cond): 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 - ) + 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 2d29fa3a..cfef5cf5 100644 --- a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js +++ b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js @@ -290,7 +290,9 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { const startNode = graphStore.nodes.find((el) => el.type === "start"); // Dual-write: serialize Start Node config back to root trigger_condition - doc.trigger_condition = serializeField(startNode?.data?.trigger_condition || startNode?.data?.config); + doc.trigger_condition = serializeField( + startNode?.data?.trigger_condition || startNode?.data?.config + ); doc.compiled_expression = null; if (startNode?.data) { diff --git a/flexirule/ruleflow/core/action_handlers/sub_rule.py b/flexirule/ruleflow/core/action_handlers/sub_rule.py index 4208e635..46fcd21e 100644 --- a/flexirule/ruleflow/core/action_handlers/sub_rule.py +++ b/flexirule/ruleflow/core/action_handlers/sub_rule.py @@ -241,9 +241,7 @@ def execute(self, action, context, engine): if not sub_rule_doc.compiled_expression: from flexirule.ruleflow.core.compiler import ConditionCompiler - sub_rule_doc.compiled_expression = ConditionCompiler().compile( - sub_rule_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 3819605f..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")) or bool(frappe.db.get_value("Rule Action", {"parent": rule_name, "action_type": "Entry Action"}, "config")), + "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)), diff --git a/flexirule/ruleflow/doctype/rule/test_migration.py b/flexirule/ruleflow/doctype/rule/test_migration.py index f1d8f564..386055aa 100644 --- a/flexirule/ruleflow/doctype/rule/test_migration.py +++ b/flexirule/ruleflow/doctype/rule/test_migration.py @@ -1,7 +1,9 @@ -import frappe import json + +import frappe from frappe.tests.utils import FrappeTestCase + class TestTriggerConditionMigration(FrappeTestCase): def setUp(self): self.rule_name = "Migration Test Rule" @@ -11,33 +13,33 @@ def setUp(self): # Create a rule with legacy trigger_condition self.condition = { "op": "and", - "conditions": [ - {"left": {"ref": "doc.status"}, "op": "==", "right": {"value": "Open"}} - ] + "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 = 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() @@ -78,18 +80,18 @@ 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"}} - ] + "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)) - from flexirule.patches.v1_0.migrate_trigger_condition_to_entry_action import execute # 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