Remove post migrate handler with a default instance creation via Migration

[VaZark]

This functionally works when running migrations and skips the default theme creation issue on app installation.

• Fix tests

[fabiocaccamo]

CI fails.

[VaZark]

even though not the most elegant method, I chose to go with raw sql.

Using apps.get_model("admin_interface", "Theme") throws errors due to inconsistency between the the table schema at the moment of migration (after 0001_initial) and the current shape of the model in the code.

I'm open to any alternate ideas for inserting data during migration without jumping through signals tho

[codecov]

Codecov Report

Base: 95.70% // Head: 94.49% // Decreases project coverage by -1.20% ⚠️

Coverage data is based on head (329dafa) compared to base (82f2fd7).
Patch has no changes to coverable lines.

❗ Current head 329dafa differs from pull request most recent head 2ff78ff. Consider uploading reports for the commit 2ff78ff to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   95.70%   94.49%   -1.21%     
==========================================
  Files          37       37              
  Lines         419      418       -1     
==========================================
- Hits          401      395       -6     
- Misses         18       23       +5     

Flag	Coverage Δ
unittests	94.49% <ø> (-1.21%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
admin_interface/apps.py	100.00% <ø> (ø)
admin_interface/migrations/0001_initial.py	100.00% <ø> (ø)
...nterface/migrations/0020_module_selected_colors.py	100.00% <ø> (ø)
admin_interface/models.py	100.00% <ø> (ø)
...min_interface/templatetags/admin_interface_tags.py	80.45% <0.00%> (-4.55%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[VaZark]

note : Tests are currently broken on sqlite for django < 2.0 .. looks some kind of error parsing multiline SQL statements (only for sqlite)?

@fabiocaccamo any idea when you might be dropping support for django < 2.2 ? (mentioned here)

[merwok]

I don’t think it’s correct to modify a migration that’s already deployed.

Why not add a new migration, and use apps.get_model without issue?

[VaZark]

The app_model can change again in the future. Then it will be inconsistent again :( .. so raw SQL is the cleanest way to keep it

I added the raw SQL to the first migration as that's the least risk for inconsistency. Adding it later risks accidental data-overwriting shenanigans

[merwok]

The app_model can change again in the future. Then it will be inconsistent again

No it will not? People run migrations up to the one that creates a default Theme, then new migrations will add fields with default value, so nothing breaks.

It just does not make sense to create a Theme version 0022 in the migration 0001.

[VaZark]

The result of the app_model returns the fields configured in code. There have been some field renaming in the past migrations.

So if there are any new renames or removals in the models, that breaks the result from app_model vis-à-vis the migration state. Insert stmt will be run with columns and values not in the table as well.

So just by adding the raw sql in the initial migration, i can ensure that new migration is consistent and old ones are never touched. We would not like to have new db entry added for something already in production by a third party library

To clarify, the sql script inserts for version 1. The app_model tries to insert for version 28.

[VaZark]

I’m open to any alternative ideas that account for rename and field removal. 👍

[merwok]

Editing deployed migrations is a django no-go. I am not going to migrate my servers to 0001 then up again and lose my data.

I am sorry but I do not understand your point about using apps.get_model. I use it quite often to populate data.
For example, let’s say in my app I have migrations 0001 to 0006 that bring the table to a specific schema, then the class retrieved with apps.get_model in migration 0007 conforms to that schema and can be used to create records, then if I add or change fields in migrations 0008 and 0009 they will be added or changed in the existing records. I never edit the code in 0007 to change what’s there, it’s always running with the schema of that step.

[VaZark]

Hmm.. maybe I’m mixing something up.

This is the error message i see when i use app_model in the initial migration.

2022-12-01T12:59:13.0909724Z     return self.cursor.execute(sql, params)
2022-12-01T12:59:13.0910132Z django.db.utils.ProgrammingError: column "css_header_title_color" of relation "admin_interface_theme" does not exist
2022-12-01T12:59:13.0910560Z LINE 1: ...", "logo_visible", "css_header_background_color", "css_heade...

You can checkout the commit results here

I’ll take a another look tmrw

[VaZark]

Added new commit with two migrations (one with app_model and second with field rename) that show why tests break if we go with the app_model approach.

The issue has never been the logic itself. I'm completely on board with that. It's more about the tests, they don't even start :(

[fabiocaccamo]

I think that here we are really overcomplicating the problem, I will drop the post_migrate handler in the next release and let's see the next open issue :)

Easier to ask for forgiveness than permission