chore: fix column per row not been saved and added tests #291
Conversation
This file contains hidden or 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
Reviewer's GuideThis PR corrects the entangled_fields configuration in grid form definitions to ensure column-per-row settings are persisted and expands test coverage by adding and refining tests for DeviceChoiceField/OptionalDeviceChoiceField as well as comprehensive grid form behavior validations. Class diagram for updated grid form definitionsclassDiagram
class GridRowBaseForm {
+Meta
+_meta
}
class GridRowForm {
}
GridRowForm --|> GridRowBaseForm
GridRowBaseForm : entangled_fields["config"] += extra_fields_column.keys()
class GridColumnBaseForm {
+Meta
+_meta
}
class GridColumnForm {
}
GridColumnForm --|> GridColumnBaseForm
GridColumnBaseForm : entangled_fields["config"] += extra_fields_column.keys()
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mrbazzan - I've reviewed your changes - here's some feedback:
- The two test methods named test_grid_container_form collide—rename the second to something like test_grid_column_form so both actually run.
- Rather than mutating _meta.entangled_fields in scattered spots, consider centralizing that in your form definitions to avoid potential inconsistencies down the line.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two test methods named test_grid_container_form collide—rename the second to something like test_grid_column_form so both actually run.
- Rather than mutating _meta.entangled_fields in scattered spots, consider centralizing that in your form definitions to avoid potential inconsistencies down the line.
## Individual Comments
### Comment 1
<location> `tests/test_fields.py:37` </location>
<code_context>
],
)
+
+ def test_optional_device_choice_field(self):
+ class Form(forms.Form):
+ odc = OptionalDeviceChoiceField()
</code_context>
<issue_to_address>
Missing test for invalid input in OptionalDeviceChoiceField.
Add a test to verify that OptionalDeviceChoiceField rejects values not present in DEVICE_CHOICES.
</issue_to_address>
### Comment 2
<location> `tests/test_fields.py:61` </location>
<code_context>
+ self.assertTrue(form.is_valid(), form.errors)
+ self.assertEqual(form.cleaned_data["dc"], ["xs"])
+
+ form_1 = Form2(data={})
+ self.assertFalse(form_1.is_valid())
+ self.assertFormError(form_1, "dc_not_required",
+ ["Please select at least one device size"])
</code_context>
<issue_to_address>
Test for DeviceChoiceField with multiple valid and invalid values is missing.
Please add tests for DeviceChoiceField with multiple valid values and with a mix of valid and invalid values to better verify validation logic.
</issue_to_address>
### Comment 3
<location> `tests/grid/test_forms.py:10` </location>
<code_context>
+
+
+class GridFormTestCase(TestCase):
+ def test_grid_container_form(self):
+ form = GridContainerForm(data={
+ "container_type": "container",
</code_context>
<issue_to_address>
Duplicate test method name 'test_grid_container_form'.
Only the second method will run. Please rename one to reflect its purpose, such as 'test_grid_column_form'.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
+ Coverage 88.89% 89.01% +0.11%
==========================================
Files 124 124
Lines 3385 3385
Branches 288 288
==========================================
+ Hits 3009 3013 +4
+ Misses 259 254 -5
- Partials 117 118 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mrbazzan
force-pushed
the
fix/grid_row
branch
2 times, most recently
from
a85e288 to
3393f6b
Compare
fsbraun
approved these changes
Jul 20, 2025
|
@mrbazzan Let's get this released! 🚢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
Fix saving of per-row column settings by correcting entangled_fields metadata and form naming in grid forms, and add unit tests for device choice fields and grid container/row/column forms
Bug Fixes:
Tests: