Skip to content
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.

Sign up for GitHub

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

[Django Upgrade] [ENG-4072] The Rest - Part 2 #10072

Merged
merged 12 commits into from
Oct 10, 2022
Merged

[Django Upgrade] [ENG-4072] The Rest - Part 2 #10072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bfc5b8c
Fix missing or insufficient permission test for amdin preprints view
cslzchen Sep 29, 2022
b927676
Fix admin user view tests by using HTTP response headers object
cslzchen Sep 29, 2022
a61cd1b
Fix duplicate view_node permissions in node and instn view tests
cslzchen Sep 27, 2022
21bfede
Rework refresh_from_db() to reload GFKs
cslzchen Oct 3, 2022
ca45a2a
Fix visible contributor query
cslzchen Sep 30, 2022
0375a5a
Fix handle_archive_fail and its tests
cslzchen Sep 27, 2022
b3aa28d
Use a different schema to fix tests failure due to django3 upgrade
Sep 13, 2022
9b30044
Update URLs for URL Validation tests with Django 3
cslzchen Oct 3, 2022
75a99e2
Revert "Fix an issue where get_node() is not avaialbe from context view"
cslzchen Oct 4, 2022
b311d18
Improve to_internal_value() in NodeRelationshipField
cslzchen Oct 4, 2022
227ee3a
Add alt fixtures to prevent both siblings & parent/child conflicts
cslzchen Oct 4, 2022
16f7a3a
Fix linked_by_nodes in node related counts test
cslzchen Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions admin_tests/institutions/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@
from nose import tools as nt
from django.test import RequestFactory
from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import PermissionDenied


from tests.base import AdminTestCase
from osf_tests.factories import (
AuthUserFactory,
InstitutionFactory,
ProjectFactory
)
from osf.models import Institution, Node
from osf.models import Institution, Node, AbstractNode

from admin_tests.utilities import setup_form_view, setup_user_view

Expand Down Expand Up @@ -208,7 +208,10 @@ def setUp(self):
self.institution = InstitutionFactory()

self.user = AuthUserFactory()
self.view_node = Permission.objects.get(codename='view_node')
self.view_node = Permission.objects.filter(
codename='view_node',
content_type_id=ContentType.objects.get_for_model(AbstractNode).id
).first()
Comment on lines -211 to +214
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue of duplicate view_node permission is caused by the upgraded django added new default view_ permissions for node. Need to specify the content_type to point to the right one.

SELECT * FROM auth_permission WHERE codename='view_node';

+---+---------------------+---------------+---------+
|id |name                 |content_type_id|codename |
+---+---------------------+---------------+---------+
|73 |Can view node details|7              |view_node|
|515|Can view node        |14             |view_node|
+---+---------------------+---------------+---------+

self.user.user_permissions.add(self.view_node)
self.user.affiliated_institutions.add(self.institution)
self.user.save()
Expand Down
18 changes: 14 additions & 4 deletions admin_tests/nodes/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import datetime
import responses

