Skip to content
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

Fix all the deprecated-warnings and upgrade django to 1.11 #664

Open
wants to merge 15 commits into
base: django-2.2
Choose a base branch
from

Conversation

sks444
Copy link
Contributor

@sks444 sks444 commented Apr 5, 2020

Toward #611

Note: This PR has changes incompatible with Python 2.7 and is a part of the task involving Django 2.2 upgrade

  • Upgrading django-pagedown to 1.0.6 because the latest version doesn't support Django 1.11, will be upgrading to latest while moving to Django 2.0

  • Fix AttributeError: type object 'RadioSelect' has no attribute 'renderer': 1

  • Use django-markdown-app as django_markdown is not maintained.

  • Upgrade djangorestframework to latest version which removes the support of filters, so update django-filters to latest and made changes according to the requirements: 1

  • Upgrade celery to latest version

  • Droping use of django-uuidfield as Django has it's own UUIDfield: need to figure out a way to handle the migration

  • Upgrade django-bootstrap3==11.0.0, needs to upgraded to latest while moving to Django 2

  • core.context_processors is moved to template.context_processors

  • Use user.is_authenticated as property

After these changes, only deprecated warnings remaining is related to Django which I think will vanish when we finally move to Django 2.0

@coveralls
Copy link

coveralls commented Apr 5, 2020

Coverage Status

Coverage increased (+0.03%) to 67.292% when pulling e70b6b2 on sks444:fix-all-deprecated-warnings into 49af450 on pythonindia:master.

@pradyunsg
Copy link
Contributor

pradyunsg commented Apr 5, 2020

@sks444 If you could make atomic commits, that'd make reviewing your PR (as well as iterating on them later) much easier! ^>^

@@ -14,7 +13,7 @@ def expiry_time(expiry_mins=60):


class Device(TimeAuditModel):
uuid = UUIDField(version=1, hyphenate=True, unique=True, db_index=True)
uuid = models.UUIDField(default=uuid.uuid4, editable=False, unique=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so we'll move to Django's UUIDField and figure out a way to migrate old data to this.

@pradyunsg would appreciate some help around this.

So I replaced the django-uuidfield with Django's UUIDField which created a new migration. But the old migration of this field still exists which will throw error as the uuidfield package doesn't exist anymore. What do you think would be the best way to handle this scenario, currently I am commenting out the lines which uses that package in the old migration.

Also, how do I migrate the new migrations, as uuid column for this model already exists. And while doing so how do I make sure that the old data of that field is migrated to the new one.

I looked it up online but couldn't find anything on how to tackle this problem, some guidance/hints would be great. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this is resolved, as I got to know that it's better to edit the old migration files to pretend that the field was always a models.UUIDField. That way we won't require any migration. Checkout the answer at Django Forum for more clarification.

@sks444
Copy link
Contributor Author

sks444 commented Apr 8, 2020

@sks444 If you could make atomic commits, that'd make reviewing your PR (as well as iterating on them later) much easier! ^>^

Yes, thank you for the suggestion @pradyunsg , I will try to do atomic commits. Actually in this case, when I upgrade django, it starts breaking few other things, so all those fixes should go in one commit right? Or should I have a separate commit for each of the fix? But they are dependent on only one thing, i.e. upgrading django.

@pradyunsg
Copy link
Contributor

I think a separate commit per fix would be better.

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch 2 times, most recently from ddba3a4 to 5707742 Compare April 9, 2020 22:07
@sks444 sks444 marked this pull request as ready for review April 9, 2020 22:13
@sks444
Copy link
Contributor Author

sks444 commented Apr 9, 2020

I think this is ready for review now. :)

@ananyo2012 ananyo2012 closed this Apr 10, 2020
@ananyo2012 ananyo2012 reopened this Apr 10, 2020
@ananyo2012
Copy link
Contributor

@sks444 Why are new migrations created for conferences and proposals ? There is no change in conference or proposal model.

