Skip to content

Commit

Permalink
add refactor Django code for backward compatible syntax changes (#9300)
Browse files Browse the repository at this point in the history
## Purpose

Get backwards compatible code out of the way before we flip over to Django 2

## Changes
- Make Many-to-manys use direct assignment
- add  `on_delete` kwarg to  foreign key fields
- Get urls helpers from  `django.urls` instead of `django.core.urlresolvers` which is depreciated
- Change url namespaces for new Django 2 url format.

## QA Notes

- No data migration
- Low risk
- No permissions code touched
- No user facing changes

## Documentation

Tech task, no new docs.

## Side Effects

None that I know of.

## Ticket

https://openscience.atlassian.net/browse/ENG-1392
  • Loading branch information
Johnetordoff committed May 18, 2020
1 parent 9521eea commit 312c347
Show file tree
Hide file tree
Showing 48 changed files with 137 additions and 106 deletions.
2 changes: 1 addition & 1 deletion addons/base/tests/models.py
Expand Up @@ -264,7 +264,7 @@ def test_folder_defaults_to_none(self):
assert_is_none(node_settings.folder_id)

This comment has been minimized.

Copy link
@frankduff123

frankduff123 Sep 11, 2020

    self.user_settings.revoke_oauth_access(self.external_account)
    assert_equal(mock_revoke.call_count, 0)
def test_complete_auth_false(self):
    self.node_settings.user_settings = None
    assert_false(self.node_settings.has_auth)
    assert_false(self.node_settings.complete)
def test_fields(self):
    node_settings = self.NodeSettingsClass(owner=ProjectFactory(), user_settings=self.user_settings)
    node_settings.save()
    assert_true(node_settings.user_settings)
    assert_equal(node_settings.user_settings.owner, self.user)
    assert_true(hasattr(node_settings, 'folder_id'))
    assert_true(hasattr(node_settings, 'user_settings'))
def test_folder_defaults_to_none(self):
    node_settings = self.NodeSettingsClass(user_settings=self.user_settings)
    node_settings.save()
    assert_is_none(node_settings.folder_id)

assert_false(ser.user_is_owner)

def test_user_is_owner_node_not_authorized_user_has_no_accounts(self):
    self.user.external_accounts = []
    self.user.external_accounts.clear()
    assert_false(self.user_settings.external_accounts.count())
    assert_false(self.ser.user_is_owner)

def test_user_is_owner_node_not_authorized_user_has_accounts(self):
    assert_true(self.user_settings.external_accounts.count())
    assert_true(self.ser.user_is_owner)
def test_user_is_owner_node_authorized_user_is_not_owner(self):
    self.node_settings.external_account = self.ExternalAccountFactory()
    with mock.patch('addons.base.models.BaseOAuthUserSettings.verify_oauth_access',
            return_value=True):
        self.user.external_accounts = []
        self.user.external_accounts.clear()
        assert_false(self.ser.user_is_owner)

def test_user_is_owner_node_authorized_user_is_owner(self):

addons/base/tests/serializers.py
@@ -99,7 +99,7 @@ def test_user_is_owner_no_node_settings(self):
assert_false(ser.user_is_owner)

def test_user_is_owner_node_not_authorized_user_has_no_accounts(self):
    self.user.external_accounts = []
    self.user.external_accounts.clear()
    assert_false(self.user_settings.external_accounts.count())
    assert_false(self.ser.user_is_owner)

def test_user_is_owner_node_not_authorized_user_has_accounts(self):
    assert_true(self.user_settings.external_accounts.count())
    assert_true(self.ser.user_is_owner)
def test_user_is_owner_node_authorized_user_is_not_owner(self):
    self.node_settings.external_account = self.ExternalAccountFactory()
    with mock.patch('addons.base.models.BaseOAuthUserSettings.verify_oauth_access',
            return_value=True):
        self.user.external_accounts = []
        self.user.external_accounts.clear()
        assert_false(self.ser.user_is_owner)self.account = account
def __repr__(self):
    return '<{name}: {status}>'.format(
        name=self.__class__.__name__,
        status=self.account.display_name if self.account else 'anonymous'
    )

class UserSettings(BaseOAuthUserSettings):
oauth_provider = GitLabProvider
serializer = GitLabSerializer
class NodeSettings(BaseOAuthNodeSettings, BaseStorageAddon):
oauth_provider = GitLabProvider
serializer = GitLabSerializer
user = models.TextField(blank=True, null=True)
repo = models.TextField(blank=True, null=True)
repo_id = models.TextField(blank=True, null=True)
hook_id = models.TextField(blank=True, null=True)
hook_secret = models.TextField(blank=True, null=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True, on_delete=models.CASCADE)

@property
def folder_id(self):

assert_false(ser.user_is_owner)

def test_user_is_owner_node_authorized_user_is_owner(self):

self.account = account
def repr(self):
return '<{name}: {status}>'.format(
name=self.class.name,
status=self.account.display_name if self.account else 'anonymous'
)
class UserSettings(BaseOAuthUserSettings):
oauth_provider = GitLabProvider
serializer = GitLabSerializer
class NodeSettings(BaseOAuthNodeSettings, BaseStorageAddon):
oauth_provider = GitLabProvider
serializer = GitLabSerializer
user = models.TextField(blank=True, null=True)
repo = models.TextField(blank=True, null=True)
repo_id = models.TextField(blank=True, null=True)
hook_id = models.TextField(blank=True, null=True)
hook_secret = models.TextField(blank=True, null=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True, on_delete=models.CASCADE)

@property
def folder_id(self):

def test_has_auth(self):
self.user.external_accounts = []
self.user.external_accounts.clear()
self.user_settings.reload()
node = ProjectFactory()
settings = self.NodeSettingsClass(user_settings=self.user_settings, owner=node)
Expand Down
4 changes: 2 additions & 2 deletions addons/base/tests/serializers.py
Expand Up @@ -99,7 +99,7 @@ def test_user_is_owner_no_node_settings(self):
assert_false(ser.user_is_owner)

def test_user_is_owner_node_not_authorized_user_has_no_accounts(self):
self.user.external_accounts = []
self.user.external_accounts.clear()
assert_false(self.user_settings.external_accounts.count())
assert_false(self.ser.user_is_owner)

Expand All @@ -111,7 +111,7 @@ def test_user_is_owner_node_authorized_user_is_not_owner(self):
self.node_settings.external_account = self.ExternalAccountFactory()
with mock.patch('addons.base.models.BaseOAuthUserSettings.verify_oauth_access',
return_value=True):
self.user.external_accounts = []
self.user.external_accounts.clear()
assert_false(self.ser.user_is_owner)

def test_user_is_owner_node_authorized_user_is_owner(self):
Expand Down
2 changes: 1 addition & 1 deletion addons/gitlab/models.py
Expand Up @@ -76,7 +76,7 @@ class NodeSettings(BaseOAuthNodeSettings, BaseStorageAddon):
repo_id = models.TextField(blank=True, null=True)
hook_id = models.TextField(blank=True, null=True)
hook_secret = models.TextField(blank=True, null=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True, on_delete=models.CASCADE)

@property
def folder_id(self):
Expand Down
2 changes: 1 addition & 1 deletion addons/onedrive/models.py
Expand Up @@ -98,7 +98,7 @@ class NodeSettings(BaseOAuthNodeSettings, BaseStorageAddon):

folder_id = models.TextField(null=True, blank=True)
folder_path = models.TextField(null=True, blank=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True)
user_settings = models.ForeignKey(UserSettings, null=True, blank=True, on_delete=models.CASCADE)

_api = None

Expand Down
2 changes: 1 addition & 1 deletion admin/asset_files/views.py
@@ -1,5 +1,5 @@
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.db.models import Case, CharField, Value, When
from django.forms.models import model_to_dict
from django.views.generic import ListView, DetailView, View, CreateView, DeleteView, UpdateView
Expand Down
2 changes: 1 addition & 1 deletion admin/banners/views.py
Expand Up @@ -5,7 +5,7 @@

from django.shortcuts import redirect
from django.forms.models import model_to_dict
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.views.generic import ListView, DetailView, View, CreateView, UpdateView, DeleteView
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.contrib import messages
Expand Down
2 changes: 1 addition & 1 deletion admin/base/utils.py
Expand Up @@ -4,7 +4,7 @@
from osf.models import Subject, NodeLicense

from django.core.exceptions import ValidationError, PermissionDenied
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.validators import RegexValidator, _lazy_re_compile
from django.utils.http import urlencode
from django.utils.translation import ugettext_lazy as _
Expand Down
4 changes: 2 additions & 2 deletions admin/collection_providers/views.py
Expand Up @@ -3,7 +3,7 @@
from django.http import HttpResponse
from django.core import serializers
from django.core.exceptions import ValidationError
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.shortcuts import redirect
from django.views.generic import View, CreateView, ListView, DetailView, UpdateView, DeleteView, TemplateView
from django.contrib import messages
Expand Down Expand Up @@ -317,7 +317,7 @@ def create_or_update_provider(self, provider_data):
provider.primary_collection.program_area_choices = primary_collection['fields']['program_area_choices']
provider.primary_collection.save()
if licenses:
provider.licenses_acceptable = licenses
provider.licenses_acceptable.add(*licenses)
if default_license:
provider.default_license = NodeLicense.objects.get(license_id=default_license)
return provider
2 changes: 1 addition & 1 deletion admin/common_auth/admin.py
Expand Up @@ -3,7 +3,7 @@
from django.contrib import admin
from django.contrib.admin.models import DELETION
from django.contrib.auth.models import Permission
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.utils.html import escape

from osf.models import AdminLogEntry
Expand Down
2 changes: 1 addition & 1 deletion admin/common_auth/urls.py
@@ -1,7 +1,7 @@
from __future__ import absolute_import

from django.conf.urls import url
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.contrib.auth.views import password_change, password_change_done

from admin.common_auth import views
Expand Down
2 changes: 1 addition & 1 deletion admin/common_auth/views.py
@@ -1,6 +1,6 @@
from __future__ import absolute_import, unicode_literals

from django.core.urlresolvers import reverse, reverse_lazy
from django.urls import reverse, reverse_lazy
from django.http import Http404
from django.shortcuts import redirect
from django.utils.decorators import method_decorator
Expand Down
2 changes: 1 addition & 1 deletion admin/institutions/views.py
Expand Up @@ -6,7 +6,7 @@
from django.core import serializers
from django.shortcuts import redirect
from django.forms.models import model_to_dict
from django.core.urlresolvers import reverse_lazy, reverse
from django.urls import reverse_lazy, reverse
from django.http import HttpResponse, JsonResponse
from django.views.generic import ListView, DetailView, View, CreateView, UpdateView, DeleteView, TemplateView
from django.views.generic.edit import FormView
Expand Down
4 changes: 2 additions & 2 deletions admin/meetings/views.py
Expand Up @@ -3,7 +3,7 @@

from django.contrib.auth.mixins import PermissionRequiredMixin
from django.views.generic import ListView, FormView
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.http import Http404

from framework.auth.core import get_user
Expand Down Expand Up @@ -113,7 +113,7 @@ def form_valid(self, form):
**data
)
new_conf.save()
new_conf.admins = admin_users
new_conf.admins.add(*admin_users)
new_conf.field_names.update(custom_fields)
new_conf.save()
return super(MeetingCreateFormView, self).form_valid(form)
Expand Down
2 changes: 1 addition & 1 deletion admin/nodes/templatetags/node_extras.py
@@ -1,5 +1,5 @@
from django import template
from django.core.urlresolvers import reverse
from django.urls import reverse

