feat: Upgrade to Django 2.2 #1416
Conversation
Codecov Report
@@ Coverage Diff @@
## django_2_upgrade #1416 +/- ##
=================================================
Coverage 74.79% 74.79%
=================================================
Files 68 68
Lines 988 988
Branches 124 124
=================================================
Hits 739 739
Misses 246 246
Partials 3 3 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 17 of 20 files at r1.
Reviewable status: 17 of 20 files reviewed, 4 unresolved discussions (waiting on @faucomte97)
a discussion (no related file):
are we editing previous migrations here?
setup.py, line 25 at r1 (raw file):
include_package_data=True, install_requires=[ "django>=1.11.24, <= 2.0.0",
we don't have to support 1.11 if we have the long-running feature branch
aimmo/migrations/0003_auto_20160802_1418.py, line 104 at r1 (raw file):
to=settings.AUTH_USER_MODEL, null=True, on_delete=models.SET_NULL,
are we editing a previous migration here? I think we forgot to name this migration something human understandable
aimmo/tests/test_models.py, line 6 at r1 (raw file):
class TestModels(TestCase):
I see what this test case is testing here but I'm unsure whether how useful this test will be to keep? Each test has a maintainability cost associated to it too.
There was a problem hiding this comment.
Reviewed 3 of 20 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faucomte97)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mrniket)
a discussion (no related file):
Previously, mrniket (Niket Shah) wrote…
are we editing previous migrations here?
Yeah, it was required by Django 2.0 to update the on_delete fields everywhere including the past migrations. Without that the tests can't run and the migrations can't be run either
setup.py, line 25 at r1 (raw file):
Previously, mrniket (Niket Shah) wrote…
we don't have to support 1.11 if we have the long-running feature branch
Done.
aimmo/migrations/0003_auto_20160802_1418.py, line 104 at r1 (raw file):
Previously, mrniket (Niket Shah) wrote…
are we editing a previous migration here? I think we forgot to name this migration something human understandable
Yeah there are about 6 old migrations that have no clear name. Should I figure out what they were for and rename them?
aimmo/tests/test_models.py, line 6 at r1 (raw file):
Previously, mrniket (Niket Shah) wrote…
I see what this test case is testing here but I'm unsure whether how useful this test will be to keep? Each test has a maintainability cost associated to it too.
True, I think the test would only be useful if someone ever changes the on_delete field, so I guess it's more of a safeguard. If you think it's not really useful, I can remove it
There was a problem hiding this comment.
Reviewed 18 of 20 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @faucomte97 and @mrniket)
example_project/example_project/urls.py, line 11 at r2 (raw file):
urlpatterns = [ url(r"^", include(portal_urls)), url(r'^administration/', include((admin.site.urls[0], 'admin'), namespace='admin')),
Double quotes and idea in portal PR 🙂
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @faucomte97 and @mrniket)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97 and @razvan-pro)
aimmo/migrations/0003_auto_20160802_1418.py, line 104 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Yeah there are about 6 old migrations that have no clear name. Should I figure out what they were for and rename them?
Dw, I think it's fine to leave it as it is 😊
aimmo/tests/test_models.py, line 6 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
True, I think the test would only be useful if someone ever changes the
on_deletefield, so I guess it's more of a safeguard. If you think it's not really useful, I can remove it
Yeah, I'm in favour of removing it. @razvan-pro, second opinion?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97 and @razvan-pro)
example_project/example_project/urls.py, line 11 at r2 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Double quotes and idea in portal PR 🙂
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97 and @razvan-pro)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97 and @mrniket)
aimmo/tests/test_models.py, line 6 at r1 (raw file):
Previously, mrniket (Niket Shah) wrote…
Yeah, I'm in favour of removing it. @razvan-pro, second opinion?
Hm, it would be good to have a test like this, but maybe we can do it more isolated without going through all the models at once: e.g. test if removing a worksheet which has a game attached to it would raise an error (not sure if users can delete worksheets at the moment). If we don't have tests like this already, it may take a long time to cover everything individually now, so then yeah, we can probably delete this for now and do some functional tests in the future.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97 and @razvan-pro)
aimmo/tests/test_models.py, line 6 at r1 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Hm, it would be good to have a test like this, but maybe we can do it more isolated without going through all the models at once: e.g. test if removing a worksheet which has a game attached to it would raise an error (not sure if users can delete worksheets at the moment). If we don't have tests like this already, it may take a long time to cover everything individually now, so then yeah, we can probably delete this for now and do some functional tests in the future.
Yeah, I was thinking a higher-level test but be better in this instance. I believe we don't have tests like this currently...
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, 6 of 6 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)
# Conflicts: # aimmo/__init__.py # aimmo/tests/test_views.py
There was a problem hiding this comment.
Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @faucomte97, @mrniket, and @razvan-pro)
aimmo/tests/test_models.py, line 6 at r1 (raw file):
Previously, mrniket (Niket Shah) wrote…
Yeah, I was thinking a higher-level test but be better in this instance. I believe we don't have tests like this currently...
Done.
There was a problem hiding this comment.
Reviewed 28 of 28 files at r11.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97 and @mrniket)
Pipfile, line 10 at r11 (raw file):
pytest-django = "==4.1.0" pytest-pythonpath = "*" #codeforlife-portal = "*"
Idea: can we try getting portal from a git branch here? I see there is a way to specify this https://github.com/pypa/pipfile
setup.py, line 26 at r11 (raw file):
install_requires=[ "django==2.2.*", # "django-autoconfig >= 0.3.6, < 1.0.0",
We'll need to fix this somehow, I assume it's still needed right?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrniket and @razvan-pro)
Pipfile, line 10 at r11 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Idea: can we try getting portal from a git branch here? I see there is a way to specify this https://github.com/pypa/pipfile
I can't seem to lock the Pipfile if I do that because portal points to old repos still :(
setup.py, line 26 at r11 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
We'll need to fix this somehow, I assume it's still needed right?
yes indeed
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mrniket)
Pipfile, line 10 at r11 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I can't seem to lock the Pipfile if I do that because portal points to old repos still :(
Ah, I see, yeah, we'd need to sync all of them if we want to try this
This change is