Skip to content

Ensure new logic evaluation is the only logic evaluation#6261

Merged
sergei-maertens merged 10 commits into
mainfrom
chore/remove-new-logic-evaluation-feature-flag
May 26, 2026
Merged

Ensure new logic evaluation is the only logic evaluation#6261
sergei-maertens merged 10 commits into
mainfrom
chore/remove-new-logic-evaluation-feature-flag

Conversation

@viktorvanwijk
Copy link
Copy Markdown
Contributor

@viktorvanwijk viktorvanwijk commented May 7, 2026

Partly closes #6164

Changes

  • Removed usages of new_logic_evaluation_enabled feature flag
  • Removed usages of the trigger_from_step setting for logic rules

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Checked new model fields are usable in the admin
    • Problem detection in the admin email digest is handled
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how
  • Documentation

    • Added documentation which describes the changes

@viktorvanwijk viktorvanwijk changed the title Remove new logic evaluation feature flag Ensure new logic evaluation is the only logic evaluation May 7, 2026
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 37ee520 to 03a18e3 Compare May 7, 2026 14:03
@viktorvanwijk viktorvanwijk marked this pull request as ready for review May 7, 2026 14:10
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.12%. Comparing base (d1cf2bc) to head (22d2f45).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6261   +/-   ##
=======================================
  Coverage   97.12%   97.12%           
=======================================
  Files         870      870           
  Lines       33307    33263   -44     
  Branches     3076     3064   -12     
=======================================
- Hits        32349    32308   -41     
+ Misses        646      645    -1     
+ Partials      312      310    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 03a18e3 to b9edab2 Compare May 7, 2026 14:51
@viktorvanwijk viktorvanwijk marked this pull request as draft May 7, 2026 14:57
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from b9edab2 to cf163c0 Compare May 7, 2026 14:58
@viktorvanwijk viktorvanwijk marked this pull request as ready for review May 7, 2026 15:34
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from cf163c0 to f271b10 Compare May 8, 2026 15:02
@viktorvanwijk viktorvanwijk marked this pull request as draft May 8, 2026 15:02
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch 2 times, most recently from ca11010 to b9b5e74 Compare May 11, 2026 10:21
@viktorvanwijk viktorvanwijk marked this pull request as ready for review May 11, 2026 10:23
@viktorvanwijk viktorvanwijk requested a review from vaszig May 11, 2026 10:24
Comment thread docs/installation/upgrade-400.rst
Comment thread src/openforms/forms/migrations/0131_remove_formlogic_trigger_from_step.py Outdated
Comment thread src/openforms/js/components/admin/form_design/FormLogic.stories.js
Comment thread bin/check_disable_next_logic_action.py
Copy link
Copy Markdown
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Basically removing stuff related to the flag so not to much to say. Only a couple of questions (as I haven't dived into the new logic evaluation).

  • Also saw some data migrations tests related to trigger_from_step, I believe they can be removed in the next version
  • Regarding the UI tests I see some differences in the languages, can you take a look?

@viktorvanwijk
Copy link
Copy Markdown
Contributor Author

Also saw some data migrations tests related to `trigger_from_step`, I believe they can be removed in the next version

You mean the ChangeDisableLogicActionMigrationTests? I thought as long as the migration isn't squashed, it's OK to leave it

* Regarding the UI tests I see  some differences in the languages, can you take a look?

Will do!

@vaszig
Copy link
Copy Markdown
Contributor

vaszig commented May 12, 2026

Also saw some data migrations tests related to `trigger_from_step`, I believe they can be removed in the next version

You mean the ChangeDisableLogicActionMigrationTests? I thought as long as the migration isn't squashed, it's OK to leave it

Yes, I agree!

@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from b9b5e74 to 3020ec3 Compare May 15, 2026 07:59
@viktorvanwijk
Copy link
Copy Markdown
Contributor Author

Regarding the UI tests I see some differences in the languages, can you take a look?

Discussed on Slack: seems like it compares to an outdated chromatic snapshot? I have triggered a rebuild, but that didn't solve it unfortunately. The messages as they are in the new snapshots are the correct ones, see https://github.com/open-formulieren/open-forms/blob/main/src/openforms/js/lang/nl.json#L3493 (they are not translated yet)

@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 3020ec3 to d36af49 Compare May 15, 2026 12:28
@viktorvanwijk viktorvanwijk marked this pull request as draft May 15, 2026 14:43
Comment thread src/openforms/conf/base.py Outdated
Comment thread Dockerfile
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from d36af49 to 598c4b9 Compare May 18, 2026 10:13
@viktorvanwijk viktorvanwijk marked this pull request as ready for review May 18, 2026 10:31
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch 2 times, most recently from 16f103e to 835c6b5 Compare May 18, 2026 14:53
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 835c6b5 to 849b82e Compare May 19, 2026 06:48
@viktorvanwijk viktorvanwijk force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 849b82e to 01b8b33 Compare May 26, 2026 07:20
This will be the default in 4.0 :)