from osf.models import Node, OSFUser

Expand Down
2 changes: 1 addition & 1 deletion admin/osf_groups/views.py
@@ -1,5 +1,5 @@
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.views.generic import FormView, ListView

from osf.models import OSFGroup
Expand Down
5 changes: 3 additions & 2 deletions admin/preprint_providers/views.py
Expand Up @@ -6,7 +6,7 @@
from django.http import Http404
from django.core import serializers
from django.core.exceptions import ValidationError
from django.core.urlresolvers import reverse_lazy, reverse
from django.urls import reverse_lazy, reverse
from django.http import HttpResponse, JsonResponse
from django.views.generic import ListView, DetailView, View, CreateView, DeleteView, TemplateView, UpdateView
from django.views.generic.edit import FormView
Expand Down Expand Up @@ -331,7 +331,8 @@ def create_or_update_provider(self, provider_data):
provider.save()

if licenses:
provider.licenses_acceptable = licenses
provider.licenses_acceptable.clear()
provider.licenses_acceptable.add(*licenses)
if default_license:
provider.default_license = NodeLicense.objects.get(license_id=default_license)
# Only adds the JSON taxonomy if there is no existing taxonomy data
Expand Down
2 changes: 1 addition & 1 deletion admin/preprints/views.py
Expand Up @@ -2,7 +2,7 @@

