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

Django 2.0 & 2.1 support #6402

Merged
merged 17 commits into from Jul 13, 2018

Conversation

Projects
None yet
3 participants
@toffi9
Contributor

toffi9 commented Jun 4, 2018

Before merge of this PR please merge divio/djangocms-text-ckeditor#483

Please @czpython write in review what will be your desired travis matrix for dj21 dj20 and dj111. And also please write how this matrix works because its only travis without tox.

@czpython czpython self-assigned this Jun 4, 2018

@czpython czpython added this to the 3.6.0 milestone Jun 4, 2018

@czpython

Thanks @toffi9.
Please replace all instances of if django.get_version() < x.y.z with the format from utils/compat (DJANGO_1_11)

@@ -4,7 +4,7 @@
from django.contrib import admin
from django.contrib.admin.options import csrf_protect_m
from django.core.exceptions import PermissionDenied
from django.core.urlresolvers import reverse
from django.urls import reverse

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

change the order because urls comes after http

@@ -4,7 +4,7 @@
from django.contrib.auth import get_permission_codename, get_user_model
from django.contrib.auth.models import AnonymousUser
from django.contrib.sites.models import Site
from django.core.urlresolvers import reverse, NoReverseMatch, resolve, Resolver404
from django.urls import reverse, NoReverseMatch, resolve, Resolver404

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

same note about import order

@@ -1,6 +1,6 @@
# -*- coding: utf-8 -*-
from django.conf import settings
from django.core.urlresolvers import resolve, Resolver404, reverse
from django.urls import resolve, Resolver404, reverse

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

import order

@@ -2,7 +2,7 @@
import logging
import sys
from django.core.management import color_style
from django.core.urlresolvers import clear_url_caches
from django.urls import clear_url_caches

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

import order

@@ -7,7 +7,7 @@
from django.conf import settings
from django.contrib.sites.models import Site
from django.core.mail import mail_managers
from django.core.urlresolvers import reverse
from django.urls import reverse

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

import order

@@ -990,10 +991,12 @@ def test_emulate_admin_index(self):
request.user = user
# Note, the query count is inflated by doing additional lookups
# because there's a site param in the request.
with self.assertNumQueries(FuzzyInt(3,4)):
with self.assertNumQueries(FuzzyInt(4,5)):

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

why is query count going up?

from cms.apphook_pool import apphook_pool
from cms.models.pagemodel import Page
from cms.utils import get_current_site
from cms.utils.i18n import get_language_list
from cms.utils.moderator import use_draft
try:

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

put this in the utils/compat/django file

@@ -248,8 +266,11 @@ def _get_app_patterns(site):
for lang in hooked_applications[page_id].keys():
(app_ns, inst_ns), current_patterns, app = hooked_applications[page_id][lang] # nopyflakes
if not resolver:
regex_pattern = r''
if django.VERSION >= (2, 0):

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

regex_pattern is being overriden, use an if / else or just an inline:
regex_pattern = RegexPattern(regex_pattern) if django.VERSION >= (2, 0) else r''

@@ -203,7 +204,13 @@ def get_page_actions_for_user(user, site):
for perm in page_permissions.iterator():
# set internal fk cache to our page with loaded ancestors and descendants
perm._page_cache = pages_by_id[perm.page_id]
if django.VERSION >= (2, 0):
if not hasattr(perm._state, 'fields_cache'):

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

PagePermission.page.set_cached_value(pages_by_id[perm.page_id])

if django.VERSION >= (2, 0):
if not hasattr(perm._state, 'fields_cache'):
perm._state.fields_cache = {}
perm._state.fields_cache['page'] = pages_by_id[perm.page_id]

This comment has been minimized.

@czpython

czpython Jun 11, 2018

Contributor

PagePermission.page.set_cached_value(pages_by_id[perm.page_id])

@toffi9

This comment has been minimized.

Contributor

toffi9 commented Jun 27, 2018

Still need to resolve conflicts and provide CI config

@@ -4,7 +4,7 @@
from django.core.cache import cache
from django.conf import settings
from django.contrib.auth.models import Permission
from django.core.urlresolvers import clear_url_caches
from django.urls import clear_url_caches

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

Still applies

@@ -203,7 +204,11 @@ def get_page_actions_for_user(user, site):
for perm in page_permissions.iterator():
# set internal fk cache to our page with loaded ancestors and descendants
perm._page_cache = pages_by_id[perm.page_id]
if not DJANGO_1_11:

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

I suggest to use if DJANGO_1_11: pass else:pass
With comments as to one being django 1.11 only and the other django 2.0 and up

@@ -354,7 +359,10 @@ def get_view_restrictions(pages):
for perm in page_permissions:
# set internal fk cache to our page with loaded ancestors and descendants
perm._page_cache = pages_by_id[perm.page_id]
if not DJANGO_1_11:

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

