Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

refactor: upgrade django to 3.0.8 #578

Merged
merged 1 commit into from
Jul 24, 2020
Merged

refactor: upgrade django to 3.0.8 #578

merged 1 commit into from
Jul 24, 2020

Conversation

satya7289
Copy link
Contributor

@satya7289 satya7289 commented Jul 8, 2020 •
edited
Loading

Description

Upgrade django to 3.0.8.
major changes for upgrading to 3.0.8 are as follows

  • update from django.core.urlresolvers import reverse -> from django.urls import reverse
  • add on_delete=models.CASCADE to foreignkey and onetoonekey relationship for the model.
  • update requirement files.
  • remove old migration to latest one(because some of the migrations are created with python2.7 which conflict with python3 or django 2.0)

Fixes #565

Type of Change:

Delete irrelevant options.

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)

How Has This Been Tested?

pass all test cases. running fine locally.
Screenshot from 2020-07-14 14-27-00

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • New and existing unit tests pass locally with my changes

SanketDG
SanketDG suggested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

I don't remember deciding with you or @sakshi1499 that we are going to stick to using version 2. Django 2.0 was almost released two years ago and has already reached support end https://www.djangoproject.com/download/#supported-versions

We can go with 2.2 LTS, but given Anita-B is a fast moving organization it makes no sense to use an older LTS version.

I would like to know what problems you faced with upgrading to 3.0, and we can all try to fix them.

requirements/prod.txt Show resolved Hide resolved
@gaurivn gaurivn added the Status: Changes Requested Changes are required to be done by the PR author. label Jul 9, 2020
theyashshahs
theyashshahs suggested changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@theyashshahs theyashshahs left a comment

Choose a reason for hiding this comment

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

@satya7289 remove the migrations from the folder

@satya7289
Copy link
Contributor Author

@satya7289 remove the migrations from the folder

I have to modify the migrations file for upgrading django to latest version(as some of the migrations where created using python2.7 which is not supported by django latest version). see

@SanketDG SanketDG mentioned this pull request Jul 11, 2020
Closed
2 tasks
theyashshahs
theyashshahs reviewed Jul 12, 2020
View reviewed changes
Copy link
Contributor

@theyashshahs theyashshahs left a comment

Choose a reason for hiding this comment

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

@satya7289 can you provide a GIF or a screenshot of the working :)

@satya7289 satya7289 changed the title Upgrade django to 2.0 refactor: upgrade django to 2.2.14 Jul 14, 2020
@satya7289 satya7289 added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jul 14, 2020
@SanketDG
Copy link
Contributor

@satya7289 Did you make the changes as we discussed for Django 3.0?

@satya7289 satya7289 force-pushed the upgrade-Django branch 2 times, most recently from 7b3ecc3 to 09eaffb Compare July 18, 2020 18:22
@satya7289 satya7289 changed the title refactor: upgrade django to 2.2.14 refactor: upgrade django to 3.0.8 Jul 18, 2020
@SanketDG SanketDG force-pushed the upgrade-Django branch 3 times, most recently from c2a0e46 to 9c0fab0 Compare July 21, 2020 11:37
@SanketDG SanketDG mentioned this pull request Jul 21, 2020
Closed
@satya7289 satya7289 force-pushed the upgrade-Django branch 2 times, most recently from b73d097 to 7179e2d Compare July 21, 2020 14:56
This was referenced Jul 21, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
SanketDG
SanketDG reviewed Jul 21, 2020
View reviewed changes
.travis.yml
@@ -44,4 +47,4 @@ notifications:
on_failure: always
on_start: false
after_success:
coveralls --rcfile=.coveragerc
coveralls --rcfile=.coveragerc
Copy link
Contributor

Choose a reason for hiding this comment

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

you removed the newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it metter here, as it is not .py file!!!

@SanketDG
Copy link
Contributor

Great work @satya7289 !

Why did you add html file changes though? They cannot be part of this PR.

@satya7289
Copy link
Contributor Author

satya7289 commented Jul 21, 2020 •
edited
Loading

Great work @satya7289 !

Why did you add html file changes though? They cannot be part of this PR.

{% load staticfiles %} has been changed to {% load static %} for django 3.0.8

SanketDG
SanketDG suggested changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

A few nitpicks.

Also the migrations were generated with Django 2, can you re-generate them with Django 3?

setup.cfg
@@ -1,4 +1,4 @@
[flake8]
ignore = F403
ignore = F403, F405
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the need for adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to import * is used some files. Exactly not remember which files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any star imports though

systers_portal/templates/common/contact.html Outdated
@@ -19,4 +20,4 @@ <h1 class="contact-head">Contact Systers</h1>
</div>
<div class="mt40"></div>
</div>
{% endblock %}
{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

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

add the newlines back

@satya7289 satya7289 requested a review from SanketDG July 21, 2020 18:34
@satya7289 satya7289 mentioned this pull request Jul 22, 2020
Open
2 tasks
This was referenced Jul 24, 2020
Closed
Closed
SanketDG
SanketDG approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

This looks really good!

@satya7289 Excellent work, and patience!

This was referenced Jul 24, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@SanketDG SanketDG merged commit 3789c61 into anitab-org:develop Jul 24, 2020
@SanketDG
Copy link
Contributor

🎉 🎉

This is great @satya7289

I have opened #622 (will update it soon) for some warnings during the test run

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SanketDG SanketDG SanketDG approved these changes

@sakshi1499 sakshi1499 sakshi1499 approved these changes

@theyashshahs theyashshahs Awaiting requested review from theyashshahs

Assignees
No one assigned
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade Django to latest version
5 participants
@satya7289 @SanketDG @sakshi1499 @theyashshahs @gaurivn