Skip to content

/ pythondotorg

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

Jump to bottom

Upgrade to Django 2.2. #1830

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Upgrade to Django 2.2. #1830

wants to merge 1 commit into from
+476 −264

Conversation

@luzfcb
Copy link

@luzfcb luzfcb commented Aug 1, 2021 •
edited

Fix #1819

  • Replaced the os.environ usage by python-decouple
  • Added the env_sample file with the environment variables that is available to use. If a file named .env exists on the project root path, the python-decouple will read. Note: Environment variables have precedence over the python-decouple config files.
  • Upgraded to Django 2.2.
  • Updated dependencies for django 2.2 compatibility
  • Synced django manage.py implementation with upstream
  • Added the apps.py files for all modules that have migrations, template tags, or management commands
  • Use path() or re_path() instead of url()
  • Use gettext_lazy instead of ugettext_lazy
  • Renamed filter_fields to filterset_fields and filter_class to filterset_class to be compatible with the new version of django-filter
  • Removed W605 invalid escape sequence \ in events/tests/test_importer.py (related to https://docs.python.org/3/whatsnew/3.6.html#deprecated-python-behavior)
  • Fix factory_boy DjangoModelFactory imports
  • Fix url_name function shadowing on pydotorg/context_processors.py
  • Added FORM_RENDERER = 'django.forms.renderers.DjangoTemplates' to pydotorg/settings/base.py
  • Added the obj=None argument on sponsors.admin.SponsorBenefitInline.has_add_permission method
  • Renamed base_name to basename on pydotorg/urls_api.py to be compatible with the new version of django-restframework
  • Added a missing migration to the added_by_user field of SponsorBenefit model that that Django <=2.0 cannot detect. This migration only includes blank=True in the migration and does not touch the database, which means do NOT generate SQL.
  • Added a test case to detect missing migrations.
  • Load static templatetag instead of admin_static.
  • Load static templatetag instead of staticfiles.
  • Replace NullBooleanField() by BooleanField(null=True). Note: The migration will not generate an SQL. Django 2.1 release notes recommend this change but only in Django 3.1 NullBooleanField is effectively marked as deprecated.
  • Fork the django-easy-pdf to use BytesIO from io and not from six

TODO:

  • Upgrade django-allauth
  • Migrate to sentry-sdk (raven is deprecated)
  • Check if it is possible to generate the OpenAPI schema and after using it as a source for schemathesis generate and run tests and thus ensure that there are no update errors (at least in the underlying code that can be executed via API)

Django 3.2 TODO:

Note: The code as it is in this pull-request is already capable of running the test suite with Django 3.2

  • Add DEFAULT_AUTO_FIELD = 'django.db.models.AutoField' to pydotorg/settings/base.py to avoid unwanted migrations
  • Replace from django.contrib.postgres.fields import JSONField by from django.db.models import JSONField on community/models.py and deal with the migrations.
  • Remove default_app_config variable from all __init__.py files of all apps.

Note:

Since there are many changes, I will redo the commits and break them down into smaller changes. (or maybe create multiple pull-requests)
@luzfcb luzfcb force-pushed the labcodes:django2.2 branch 7 times, most recently from 580250d to d302197 Aug 1, 2021
@luzfcb
Copy link
Author

@luzfcb luzfcb commented Aug 2, 2021 •
edited

@berinhard @ewdurbin I have no idea the cause of this error. https://app.travis-ci.com/github/python/pythondotorg/builds/234219521#L1327-L1429
Any suggestion? Any help is welcome.

@luzfcb luzfcb marked this pull request as ready for review Aug 2, 2021
@luzfcb
luzfcb reviewed Aug 2, 2021
View changes
.gitignore Show resolved Hide resolved
@luzfcb
luzfcb reviewed Aug 2, 2021
View changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@luzfcb luzfcb marked this pull request as draft Aug 2, 2021
@berinhard
berinhard reviewed Aug 10, 2021
View changes
Copy link
Collaborator

@berinhard berinhard left a comment

@luzfcb thanks again for opening such an important PR. I noticed that there are 3 tests failing but I didn't have the opportunity to run then locally to investigate the problem. As soon as I finish with other priorities, I'll investigate them. If you want to pair on them, I'll be happy to do so. I'm usually available during morning time.
base-requirements.txt
xhtml2pdf==0.2.5
django-easy-pdf==0.1.1
django-easy-pdf@https://github.com/labcodes/django-easy-pdf/archive/60f2a70a056655d271a215ceded57f97ffaa4fea.zip

This comment has been minimized.

Sign in to view
@berinhard

berinhard Aug 10, 2021
Collaborator

@luzfcb why to use a custom fork instead of a regular version?

This comment has been minimized.

Sign in to view
@luzfcb

luzfcb Aug 10, 2021
Author

Is only this change:
labcodes/django-easy-pdf@60f2a70

Was required to replace
from django.utils.six import BytesIO
by
from io import BytesIO

because the django.utils.six is deprecated.

Anyway, django-easy-pdf seems unmaintained project

This comment has been minimized.

Sign in to view
@berinhard

berinhard Aug 11, 2021
Collaborator

Thanks, it makes sense to me. @ewdurbin how to handle this? Should we add a fork Labcode's fork with @python organization so we have control of such type of customization.
.pre-commit-config.yaml Outdated Show resolved Hide resolved
base-requirements.txt Outdated Show resolved Hide resolved
env_sample Show resolved Hide resolved
events/tests/test_importer.py Show resolved Hide resolved
users/models.py Show resolved Hide resolved
users/migrations/0014_auto_20210801_2332.py Outdated Show resolved Hide resolved
sponsors/migrations/0031_auto_20210801_2038.py Outdated Show resolved Hide resolved
@luzfcb luzfcb force-pushed the labcodes:django2.2 branch from d302197 to 3fd5efb Aug 11, 2021
@luzfcb luzfcb requested a review from berinhard Aug 11, 2021
@luzfcb
Copy link
Author

@luzfcb luzfcb commented Aug 11, 2021

@berinhard I updated the description, made some requested changes, and rebased with the main branch. Can review again?

@luzfcb luzfcb force-pushed the labcodes:django2.2 branch from 3fd5efb to 106e49c Aug 11, 2021
@luzfcb
Copy link
Author

@luzfcb luzfcb commented Aug 11, 2021 •
edited 

I added a test case to check for missing migrations.
If this test fails, something like this will be shown:

```python
FAIL: test_missing_migrations (pydotorg.tests.test_migrations.MissingMigrationTestCase)
Tests if there are any changes left in the Django models for which the migration script was not generated.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/pythondotorg/pydotorg/tests/test_migrations.py", line 24, in test_missing_migrations
    self.assertFalse(
AssertionError: {'sponsors': [<Migration sponsors.0031_auto_20210811_0901>]} is not false : Missing migrations were detected on the following apps: sponsors. Run `python3 manage.py makemigrations` to generate new ones.
```

https://app.travis-ci.com/github/python/pythondotorg/builds/235045671#L1655

yes, I know there is `python manage.py makemigrations --dry-run --check`, but that way, we ensure there will never be a missing migration even if we forget to run the `makemigrations --dry-run --check`

I created a separate pull-request for this issue #1839

@luzfcb luzfcb force-pushed the labcodes:django2.2 branch 3 times, most recently from 0ba20dc to 7785b5d Aug 19, 2021
@luzfcb
Upgrade to Django 2.2.
9d8449b 
Update dependencies for django 2.2 compatibility
@luzfcb luzfcb force-pushed the labcodes:django2.2 branch from 7785b5d to 9d8449b Aug 21, 2021
@luzfcb
luzfcb reviewed Aug 21, 2021
View changes
templates/components/navigation-widget.html

{% elif URL_NAME %}
{% sitetree_tree from URL_NAME template "sitetree/sidebar_menu_root.html" %}
{% endif %}
Comment on lines -18 to +19

This comment has been minimized.

Sign in to view
@luzfcb

luzfcb Aug 21, 2021
Author

In theory, this makes the 3 tests start to pass
https://app.travis-ci.com/github/python/pythondotorg/builds/235762782#L1343-L1648

I'm not sure this is the correct way to fix this.

Anyway, thanks @lucianoratamero for helping me debug this error.
@luzfcb luzfcb marked this pull request as ready for review Aug 21, 2021
@luzfcb
Copy link
Author

@luzfcb luzfcb commented Aug 21, 2021

@berinhard @ewdurbin this pull request is ready for review.

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

@berinhard berinhard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Upgrade to Django 2.2 LTS version
2 participants
@luzfcb @berinhard