from django.views.generic import UpdateView, DeleteView, ListView
from django.utils import timezone
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.shortcuts import redirect
from django.views.defaults import page_not_found
Expand Down
4 changes: 2 additions & 2 deletions admin/registration_providers/views.py
Expand Up @@ -4,7 +4,7 @@
from django.core import serializers
from django.core.exceptions import ValidationError
from django.core.management import call_command
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.shortcuts import redirect
from django.views.generic import View, CreateView, ListView, DetailView, UpdateView, DeleteView, TemplateView
from django.contrib.auth.mixins import PermissionRequiredMixin
Expand Down Expand Up @@ -294,7 +294,7 @@ def create_or_update_provider(self, provider_data):
provider.save()

if licenses:
provider.licenses_acceptable = licenses
provider.licenses_acceptable.add(*licenses)
if default_license:
provider.default_license = NodeLicense.objects.get(license_id=default_license)

Expand Down
2 changes: 1 addition & 1 deletion admin/subjects/views.py
@@ -1,5 +1,5 @@
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.core.urlresolvers import reverse_lazy
from django.urls import reverse_lazy
from django.views.generic import ListView, UpdateView

from admin.subjects.forms import SubjectForm
Expand Down
2 changes: 1 addition & 1 deletion admin/users/templatetags/user_extras.py
@@ -1,5 +1,5 @@
from django import template
from django.core.urlresolvers import reverse
from django.urls import reverse

register = template.Library()

Expand Down
2 changes: 1 addition & 1 deletion admin/users/views.py
Expand Up @@ -9,7 +9,7 @@
from django.views.generic import FormView, DeleteView, ListView, TemplateView
from django.contrib import messages
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.core.mail import send_mail
from django.http import Http404, HttpResponse
Expand Down
2 changes: 1 addition & 1 deletion admin_tests/meetings/test_views.py
Expand Up @@ -2,7 +2,7 @@

from django.test import RequestFactory
from django.http import Http404
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.contrib.auth.models import Permission
from django.core.exceptions import PermissionDenied

Expand Down
2 changes: 1 addition & 1 deletion admin_tests/nodes/test_views.py
Expand Up @@ -24,7 +24,7 @@
from nose import tools as nt
from django.utils import timezone
from django.test import RequestFactory
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission
from framework.auth.core import Auth
Expand Down
2 changes: 1 addition & 1 deletion admin_tests/preprint_providers/test_views.py
Expand Up @@ -177,7 +177,7 @@ def setUp(self):
self.import_view = views.ImportPreprintProvider()
self.import_view = setup_user_view(self.import_view, self.import_request, user=self.user)

self.preprint_provider.licenses_acceptable = [NodeLicense.objects.get(license_id='NONE')]
self.preprint_provider.licenses_acceptable.add(*[NodeLicense.objects.get(license_id='NONE')])
self.subject = SubjectFactory(provider=self.preprint_provider)

