Replace form price logic with variable + normal logic #4856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergei-maertens
force-pushed
the
cleanup/4771-remove-price-logic
branch
from
a4eb8db to
702b8e3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4856 +/- ##
=======================================
Coverage 96.61% 96.61%
=======================================
Files 752 751 -1
Lines 25725 25699 -26
Branches 3406 3401 -5
=======================================
- Hits 24853 24828 -25
+ Misses 610 609 -1
Partials 262 262 ☔ View full report in Codecov by Sentry. |
sergei-maertens
force-pushed
the
cleanup/4771-remove-price-logic
branch
from
702b8e3 to
6cb7e41
Compare
sergei-maertens
force-pushed
the
cleanup/4771-remove-price-logic
branch
from
323e833 to
f5acb7a
Compare
sergei-maertens
changed the title
sergei-maertens
force-pushed
the
cleanup/4771-remove-price-logic
branch
from
f5acb7a to
36e255a
Compare
…logic Converted the current behaviour of price logic rules into assignments of the price logic variable value, including the fallback behaviour to the product price if no logic rule matches. This last bit is slightly nuanced, since changing the price on the product no longer results in the fallback case being automatically updated along, and that requires a note in the changelog (!). At first, I wanted to trigger the logic rules only when the last step was reached, using trigger_from_step, but I'm unsure that it behaves correctly if the last step is somehow made not applicable with other logic rules, so instead these rules are always evaluated. If it's safe, users can update the auto-migrated rules to optimize performance. I've also opted to not implement the reverse operation due to the involved complexity - we should definitely recommend to make database backups before upgrading to 3.0. We can be a bit bolder than usualy because it's part of a major version with other breaking changes anyway, and it should be fine (tm) for the majority of the cases.
Price logic rules are replaced with normal logic rules that assign the calculated price to a variable, and instead you should point to the form variable holding the calculated value.
We can't effectively remove this codepath yet because it's still used in the migration tests to ensure the calculated price before and after migrating has the same outcome, rather than testing the implementation details/state of the database. API endpoints and UI are removed, so there should not be a way to be able to get new price logic rules in the system. We'll also remove admin access and support for copying them when copying a form.
There are some slight behaviour changes here that need to be documented, namely the refusal to fall back to product price if price is derived from a variable, and the fact we can no longer make some sanity check assertions in the test method. However, the behaviour is covered by the main assertion of the test.
... and other things that should've had their documentation added already but I neglected to do so.
sergei-maertens
force-pushed
the
cleanup/4771-remove-price-logic
branch
from
36e255a to
bf46b3e
Compare
vaszig
approved these changes
Nov 28, 2024
| @@ -76,6 +77,12 @@ def get_submission_price(submission: Submission) -> Decimal: | |||
| data = submission.data | |||
| # test the rules one by one, if relevant | |||
| price_rules = form.formpricelogic_set.all() | |||
| if price_rules: | |||
| warnings.warn( | |||
| "Price logic rules are no longer supported. The left-over implementation " | |||
There was a problem hiding this comment.
We mean the model as well, right?
There was a problem hiding this comment.
Yes, that's implied! I can't delete the models yet because then this implementation breaks and that breaks the tests. I'll make sure to get rid of that once 3.0 is released.
Yeah I think a general remarks like that is needed either way. Something like "As always we recommend making and testing a database backup before upgrading, especially on major versions with breaking changes." Just something to keep in mind for next month! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4771
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shCommit hygiene