from osf.models import AdminLogEntry, NodeLog
from osf.models import AdminLogEntry, NodeLog, AbstractNode
from admin.nodes.views import (
NodeDeleteView,
NodeRemoveContributorView,
Expand All @@ -29,6 +29,7 @@
from django.urls import reverse
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from framework.auth.core import Auth

from tests.base import AdminTestCase
Expand Down Expand Up @@ -97,7 +98,10 @@ def test_correct_view_permissions(self):
node = ProjectFactory()
guid = node._id

change_permission = Permission.objects.get(codename='view_node')
change_permission = Permission.objects.filter(
codename='view_node',
content_type_id=ContentType.objects.get_for_model(AbstractNode).id
).first()
user.user_permissions.add(change_permission)
user.save()

Expand Down Expand Up @@ -153,7 +157,10 @@ def test_correct_view_permissions(self):
guid = self.node._id

change_permission = Permission.objects.get(codename='delete_node')
view_permission = Permission.objects.get(codename='view_node')
view_permission = Permission.objects.filter(
codename='view_node',
content_type_id=ContentType.objects.get_for_model(AbstractNode).id
).first()
user.user_permissions.add(change_permission)
user.user_permissions.add(view_permission)
user.save()
Expand Down Expand Up @@ -226,7 +233,10 @@ def test_no_user_permissions_raises_error(self):

def test_correct_view_permissions(self):
change_permission = Permission.objects.get(codename='change_node')
view_permission = Permission.objects.get(codename='view_node')
view_permission = Permission.objects.filter(
codename='view_node',
content_type_id=ContentType.objects.get_for_model(AbstractNode).id
).first()
self.user.user_permissions.add(change_permission)
self.user.user_permissions.add(view_permission)
self.user.save()
Expand Down
10 changes: 5 additions & 5 deletions admin_tests/preprints/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ def test_get_object(self, req, preprint, plain_view):
def test_no_user_permissions_raises_error(self, user, preprint, plain_view):
request = RequestFactory().get(reverse('preprints:preprint', kwargs={'guid': preprint._id}))
request.user = user
resp = plain_view.as_view()(request, guid=preprint._id)
assert resp._headers['location'][1] == f'/accounts/login/?next=/preprints/{preprint._id}/'
with pytest.raises(PermissionDenied):
plain_view.as_view()(request, guid=preprint._id)
Comment on lines -106 to +107
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there is no permission or when permission is insufficient, django admin either redirect user to login page ( if the user is not authenticated) or raise the permission error (if authenticated). OSFUser set is_authenticated to True all the time and thus the correct behavior is raising exceptions.


def test_get_flagged_spam(self, superuser, preprint, ham_preprint, spam_preprint, flagged_preprint):
request = RequestFactory().get(reverse('preprints:flagged-spam'))
Expand Down Expand Up @@ -175,16 +175,16 @@ def test_confirm_ham(self, preprint, superuser, mock_akismet):
assert preprint.spam_status == SpamStatus.HAM
assert preprint.is_public

def test_correct_view_permissions(self, user, preprint, plain_view):
def test_valid_but_insufficient_view_permissions(self, user, preprint, plain_view):
view_permission = Permission.objects.get(codename='view_preprint')
user.user_permissions.add(view_permission)
user.save()

request = RequestFactory().get(reverse('preprints:preprint', kwargs={'guid': preprint._id}))
request.user = user

response = plain_view.as_view()(request, guid=preprint._id)
assert response.status_code == 302
with pytest.raises(PermissionDenied):
plain_view.as_view()(request, guid=preprint._id)
Comment on lines -178 to +187
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto, also renamed the test name to avoid confusion.


def test_change_preprint_provider(self, user, preprint, plain_view):
change_permission = Permission.objects.get(codename='change_preprint')
Expand Down
10 changes: 5 additions & 5 deletions admin_tests/users/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ def test_search_user_by_guid(self):
nt.assert_true(form.is_valid())
response = self.view.form_valid(form)
nt.assert_equal(response.status_code, 302)
nt.assert_equal(response._headers['location'][1], '/users/{}/'.format(self.user_1.guids.first()._id))
nt.assert_equal(response.headers['location'], '/users/{}/'.format(self.user_1.guids.first()._id))

def test_search_user_by_name(self):
form_data = {
Expand All @@ -424,7 +424,7 @@ def test_search_user_by_name(self):
nt.assert_true(form.is_valid())
response = self.view.form_valid(form)
nt.assert_equal(response.status_code, 302)
nt.assert_equal(response._headers['location'][1], '/users/search/Hardy/')
nt.assert_equal(response.headers['location'], '/users/search/Hardy/')

def test_search_user_by_name_with_punctuation(self):
form_data = {
Expand All @@ -434,7 +434,7 @@ def test_search_user_by_name_with_punctuation(self):
nt.assert_true(form.is_valid())
response = self.view.form_valid(form)
nt.assert_equal(response.status_code, 302)
nt.assert_equal(response._headers['location'][1], '/users/search/Dr.%20Sportello-Fay,%20PI%20@,%20%23,%20$,%20%25,%20%5E,%20&,%20*,%20(,%20),%20~/')
nt.assert_equal(response.headers['location'], '/users/search/Dr.%20Sportello-Fay,%20PI%20@,%20%23,%20$,%20%25,%20%5E,%20&,%20*,%20(,%20),%20~/')

def test_search_user_by_username(self):
form_data = {
Expand All @@ -444,7 +444,7 @@ def test_search_user_by_username(self):
nt.assert_true(form.is_valid())
response = self.view.form_valid(form)
nt.assert_equal(response.status_code, 302)
nt.assert_equal(response._headers['location'][1], '/users/{}/'.format(self.user_1.guids.first()._id))
nt.assert_equal(response.headers['location'], '/users/{}/'.format(self.user_1.guids.first()._id))

def test_search_user_by_alternate_email(self):
form_data = {
Expand All @@ -454,7 +454,7 @@ def test_search_user_by_alternate_email(self):
nt.assert_true(form.is_valid())
response = self.view.form_valid(form)
nt.assert_equal(response.status_code, 302)
nt.assert_equal(response._headers['location'][1], '/users/{}/'.format(self.user_2.guids.first()._id))
nt.assert_equal(response.headers['location'], '/users/{}/'.format(self.user_2.guids.first()._id))

def test_search_user_list(self):
view = views.UserSearchList()
Expand Down
7 changes: 6 additions & 1 deletion api/draft_registrations/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@
class NodeRelationshipField(RelationshipField):

def to_internal_value(self, node_id):
return {'branched_from': Node.load(node_id)}
context_view = self.context['view']
if hasattr(context_view, 'get_node'):
node = self.context['view'].get_node(node_id=node_id) if node_id else None
else:
node = Node.load(node_id)
return {'branched_from': node}


class DraftRegistrationSerializer(DraftRegistrationLegacySerializer, TaxonomizableSerializerMixin):
Expand Down
57 changes: 38 additions & 19 deletions api_tests/draft_registrations/views/test_draft_registration_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,13 @@ def test_cannot_view_draft_list(


class TestDraftRegistrationCreateWithNode(TestDraftRegistrationCreate):

# Overrides `url_draft_registrations` in `TestDraftRegistrationCreate`
@pytest.fixture()
def url_draft_registrations(self, project_public):
return '/{}draft_registrations/?'.format(API_BASE)

# Overrides `payload` in TestDraftRegistrationCreate`
@pytest.fixture()
def payload(self, metaschema_open_ended, provider, project_public):
return {
Expand Down Expand Up @@ -105,39 +108,55 @@ def payload(self, metaschema_open_ended, provider, project_public):
}
}

# Temporary alternative provider that supports `metaschema_open_ended` in `TestDraftRegistrationCreate`
# This provider is created to fix the first 3 tests in this test class due to test DB changes with the
# Django 3 Upgrade. A long-term solution is to create and/or use dedicated schemas for testing.
@pytest.fixture()
def provider_alt(self, metaschema_open_ended):
default_provider = RegistrationProvider.get_default()
default_provider.schemas.add(metaschema_open_ended)
default_provider.save()
return default_provider
Comment on lines +111 to +119
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestDraftRegistrationCreateWithNode inherits from TestDraftRegistrationCreate. Thus modifying the provider in the latter breaks TestDraftRegistrationCreate. In addition, some tests in TestDraftRegistrationCreateWithNode also relied on this version of metaschema_open_ended not in provider.

This issue is caused by our factories relies on test DB (i.e. adding the pk=1 schema when calling RegistrationFactory) instead of adding a schema with specified name and pinned version. Pre and post upgrade test DB are slightly different which breaks the tests.


# Similarly, this is a temporary alternative payload that uses the above `provider_alt`.
@pytest.fixture()
def payload_alt(self, payload, provider_alt):
new_payload = payload.copy()
new_payload['data']['relationships']['provider']['data']['id'] = provider_alt._id
return new_payload

# Overrides TestDraftRegistrationList
def test_cannot_create_draft_errors(
self, app, user, project_public, payload, url_draft_registrations):
def test_cannot_create_draft_errors(self, app, user, payload_alt, project_public, url_draft_registrations):
# test_cannot_create_draft_from_a_registration
registration = RegistrationFactory(
project=project_public, creator=user)
payload['data']['relationships']['branched_from']['data']['id'] = registration._id
payload_alt['data']['relationships']['branched_from']['data']['id'] = registration._id
res = app.post_json_api(
url_draft_registrations, payload, auth=user.auth,
url_draft_registrations, payload_alt, auth=user.auth,
expect_errors=True)
assert res.status_code == 404

# test_cannot_create_draft_from_deleted_node
project = ProjectFactory(is_public=True, creator=user)
project.is_deleted = True
project.save()
payload['data']['relationships']['branched_from']['data']['id'] = project._id
payload_alt['data']['relationships']['branched_from']['data']['id'] = project._id
res = app.post_json_api(
url_draft_registrations, payload,
url_draft_registrations, payload_alt,
auth=user.auth, expect_errors=True)
assert res.status_code == 410
assert res.json['errors'][0]['detail'] == 'The requested node is no longer available.'

# test_cannot_create_draft_from_collection
collection = CollectionFactory(creator=user)
payload['data']['relationships']['branched_from']['data']['id'] = collection._id
payload_alt['data']['relationships']['branched_from']['data']['id'] = collection._id
res = app.post_json_api(
url_draft_registrations, payload, auth=user.auth,
url_draft_registrations, payload_alt, auth=user.auth,
expect_errors=True)
assert res.status_code == 404

def test_draft_registration_attributes_copied_from_node(self, app, project_public,
url_draft_registrations, user, payload):
url_draft_registrations, user, payload_alt):

write_contrib = AuthUserFactory()
read_contrib = AuthUserFactory()
Expand All @@ -156,12 +175,12 @@ def test_draft_registration_attributes_copied_from_node(self, app, project_publi
project_public.add_contributor(write_contrib, WRITE)
project_public.add_contributor(read_contrib, READ)

res = app.post_json_api(url_draft_registrations, payload, auth=write_contrib.auth, expect_errors=True)
res = app.post_json_api(url_draft_registrations, payload_alt, auth=write_contrib.auth, expect_errors=True)
assert res.status_code == 201
res = app.post_json_api(url_draft_registrations, payload, auth=read_contrib.auth, expect_errors=True)
res = app.post_json_api(url_draft_registrations, payload_alt, auth=read_contrib.auth, expect_errors=True)
assert res.status_code == 403

res = app.post_json_api(url_draft_registrations, payload, auth=user.auth)
res = app.post_json_api(url_draft_registrations, payload_alt, auth=user.auth)
assert res.status_code == 201
attributes = res.json['data']['attributes']
assert attributes['title'] == project_public.title
Expand All @@ -180,14 +199,14 @@ def test_draft_registration_attributes_copied_from_node(self, app, project_publi
def test_cannot_create_draft(
self, app, user_write_contrib,
user_read_contrib, user_non_contrib,
project_public, payload, group,
project_public, payload_alt, group,
url_draft_registrations, group_mem):

# test_write_only_contributor_cannot_create_draft
assert user_write_contrib in project_public.contributors.all()
res = app.post_json_api(
url_draft_registrations,
payload,
payload_alt,
auth=user_write_contrib.auth,
expect_errors=True)
assert res.status_code == 201
Expand All @@ -196,29 +215,29 @@ def test_cannot_create_draft(
assert user_read_contrib in project_public.contributors.all()
res = app.post_json_api(
url_draft_registrations,
payload,
payload_alt,
auth=user_read_contrib.auth,
expect_errors=True)
assert res.status_code == 403

# test_non_authenticated_user_cannot_create_draft
res = app.post_json_api(
url_draft_registrations,
payload, expect_errors=True)
payload_alt, expect_errors=True)
assert res.status_code == 401

# test_logged_in_non_contributor_cannot_create_draft
res = app.post_json_api(
url_draft_registrations,
payload,
payload_alt,
auth=user_non_contrib.auth,
expect_errors=True)
assert res.status_code == 403

# test_group_admin_cannot_create_draft
res = app.post_json_api(
url_draft_registrations,
payload,
payload_alt,
auth=group_mem.auth,
expect_errors=True)
assert res.status_code == 201
Expand All @@ -228,7 +247,7 @@ def test_cannot_create_draft(
project_public.add_osf_group(group, WRITE)
res = app.post_json_api(
url_draft_registrations,
payload,
payload_alt,
auth=group_mem.auth,
expect_errors=True)
assert res.status_code == 201
Expand Down
12 changes: 8 additions & 4 deletions api_tests/nodes/views/test_node_detail.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
# -*- coding: utf-8 -*-
import mock
import pytest
from django.utils import timezone
from future.moves.urllib.parse import urlparse
import mock
from nose.tools import * # noqa:

import pytest
from rest_framework import exceptions

from addons.wiki.tests.factories import WikiFactory, WikiVersionFactory
from api.base.settings.defaults import API_BASE
Expand Down Expand Up @@ -33,7 +34,6 @@
WithdrawnRegistrationFactory,
DraftNodeFactory,
)
from rest_framework import exceptions
from tests.base import fake
from tests.utils import assert_latest_log, assert_latest_log_not
from website.views import find_bookmark_collection
Expand Down Expand Up @@ -480,8 +480,12 @@ def test_node_shows_related_count_for_linked_by_relationships(self, app, user, p
assert res.json['data']['relationships']['linked_by_nodes']['links']['related']['meta']['count'] == 1
assert res.json['data']['relationships']['linked_by_registrations']['links']['related']['meta']['count'] == 1

log_date = timezone.now()
project_private.deleted_date = log_date
project_private.deleted = log_date
project_private.is_deleted = True
Comment on lines +483 to 486
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: somehow, when I tried project_private.remove_node(auth=Auth(user=user)), the node is not deleted though the method went through the process and returned True.

project_private.save()
registration.reload()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must reload the registration, otherwise, registration.registered_from has the project_private node as active instead of deleted. If not, when WithdrawnRegistrationFactory is called using this registration, it old project_private overrides the deleted project_private and breaks the next assertion on linked_by_nodes

project_public.reload()

res = app.get(url)
Expand Down