def test_post(self):
Expand Down
2 changes: 1 addition & 1 deletion admin_tests/preprints/test_views.py
Expand Up @@ -2,7 +2,7 @@
import mock

from django.test import RequestFactory
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission, Group, AnonymousUser
from django.contrib.messages.storage.fallback import FallbackStorage
Expand Down
2 changes: 1 addition & 1 deletion admin_tests/subjects/test_views.py
@@ -1,6 +1,6 @@
from nose import tools as nt
from django.test import RequestFactory
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission

Expand Down
2 changes: 1 addition & 1 deletion admin_tests/users/test_views.py
Expand Up @@ -9,7 +9,7 @@
from django.test import RequestFactory
from django.http import Http404
from django.core.files.uploadedfile import SimpleUploadedFile
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission
from django.contrib.messages.storage.fallback import FallbackStorage
Expand Down
2 changes: 1 addition & 1 deletion api/base/authentication/drf.py
Expand Up @@ -30,7 +30,7 @@ def get_session_from_cookie(cookie_val):
"""

try:
session_id = itsdangerous.Signer(settings.SECRET_KEY).unsign(cookie_val)
session_id = itsdangerous.Signer(settings.SECRET_KEY).unsign(cookie_val).decode()
except itsdangerous.BadSignature:
return None
try:
Expand Down
2 changes: 1 addition & 1 deletion api/base/pagination.py
@@ -1,6 +1,6 @@
from django.utils import six
from collections import OrderedDict
from django.core.urlresolvers import reverse
from django.urls import reverse
from django.core.paginator import InvalidPage, Paginator as DjangoPaginator
from django.db.models import QuerySet

Expand Down
6 changes: 3 additions & 3 deletions api/base/urls.py
Expand Up @@ -33,7 +33,7 @@
url(r'^status/', views.status_check, name='status_check'),
url(r'^actions/', include('api.actions.urls', namespace='actions')),
url(r'^addons/', include('api.addons.urls', namespace='addons')),
url(r'^alerts/', include('api.alerts.urls', namespace='alerts')),
url(r'^alerts/', include(('api.alerts.urls', 'alerts'), namespace='alerts')),
url(r'^applications/', include('api.applications.urls', namespace='applications')),
url(r'^citations/', include('api.citations.urls', namespace='citations')),
url(r'^collections/', include('api.collections.urls', namespace='collections')),
Expand All @@ -56,7 +56,7 @@
url(r'^regions/', include('api.regions.urls', namespace='regions')),
url(r'^providers/', include('api.providers.urls', namespace='providers')),
url(r'^registrations/', include('api.registrations.urls', namespace='registrations')),
url(r'^requests/', include('api.requests.urls', namespace='requests')),
url(r'^requests/', include(('api.requests.urls', 'requests'), namespace='requests')),
url(r'^scopes/', include('api.scopes.urls', namespace='scopes')),
url(r'^search/', include('api.search.urls', namespace='search')),
url(r'^sparse/', include('api.sparse.urls', namespace='sparse')),
Expand All @@ -68,7 +68,7 @@
url(r'^users/', include('api.users.urls', namespace='users')),
url(r'^view_only_links/', include('api.view_only_links.urls', namespace='view-only-links')),
url(r'^wikis/', include('api.wikis.urls', namespace='wikis')),
url(r'^_waffle/', include('api.waffle.urls', namespace='waffle')),
url(r'^_waffle/', include(('api.waffle.urls', 'waffle'), namespace='waffle')),
],
),
),
Expand Down

0 comments on commit 312c347

Please sign in to comment.