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

cslzchen · 2022-09-23T18:26:54Z

Purpose

See: https://www.notion.so/cos/The-rest-of-the-fixes-ec6b2b2f96e74a2b902a1d57d2a8fe6a

Tickets & PRs

Ticket: https://openscience.atlassian.net/browse/ENG-4072
Reference PR with initial attempts of fix: [Django Upgrade] [Reference PR] The rest of the fixes #10048
New Reference PR with following attempts + all tests passing: [Django Upgrade] Another Reference PR #10074
Part 1 of this PR: [Django Upgrade] [ENG-4072] The Rest - Part 1 #10070

Changes

Fix linked_by_nodes in node related counts test
Add alt fixtures to prevent both siblings & parent/child conflicts
Improve to_internal_value() in NodeRelationshipField
Revert "Fix an issue where get_node() is not availalbe from context view"
Update URLs for URL Validation tests with Django 3
Use a different schema to fix tests failure due to django3 upgrade
Fix handle_archive_fail and its tests
Fix visible contributor query
Rework refresh_from_db() to reload GFKs
Fix duplicate view_node permissions in node and instn view tests
Fix admin user view tests by using HTTP response headers object
Fix missing or insufficient permission test for admin preprints view

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

osf/models/user.py

Co-authored-by: Jon Walz <jonmwalz@gmail.com>

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

This reverts commit f109ffc.

This fixes a bug that is revealed by django3 upgrade: the object returned from `self.context['view']` sometime doesn't have the `.get_node()` method.

cslzchen

Finally, all CI tests passing 🎆

cslzchen · 2022-10-05T17:45:23Z

admin_tests/preprints/test_views.py

-        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)


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.

cslzchen · 2022-10-05T17:46:00Z

admin_tests/preprints/test_views.py

-    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)


Ditto, also renamed the test name to avoid confusion.

cslzchen · 2022-10-05T17:49:56Z

admin_tests/institutions/test_views.py

-        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()


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| +---+---------------------+---------------+---------+

cslzchen · 2022-10-05T17:52:32Z

osf/models/base.py

-        # Django's refresh_from_db does not uncache GFKs
-        for field in self._meta.private_fields:
-            if hasattr(field, 'cache_attr') and field.cache_attr in self.__dict__:
-                del self.__dict__[field.cache_attr]


private_fields is not GFK though GFK seems to be in private_fields

Since Django 2.2, any cached relations are cleared from the reloaded instance

cslzchen · 2022-10-05T17:53:23Z

osf/models/base.py

+        # behavior, our customization only reloads GFKs.
+        for f in self._meta._get_fields(reverse=False):
+            # Note: the following `if` condition is how django internally identifies GFK
+            if f.is_relation and f.many_to_one and not (hasattr(f.remote_field, 'model') and f.remote_field.model):


Only refresh GFK, which is many_to_one and thus won't cause infinite loop.

cslzchen · 2022-10-05T18:08:11Z

website/static/urlValidatorTest.json

-    "http://-.~_!$&()*+,;=:%40:80%2f::::::@example.com":"should accept valid website with user but crazy username",
+    "http://-.~_!%24%26()*%2B%2C%3B%3D%3A%40%3A80%2F%3A%3A%3A%3A%3A%3A@example.com":"should accept valid website with user but crazy username",
    "http://1337.net":"should accept valid website with just numbers in domain",
    "http://definitelyawebsite.com?real=yes&page=definitely":"should accept valid website with query",
    "http://a.b-c.de":"should accept valid website with dash",
    "http://website.com:3000/?q=400":"should accept valid website with port and query",
-    "http://asd/asd@asd.com/":"should accept valid website with unusual username"
+    "http://asd%2Fasd@asd.com/":"should accept valid website with unusual username"


Django 3 is more strict on special characters encoding in URL and we should follow the spec.

cslzchen · 2022-10-05T18:29:43Z

api/draft_registrations/serializers.py

-        node = self.context['view'].get_node(node_id=node_id) if node_id else None
+        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)


After the upgrade, one test fails due to self.context['view'] returning an object not having .get_node. There is one concern of this fix since .get_node() checks permission and other stuff before loading the node. Might worth another look at the failed test.

cslzchen · 2022-10-05T18:37:57Z

api_tests/draft_registrations/views/test_draft_registration_list.py

+    # 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


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.

cslzchen · 2022-10-05T18:40:07Z

api_tests/nodes/views/test_node_detail.py

+        log_date = timezone.now()
+        project_private.deleted_date = log_date
+        project_private.deleted = log_date
        project_private.is_deleted = True


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.

cslzchen · 2022-10-05T18:42:26Z

api_tests/nodes/views/test_node_detail.py

        project_private.is_deleted = True
        project_private.save()
+        registration.reload()


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

cslzchen mentioned this pull request Sep 23, 2022

[Django Upgrade] [ENG-4072] The Rest - Part 1 #10070

Merged

cslzchen changed the title ~~[Django Upgrade] The Rest - Part 2~~ [Django Upgrade] [ENG-4072] The Rest - Part 2 Sep 23, 2022

cslzchen force-pushed the the-rest-of-the-fixes branch 3 times, most recently from 9af8993 to 5d9769a Compare September 28, 2022 17:01

cslzchen and others added 4 commits September 30, 2022 12:53

Fix missing or insufficient permission test for amdin preprints view

bfc5b8c

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

Fix admin user view tests by using HTTP response headers object

b927676

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

Fix duplicate view_node permissions in node and instn view tests

a61cd1b

Rework refresh_from_db() to reload GFKs

21bfede

cslzchen force-pushed the the-rest-of-the-fixes branch from d43ca7f to f47df68 Compare October 3, 2022 06:53

cslzchen marked this pull request as ready for review October 3, 2022 07:07

cslzchen commented Oct 3, 2022

View reviewed changes

osf/models/user.py Outdated Show resolved Hide resolved

cslzchen and others added 7 commits October 4, 2022 13:03

Fix visible contributor query

ca45a2a

Co-authored-by: Jon Walz <jonmwalz@gmail.com>

Fix handle_archive_fail and its tests

0375a5a

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

Use a different schema to fix tests failure due to django3 upgrade

b3aa28d

Co-authored-by: John Tordoff <johnetordoff@gmail.com>

Update URLs for URL Validation tests with Django 3

9b30044

Revert "Fix an issue where get_node() is not avaialbe from context view"

75a99e2

This reverts commit f109ffc.

Improve to_internal_value() in NodeRelationshipField

b311d18

This fixes a bug that is revealed by django3 upgrade: the object returned from `self.context['view']` sometime doesn't have the `.get_node()` method.

Add alt fixtures to prevent both siblings & parent/child conflicts

227ee3a

cslzchen force-pushed the the-rest-of-the-fixes branch from 339099a to 227ee3a Compare October 4, 2022 17:05

Fix linked_by_nodes in node related counts test

16f7a3a

cslzchen mentioned this pull request Oct 5, 2022

[Django Upgrade] [Reference PR] The rest of the fixes #10048

Closed

cslzchen commented Oct 5, 2022

View reviewed changes

cslzchen mentioned this pull request Oct 5, 2022

[Django Upgrade] Another Reference PR #10074

Closed

cslzchen merged commit c762700 into CenterForOpenScience:feature/django_upgrade Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cslzchen commented Sep 23, 2022 •

edited

cslzchen left a comment •

edited

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

cslzchen Oct 5, 2022

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

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

Conversation

cslzchen commented Sep 23, 2022 • edited

Purpose

Tickets & PRs

Changes

cslzchen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen commented Sep 23, 2022 •

edited

cslzchen left a comment •

edited