-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#2564] Mark catalogus as a required field on ZaakTypeconfig #1262
[#2564] Mark catalogus as a required field on ZaakTypeconfig #1262
Conversation
ec1b310
to
2212d05
Compare
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.
There are two places that rely on the condition that catalogus
is null
and should be changed:
@@ -66,3 +70,47 @@ def test_migration_0048_to_0051_multi_zgw_backend(self): | |||
expected, | |||
msg="Service config should have been moved to a new ZGWApiGroupConfig", | |||
) | |||
|
|||
|
|||
class TestMakeZaakTypeConfigCatalogusRequired(TestCase): |
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 have a utility class for migration tests that sets + resets the constraint checks (among other things). You don't have to use it, but take a look: https://github.com/maykinmedia/open-inwoner/blob/develop/src/open_inwoner/utils/tests/test_migrations.py
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.
Indeed. the difficulty here is that you need to test a failing migration, which the utility isn't set up for (I set out to address that in #1267).
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.
Ah, hadn't looked at that one, perfect.
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.
If you're generally happy with #1267 then I can rebase this PR on top of it and use the new-fangled utility to simplify this test.
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.
Done, usage example of the failing scenario at
open-inwoner/src/open_inwoner/openzaak/tests/test_migrations.py
Lines 78 to 96 in ef78f3c
class TestMakeZaakTypeConfigCatalogusRequired(TestFailingMigrations): | |
migrate_from = "0051_drop_root_zgw_fields" | |
migrate_to = "0052_zaaktypeconfig_catalogus_is_required" | |
app = "openzaak" | |
def setUpBeforeMigration(self, apps): | |
ZaakTypeConfig = apps.get_model("openzaak", "ZaakTypeConfig") | |
ZaakTypeConfig.objects.create(urls=[], catalogus=None, identificatie="foobar") | |
def test_migration_0051_to_0052_raises_with_descriptive_exception_message(self): | |
with self.assertRaises(DataError) as cm: | |
self.attempt_migration() | |
self.assertEqual( | |
str(cm.exception), | |
"Your database contains 1 ZaakTypeConfig row(s) with a missing `catalogus` field." | |
" This field is now required: please manually update all the affected rows or re-sync" | |
" your ZGW objects to ensure the field is included.", | |
) |
Excellent catch, thanks, will address these. |
b1fadf1
to
688ff8c
Compare
This field was only optional because eSuite did not (always) include a catalogus on its zaak types. That has since been fixed. This commit removes some legacy behavior which assumes that catalogus is nullable.
688ff8c
to
098ed8b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1262 +/- ##
===========================================
- Coverage 95.18% 95.17% -0.01%
===========================================
Files 980 981 +1
Lines 35687 35665 -22
===========================================
- Hits 33969 33945 -24
- Misses 1718 1720 +2 ☔ View full report in Codecov by Sentry. |
No description provided.