-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove SubmissionValueVariable.form_variable FK field to avoid confusion #5071
Remove SubmissionValueVariable.form_variable FK field to avoid confusion #5071
Conversation
00ef125
to
09b3079
Compare
This sets up the factory to make sure the underlying FormVariable also exists for a given SubmissionValueVariable instance, but we *do* need to handle the case where this integrity is broken during runtime as records may be gone for whatever reason (and there are tests for this situation). Instead of crashing hard, we just log a message and fall back to just the raw underlying JSON value. The casting to python types is troublesome even when all information is available and the scope is too big to solve that here.
09b3079
to
c626d78
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5071 +/- ##
=======================================
Coverage 96.72% 96.72%
=======================================
Files 770 770
Lines 26517 26523 +6
Branches 3454 3454
=======================================
+ Hits 25649 25655 +6
Misses 606 606
Partials 262 262 ☔ View full report in Codecov by Sentry. |
@@ -359,10 +351,12 @@ class SubmissionValueVariable(models.Model): | |||
|
|||
objects = SubmissionValueVariableManager() | |||
|
|||
form_variable: FormVariable | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed, can't we remove the property as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're still setting this attribute dynamically and lots of code requires self.form_variable
to be defined in some capacity. This type annotation declares that it is a known attribute with a particular expected type. Not defining this here would definitely fail the type checking in CI :)
Closes #5058
Changes
form_variable
FK field as it's not needed and may otherwise cause confusion.There's still an interesting cascade delete now in the code where a form definition can delete the related form variable, leading to a submission variable existing without the matching form variable being present. I think our code is robust enough to deal with this, but it's something that could cause issues in the future if the service layer is not used to process submission values.
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.sh
Dockerfile/scripts
./bin
folderCommit hygiene