same as above

from cms.exceptions import LanguageError
from cms.utils.conf import get_cms_setting, get_site_id
try:

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

maybe add to compat module

@@ -663,6 +666,7 @@ def copy(self, site, parent_node=None, language=None,
parent_page = None
new_page = copy.copy(self)
new_page._state = ModelState()

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

this is a bit dangerous I think, what is the reason for it?

This comment has been minimized.

@toffi9

toffi9 Jul 6, 2018

Contributor

Problem is that from dj20 fields caching is in _state object when we copy page like above, then also a _state object is copied, and it's referencing to the same object as original page, and it generated failures in tests. I resolved this issue by creating new instance of this object, this is done the same way as in django (https://github.com/django/django/blob/1.11/django/db/models/base.py#L481).

@@ -1117,8 +1129,20 @@ def clear_placeholder(self, request, placeholder_id):
opts = Placeholder._meta
using = router.db_for_write(Placeholder)
plugins = placeholder.get_plugins_list(language)
get_deleted_objects_additional_kwargs = {}

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

remove this line

self.instances[command] = instance
self.stealth_options = tuple(set(stealth_options))

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

why is this casted to a set and then a tuple?

This comment has been minimized.

@toffi9

toffi9 Jul 6, 2018

Contributor

set because I wanted only unique entries and tuple to back to original type

@@ -442,8 +443,20 @@ def delete_view(self, request, object_id, extra_context=None):
# Populate deleted_objects, a data structure of all related objects that
# will also be deleted.
objs = [obj] + list(obj.get_descendant_pages())
get_deleted_objects_additional_kwargs = {}

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

remove this line

options={'default_permissions': ('add', 'change', 'delete'), 'permissions': (('view_page', 'Can view page'), ('publish_page', 'Can publish page'), ('edit_static_placeholder', 'Can edit static placeholders')), 'verbose_name': 'page', 'verbose_name_plural': 'pages'},
),
]
if not DJANGO_2_0:

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

why is this needed on django >= 2.1?

This comment has been minimized.

@toffi9

toffi9 Jul 6, 2018

Contributor

When running tests against dj21 it's showing missing migration

@@ -38,6 +38,27 @@ class PagePermissionInlineAdmin(TabularInline):
extra = 0 # edit page load time boost
show_with_view_permissions = False
def has_change_permission(self, request, obj=None):

This comment has been minimized.

@czpython

czpython Jul 3, 2018

Contributor

there's no pagepermission permission.
did you get a failing test?

This comment has been minimized.

@toffi9

toffi9 Jul 6, 2018

Contributor

https://github.com/django/django/blob/2.1b1/django/contrib/admin/options.py#L2060 this is causing the need for this code. In django 2.1 they do this and if user dont have sufficient permissions fields are removed. And it's causing test errors. It's somehow described here: https://docs.djangoproject.com/en/2.1/releases/2.1/#custom-admin-forms-need-to-take-the-view-only-case-into-account. Solution showed by django docs it's for very simple case, and its not working for us.

This comment has been minimized.

@toffi9

toffi9 Jul 13, 2018

Contributor

@czpython obj here is always Page model, not PagePermission seems like django works like that in ModelAdmin in changeform_view>>_changeform_view>>_create_formsets>>get_formsets_with_inlines>>get_inline_instances(self, request, obj=None)

@toffi9 toffi9 force-pushed the toffi9:feature/django20comp branch from ba48e40 to 19f1201 Jul 6, 2018

@coveralls

This comment has been minimized.

coveralls commented Jul 6, 2018

Coverage Status

Coverage remained the same at 78.189% when pulling 7c37df1 on toffi9:feature/django20comp into 8964d77 on divio:develop.

toffi9 added some commits Jul 9, 2018

@@ -58,18 +62,22 @@ def add_arguments(self, parser):
self.instances = {}
if self.subcommands:
stealth_options = list(self.stealth_options)

This comment has been minimized.

@czpython

czpython Jul 13, 2018

Contributor

change this to set()

)
add_builtin_arguments(parser=parser_sub)
instance.add_arguments(parser_sub)
stealth_options.extend({action.dest for action in parser_sub._actions})

This comment has been minimized.

@czpython

czpython Jul 13, 2018

Contributor

change this to add that to set from above

self.instances[command] = instance
self.stealth_options = tuple(set(stealth_options))

This comment has been minimized.

@czpython

czpython Jul 13, 2018

Contributor

remove set() from here
also does order matter for this?

toffi9 and others added some commits Jul 13, 2018

@czpython czpython merged commit 50dee52 into divio:develop Jul 13, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 13, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 16, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 16, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 16, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 16, 2018

czpython added a commit to czpython/django-cms that referenced this pull request Jul 16, 2018

czpython added a commit that referenced this pull request Jul 16, 2018

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