Note that we want to execute `report_invalid_form_logic.py` as an upgrade check, which means it has to work in both 3.5 and 4.0,
so we cannot remove the model field completely yet.
test_change_component_to_hidden_with_new_logic_evaluation:
The new logic evaluation is now the only logic evaluation, so this test basically becomes a duplicate of `test_change_component_is_hidden` in this same class.

test_modify_variable_related_to_another_step_than_the_one_being_edited:
As the comment already indicated, this test does not make sense in this context. We would need to add a PUT call to the submission step detail endpoint to
persist the user-defined variable. However, this would basically mean testing the fact that logic is executed before persisting the variables, which is already
tested in `FormStepSubmissionTests.test_user_defined_variables_are_persisted`. Note that `test_evaluate_rules_when_trigger_step_reached` (will be renamed to
`test_evaluate_rules_on_all_available_steps` in a next commit) already tests the behavior when a different step is being evaluated than the relevant step of
the logic rule. Therefore, I have opted to remove it instead.
Kinda goes hand in hand with making the new logic evaluation the default one, as a trigger-from-step is not compatible with it.
Note that we want to execute `report_invalid_form_logic.py` as an upgrade check, which means it has to work in both 3.5 and 4.0,
so we cannot remove the model field completely yet.

I have removed tests that tested trigger-from-step behavior, and modified them where applicable.

Note on test_check_logic_on_whole_submission_with_variables:
Tricky one, because it combined trigger-from-step behavior with `check_submission_logic`. We can force a certain step by using
a component instead of a user-defined variable. Logic analysis will then assign it to the output step. I have made it pass for now,
but I think this test also heavily depends on (the order of) the rules executed during `check_submission_logic` - see #6151.
This script must be run before upgrading to 3.5, and I think 3.5 will be a prerequisite before upgrading to 4.0, so it is no longer relevant
This includes displaying the advanced options as well, because it was the only one :)
@sergei-maertens sergei-maertens force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 01b8b33 to 0191892 Compare May 26, 2026 08:18
We also want to include the invalid form logic script as an additional constraint.

I have revised the script to block the upgrade if there are forms (with logic rules) that have not been converted
to the new logic evaluation. Note that these will also include forms with cycles, as a conversion is illegal if
cycles are present

4.0.0 has not been released officially, but we are tricking it to run the upgrade check already :)
@sergei-maertens sergei-maertens force-pushed the chore/remove-new-logic-evaluation-feature-flag branch from 0191892 to 22d2f45 Compare May 26, 2026 08:21
@sergei-maertens sergei-maertens merged commit 41ad97d into main May 26, 2026
32 checks passed
@sergei-maertens sergei-maertens deleted the chore/remove-new-logic-evaluation-feature-flag branch May 26, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

💥 4.0 - overview of deprecated functionalities removal

3 participants