@ananyo2012
Copy link
Contributor

Also can you rebase your changes on the latest master and solve the linting errors.

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch from 5707742 to 035282b Compare April 10, 2020 13:59
@sks444
Copy link
Contributor Author

sks444 commented Apr 10, 2020

@sks444 Why are new migrations created for conferences and proposals ? There is no change in conference or proposal model.

@ananyo2012, That is coming from the simple_history package upgrade as we are using simple_history models for conference and proposal models.

I moved the migration files to simple_history package upgrade commit so that it would be reasonable.

@sks444
Copy link
Contributor Author

sks444 commented Apr 10, 2020

So everything seems to be running perfectly, tests are passing and I am interacting with the UI and checking if these changes break something. But as I don't have much experience with the project I might miss few features. @pradyunsg, @ananyo2012, would be great if you could help me with the same. :)

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch 2 times, most recently from d527886 to e70b6b2 Compare April 10, 2020 15:55
@pradyunsg pradyunsg closed this Apr 15, 2020
@pradyunsg pradyunsg reopened this Apr 15, 2020
@pradyunsg
Copy link
Contributor

Do your thing Travis CI.

@ananyo2012
Copy link
Contributor

@sks444 Can you rebase your changes over the latest master ?

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch from e70b6b2 to b8dddb9 Compare May 6, 2020 07:17
@sks444
Copy link
Contributor Author

sks444 commented May 6, 2020

Rebased @ananyo2012

@palnabarun
Copy link
Member

Note: Putting a hold on the merge until the upgrade compatibility is tested

@palnabarun
Copy link
Member

@sks444 The CI is failing due to conflicting migrations. Can you please check?

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch from b8dddb9 to ad3aea3 Compare May 7, 2020 06:35
@sks444
Copy link
Contributor Author

sks444 commented May 7, 2020

Resolved the migration issue @palnabarun

@pradyunsg
Copy link
Contributor

* Upgrade djangorestframework to latest version

This isn't supported on Python 2.

@pythonindia/pycon-india-2020-junction Do we care about Python 2 support? If not, let's drop it?

@palnabarun
Copy link
Member

@sks444 Thank you for resolving that.

@pradyunsg We do care about Python 2 at the moment.

@palnabarun palnabarun mentioned this pull request Jun 6, 2020
1 task
@onlinejudge95
Copy link

@sks444 need your input in #687

@sks444
Copy link
Contributor Author

sks444 commented Jun 6, 2020

@onlinejudge95 we can not move directly to Django 2 without merging this pr as it would break a lot of things.

I have made all the changes here which are required to upgrade to Django 1.11. Although I have tested this on my system, it would very helpful if you could also verify this on yours. So that we could go ahead and merge this and then we can try upgrading to Django 2 and see what breaks.

@onlinejudge95
Copy link

onlinejudge95 commented Jun 6, 2020

@sks444 on it

Even I was bumping the versions incrementally it failed for 1.10 for me due to obsolete dependencies.

@palnabarun
Copy link
Member

palnabarun commented Jun 6, 2020

@sks444 -- can you please change the target of your changes to django-2.2 branch? We are starting to merge Django upgrade related changes to that branch.

@sks444 sks444 force-pushed the fix-all-deprecated-warnings branch from ad3aea3 to 8d78d59 Compare June 7, 2020 07:32
@sks444 sks444 changed the base branch from master to django-2.2 June 7, 2020 07:35
@sks444
Copy link
Contributor Author

sks444 commented Jun 7, 2020

Done @palnabarun

@dishantsethi dishantsethi mentioned this pull request Jun 7, 2020
3 tasks
@onlinejudge95
Copy link

@sks444 @palnabarun Django REST framework has dropped support for Python2.7 link. I doubt we will be able to pass the installation steps in Travis build due to this.

Proposing to drop support for Python2.7, what do you say guys?

@ananyo2012
Copy link
Contributor

ananyo2012 commented Jun 10, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants