Fix on_delete deprecation warnings

[sks444]

Related to #643, #652

Django 2 requires to have a on_delete parameter when using ForeignKey relations.

on_delete parameter defines what should happen to the related model when the respective field model gets deleted. E.g:

class ConferenceSetting(AuditModel):
    conference = models.ForeignKey(Conference, on_delete=models.CASCADE)

In the above model what should happen to ConferenceSetting object when Conference is deleted. In this case on_delete=models.CASCADE means that ConferenceSetting will also gets deleted.

But when we use on_delete=models.SET_NULL it sets the respective field to null instead of deleting the whole model object. E.g:

class UserAuditModel(models.Model):
    created_by = models.ForeignKey(
        User, related_name='created_%(class)s_set',
        null=True, blank=True, verbose_name="Created By",
        on_delete=models.SET_NULL
    )

---

So in my changes: as I am new to the codebase and don't have much familiarity with each model behaviour.

I have used on_delete=models.SET_NULL if the related field is nullable otherwise on_delete=models.CASCADE

Also added on_delete=django.db.models.deletion.CASCADE to old migration files as it is the default value provided by Django.

[coveralls]

Coverage remained the same at ?% when pulling 87df62f on sks444:on_delete_deprecated_warning into 607fa81 on pythonindia:master.

[ananyo2012]

@sks444 A whole bunch of tests are failing. Please investigate the reason and post your findings.

[sks444]

Hi @ananyo2012, tests are fixed now, a merge migration file was missing.

[ananyo2012]

@sks444 Are these migration files updated manually ? As per my understanding makemigrations creates new migrations instead of modifying older ones

[sks444]

As per my understanding makemigrations creates new migrations instead of modifying older ones

@ananyo2012, yes, I am also modifying the old migration files manually for the first time ^_^. But here is an explanation on why we needed these changes:

Django requires a default on_delete param when we use ForeignKey and OneToOne, so now as I have added on_delete to our models it is there for the new migrations files.

But the reason I had to update the old migration file is that those migrations files still have ForeignKey fields defined which doesn't have on_delete param which raises a deprecation warnings. Now as there is no other way to get rid of those warnings, so we will have to add the default value of on_delete to those migration files manually. Actually, Django also recommended to do this when they added this change.

Source

[ananyo2012]

Ah I see. We need to test this properly on the existing application and check that it doesn't break anything in production. Will check this tomorrow and update.

…

On Sun, 22 Mar, 2020, 10:40 PM Shrikrishna Singh, ***@***.***> wrote: As per my understanding makemigrations creates new migrations instead of modifying older ones @ananyo2012 <https://github.com/ananyo2012>, yes, I am also modifying the old migration files manually for the first time ^_^. But here is an explanation on why we needed these changes: Django requires a default on_delete param when we use ForeignKey and OneToOne, so now as I have added on_delete to our models it is there for the new migrations files. But the reason I had to update the old migration file is that those migrations files still have ForeignKey fields defined which doesn't have on_delete param which raises a deprecation warnings. Now as there is no other way to get rid of those warnings, so we will have to add the default value of on_delete to those migration files manually. Actually, Django also recommended to do this when they added this change. Source <https://stackoverflow.com/questions/51933474/is-it-correct-to-modify-old-migration-files-in-django> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQAER2KXYCE6MGOLLASQGDRIZBAPANCNFSM4LRJ6MYQ> .

[palnabarun]

@sks444 Can you provision a database locally, apply the old migrations, then apply the migrations again which are modified?

This would give an idea of whether they break anything.

[sks444]

@palnabarun Modification in the old migration files will not affect anything, as I have added a default value to on_delete param. Which was already applied to the database by Django even if the param was not there.

To verify this behaviour I ran python manage.py migrate after modifying the old migration files(i.e. adding default value of on_delete param to ForeignKey) and it didn't detected anything:

Operations to perform:
  Apply all migrations: feedback, contenttypes, account, proposals, sessions, conferences, devices, auth, admin, schedule, socialaccount, sites, tickets, profiles
Running migrations:
  No migrations to apply.

[sks444]

Only migration we need to apply to the database is for the new migration files which is created only when I set on_delete value to models.SET_NULL instead of models.CASCADE as I am changing the default value of on_delete.

[pradyunsg]

This seems fine to me. My only not-very-functional concern is that we have a merge migration, due to seemingly out-of-order updates to our migrations. I personally prefer to keep the migration history linear, so if that could be fixed, that'd be great!

Other than that, the code is basically not very readable or reviewable, which is just making a stronger case for #655 to me. :)

[sks444]

@pradyunsg I fixed the merge migrations problem. Generally I also don't prefer merge migrations, but I noticed it in the codebase at few places..anyway.

Other than that, the code is basically not very readable or reviewable, which is just making a stronger case for #655 to me. :)

Are you referring to the migration files changes? I think we can spare those as they are auto generated?

To make this pr reviewable I think I can create a separate pr for manual migration files changes?

[pradyunsg]

I was, yes.

It's fine for this PR. I was mostly just thinking aloud, so that I can come back it later. :)

[sks444]

Yeah, I am also feeling that while going through the codebase, it doesn't look a very newcomer friendly. Hard to read as well as there are so many things going on in a single method which I think can be split into multiple or maybe a class can be used and a bit doc string can also be helpful.

But anyway, let's work on these and see how far we can go. :)

[ananyo2012]

@sks444 Since you made the on_delete updates on the older migrations manually, so I was wondering if the new migrations are indeed needed. Are they still auto generated when you run makemigrations with the changes to old migration ?

[sks444]

Yes @ananyo2012 they are auto generated when I ran makemigrations with changes to old migration and still needed, as I am changing the default value of on_delete:

Only migration we need to apply to the database is for the new migration files which is created only when I set on_delete value to models.SET_NULL instead of models.CASCADE as I am changing the default value of on_delete.

[ananyo2012]

This looks good to go. I applied the migrations over the existing DB, and everything seems to be unchanged. Also tested the CASCADE functionality on conference and it works fine. Deletes the cascaded records of the conference.

Nice work @sks444 👍 Ship it !

[sks444]

Thanks @ananyo2012 :)

After this, I think I will move ahead with resolving warnings from the dependencies and move towards #611

Maybe while upgrading the dependencies we will have to upgrade Django to 1.11 as I think most of them won't be supporting Django 1.9.

[pradyunsg]

Gentle ping @sks444, could you merge master into this branch (discarding incoming changes) and run nox -s lint?

That should resolve nearly all the merge conflicts, and then a commit for fixing anything else that shows up, would be great!

[sks444]

@pradyunsg will nox -s lint not work if I don't have python3.6 on my system? I have 3.7 and 3.8.

RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.6'

[sks444]

@pradyunsg Can you have a look, I couldn't run nox -s lint on my machine. so I ran black to lint the code.

[pradyunsg]

@sk444 would you be OK with me pushing commits to this PR?

[sks444]

@sk444 would you be OK with me pushing commits to this PR?

Sure. :)

[pradyunsg]

Can you have a look, I couldn't run nox -s lint on my machine. so I ran black to lint the code.

Hey @sk444, could you file an issue describing why you couldn't run nox -s lint (including the error messages if you see any?)? If it's not working for a contributor, that's a bug in either our developer tools, or developer documentation. :)

[sks444]

Hi @pradyunsg, I have documented the problems I am facing while running nox locally at #660.

[pradyunsg]

@sks444 Would it be OK if I take this PR forward from here?

I feel like it'd be easier to take this to completion myself, while we figure out a good way to fix the development setup issues for you. Plus, this is basically ready other than linting issues, which I really don't want to bother you with. :)

[sks444]

Sure @pradyunsg that would be great, thank you for your help. :)

Although I have tried to fix most of the linting issues there is, but travis says I have missed one file:

reformatted /home/travis/build/pythonindia/junction/junction/conferences/models.py
All done! ✨ 🍰 ✨
1 file reformatted, 111 files left unchanged.
nox > Command pre-commit run --all-files failed with exit code 1
nox > Session lint-3.5 failed.
The command "nox" exited with 1.

Which I think is because I don't have Python 3.5 on my system.