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

Johnetordoff · 2022-08-30T23:13:44Z

Purpose

These are the final test changes and misc fixes for the Django upgrade.

Changes

fix header changes to the response object.
remove unused/broken update_version_metadata tests
add DEFAULT_AUTO_FIELD to silience depreciation warnings.
change test_node_shows_related_count_for_linked_by_relationships to match new behavior
api_tests/nodes/views/test_node_draft_registration_detail.py to use real registration schema
fixes provider__reviews_workflow keyword agruement provide subfactory problem with PreprintFactory
removes 'view_node', to fix admin tests.
turn notification email off for tests.
reload referents for cached properties
ensure creator auth is passed in RegistrationFactory

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

…ce#9964) Co-authored-by: John Tordoff <>

Use management command in post-migrate signal to create cache table instead of in Raw SQL in migrations Co-authored-by: John Tordoff <>

* Create Waffle Flags and Switches as post_migrate_signal (with configurable values) Co-authored-by: John Tordoff <>

…ience#9961) * remove licenses from migration stream and only use post-migrate signal Co-authored-by: John Tordoff <>

…ate signals (CenterForOpenScience#9974) * Set noop to all ensure_schema migrations * Set noop to shema and shema block updates * Add ensuring schema and updating schema blocks to post-migrate signals * Set noop to one-time data migrations, which include * Preprint permissions * Schema to schema blocks * Draft node permissions * Remove manual post-migration signals from PND * Move generate_object_id into migration utils * Replace duplicate function with inline imports

…orOpenScience#9958) * Fix signal and clean-up * don't use bulk create until ignore_conflicts exsists * allow BLACKLISTED_DOMAINS to be seamlessly overridden Co-authored-by: John Tordoff <>

…nterForOpenScience#9966) * Move loading of citation styles to post-migration signal * Peg repo to commit SHA * Attempt to speed things up by loading fixture * Move update_citation_styles to management command * Remove old test like thing * Clean up URL setting and docstring * Remove install bower from CI setup * Reorder imports * Reintroduce tests, but test skip them * Final clean-up Co-authored-by: John Tordoff <> Co-authored-by: Longze Chen <cslzchen@gmail.com>

…OpenScience#9965) * Remove storage region configuration from the migration stream and add to config file * Only ensure default provider and remove yaml config * Remove default storage region backfill * Region gets or creates, but not updates * Use _id and name as positional arguments and fix osf_settings Co-authored-by: John Tordoff <> Co-authored-by: Longze Chen <cslzchen@gmail.com>

…e#9993)

…lt providers (CenterForOpenScience#9971) * Remove default provider population from migration to post-migrate signal * Add conditional from local env * Load with get_default and refactor * Fix rebase conflicts: settings -> osf_settings * Remove subjects population Co-authored-by: John Tordoff <> Co-authored-by: Longze Chen <cslzchen@gmail.com>

Co-authored-by: John Tordoff <>

…rOpenScience#9983) Co-authored-by: John Tordoff <>

…erForOpenScience#10011) Co-authored-by: John Tordoff <>

…ence#9994) * Move datacite schema population out of migration stream * Move pytest fixuture data populator out of test class Co-authored-by: John Tordoff <> Co-authored-by: Longze Chen <cslzchen@gmail.com>

…pgrade

…e#10012) Co-authored-by: John Tordoff <>

…cience#10021) Co-authored-by: John Tordoff <>

…10010) Co-authored-by: John Tordoff <>

…ce#9984) Co-authored-by: John Tordoff <>

…ixtures (CenterForOpenScience#10029)

…lean_squash [Django Upgrade] [ENG-3998] Migration Clean squash + Update `RunSQL` Indexes

…pgrade

…OpenScience/osf.io into django-3-onmibus * 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io: Fix admin login failure for django upgrade # Conflicts: # api/base/authentication/backends.py

cslzchen · 2022-09-22T17:34:12Z

osf/migrations/0001_initial.py

@@ -118,7 +118,7 @@ class Migration(migrations.Migration):
                ('description', models.TextField(blank=True, default='')),
                ('category', models.CharField(blank=True, choices=[('analysis', 'Analysis'), ('communication', 'Communication'), ('data', 'Data'), ('hypothesis', 'Hypothesis'), ('instrumentation', 'Instrumentation'), ('methods and measures', 'Methods and Measures'), ('procedure', 'Procedure'), ('project', 'Project'), ('software', 'Software'), ('other', 'Other'), ('', 'Uncategorized')], default='', max_length=255)),
                ('registration_responses', osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONField(blank=True, default=dict, encoder=osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONEncoder)),
-                ('registration_responses_migrated', models.NullBooleanField(db_index=True, default=True)),


Wondering if we should make a new migration rather than modifying the init migration?

Yes, I think that's necessary for prod/staging.

cslzchen · 2022-09-22T17:42:45Z

osf_tests/factories.py

+    password = factory.PostGenerationMethodCall(
+        'set_password',
+        'queenfan86',
+        notify=False
+    )


Does this change fix any test failures? If so, what is/are it/they?

There's probably a few more examples, but here's some of the fails:

osf_tests/test_institution.py:139: in test_send_deactivation_email_call_count assert mock_send_mail.call_count == 2 E AssertionError: assert 4 == 2 E + where 4 = <MagicMock name='send_mail' id='139986975588704'>.call_count ------------------------------ Captured log call ------------------------------- INFO osf.models.institution:institution.py:222 Institution deactivation notification email has been sent to [2/2] users for [6320aa24405bf3176806b47c] _ TestOSFGroup.test_notify_group_member_email_does_not_send_before_throttle_expires _ osf_tests/test_osfgroup.py:224: in test_notify_group_member_email_does_not_send_before_throttle_expires assert mock_send_mail.call_count == 1 E AssertionError: assert 2 == 1 E + where 2 = <MagicMock name='send_mail' id='139986965797744'>.call_count ___ TestOSFGroup.test_notify_group_member_email_sends_after_throttle_expires ___ osf_tests/test_osfgroup.py:240: in test_notify_group_member_email_sends_after_throttle_expires assert mock_send_mail.call_count == 1 E AssertionError: assert 2 == 1 E + where 2 = <MagicMock name='send_mail' id='139986963925536'>.call_count _________ TestOSFGroup.test_notify_group_unregistered_member_throttle __________ osf_tests/test_osfgroup.py:258: in test_notify_group_unregistered_member_throttle assert mock_send_mail.call_count == 1 E AssertionError: assert 2 == 1 E + where 2 = <MagicMock name='send_mail' id='139986964755176'>.call_count ________________ TestUser.test_visible_contributor_to_property _________________ osf_tests/test_user.py:1826: in test_visible_contributor_to_property assert project_to_be_invisible_on._id not in visible_contributor_to_nodes E AssertionError: assert 'jhkpf' not in ['dm69n', 'jhkpf'] E + where 'jhkpf' = (title='Mandatory user-facing encoding', category='project') with guid 'jhkpf'._id ________________ TestUserMerging.test_merge_doesnt_send_signal _________________ osf_tests/test_user.py:2103: in test_merge_doesnt_send_signal assert mock_notify.called is False E AssertionError: assert True is False E + where True = <MagicMock name='send_mail' id='139986980667744'>.called ``

osf_tests/factories.py

cslzchen · 2022-09-23T01:00:00Z

api_tests/nodes/views/test_node_draft_registration_detail.py

-        return RegistrationSchema.objects.get(
-            name='OSF Preregistration',
-            schema_version=SCHEMA_VERSION)
+        schema = RegistrationSchema.objects.get_latest_version(name='OSF Preregistration')
+        schema.active = True
+        schema.save()
+        return schema


The old tests use a real schema too, but just a different version. I am not sure how this fixes any broken tests.

Your update uses the latest schema. One issue is that if we ever have a new version of that schema, the test will break. However, we can update the tests then.

I will leave this commit out for now and see what it breaks.

Update: here are the two tests that using the new schema fixes.

__ TestDraftRegistrationUpdateWithNode.test_cannot_update_registration_schema __ api_tests/nodes/views/test_node_draft_registration_detail.py:408: in test_cannot_update_registration_schema assert res.status_code == 200 E assert 400 == 200 E + where 400 = <400 Bad Request application/vnd.api+json body=b'{"errors...0"}}'/163>.status_code _ TestDraftRegistrationUpdateWithDraftNode.test_cannot_update_registration_schema _ api_tests/nodes/views/test_node_draft_registration_detail.py:408: in test_cannot_update_registration_schema assert res.status_code == 200 E assert 400 == 200 E + where 400 = <400 Bad Request application/vnd.api+json body=b'{"errors...0"}}'/163>.status_code

However, there are other places in this test that still use the old schema:

osf.io/api_tests/nodes/views/test_node_draft_registration_detail.py

Lines 546 to 550 in cce3dd3

@pytest.fixture()

def reg_schema(self):

return RegistrationSchema.objects.get(

name='OSF Preregistration',

schema_version=SCHEMA_VERSION)

, which might indicate that the old schema is OK but something else could be wrong.

api/addons/urls.py

cslzchen · 2022-09-27T16:16:41Z

osf_tests/test_archiver.py

@@ -758,6 +758,7 @@ def test_handle_archive_fail(self, mock_send_mail):
            {}
        )
        assert_equal(mock_send_mail.call_count, 2)
+        self.dst.refresh_from_db()


Comment: we use reload() and refresh_from_db() interchangeably, though they are the same, see here.

cslzchen · 2022-09-27T16:35:09Z

website/archiver/utils.py

-    dst.root.sanction.forcibly_reject()
-    dst.root.sanction.save()
+    if dst.root.sanction:
+        dst.root.sanction.forcibly_reject()
+        dst.root.sanction.save()


Looking at the RegistrationFactory and ArchiverTestCase, sanction object passed into shouldn't be None.

Here are the failures that probably led to this fix.

__________________ TestArchiverUtils.test_handle_archive_fail __________________ osf_tests/test_archiver.py:758: in test_handle_archive_fail {} website/archiver/utils.py:114: in handle_archive_fail dst.root.sanction.forcibly_reject() E AttributeError: 'NoneType' object has no attribute 'forcibly_reject' _______________ TestArchiverUtils.test_handle_archive_fail_copy ________________ osf_tests/test_archiver.py:771: in test_handle_archive_fail_copy {} website/archiver/utils.py:114: in handle_archive_fail dst.root.sanction.forcibly_reject() E AttributeError: 'NoneType' object has no attribute 'forcibly_reject' _______________ TestArchiverUtils.test_handle_archive_fail_size ________________ osf_tests/test_archiver.py:803: in test_handle_archive_fail_size {} website/archiver/utils.py:114: in handle_archive_fail dst.root.sanction.forcibly_reject() E AttributeError: 'NoneType' object has no attribute 'forcibly_reject'

However, I think the correct fix should be on the RegistrationFactory instead of the actual code. Otherwise, the two lines inside the if condition is never tested.

dst.root.sanction.forcibly_reject() dst.root.sanction.save()

cslzchen · 2022-09-27T16:44:52Z

website/project/views/comment.py

@@ -49,6 +49,7 @@ def update_file_guid_referent(self, target, event_type, payload, user=None):
            old_file = BaseFileNode.load(obj.referent._id)
            obj.referent = create_new_file(obj, source, destination, destination_node)
            obj.save()
+            obj.referent.reload()


hmm, why do we need to reload the referent object when we are not using it at all afterwards?

Update: as expected, I verified that we don't need this .reload() to pass tests

cslzchen · 2022-09-27T18:01:41Z

osf_tests/test_comment.py

-        self.guid.reload()
+        self.guid.referent.reload()


This fix makes sense since Django refresh_from_db() (called by reload()) doesn't update relationship field and in this case a GFK.

My concern is, given that we use refresh_from_db() or reload() a lot in the actual code, this behavior change due to the upgrade may led to similar problems not covered by the tests.

Behavior of 1.11: doc, code
Behavior of 3.2] doc, code

In addition, our customization of the 1.11 refresh_from_db() clears cache only, which seems to have been added in 2.2

Any cached relations are cleared from the reloaded instance.

More Update:

We were using private_fields as GFK which turns out to be wrong. Although GFKs are returned in private_fields but the correct way to check GFK should be

f.is_relation and f.many_to_one and not (hasattr(f.remote_field, 'model') and f.remote_field.model)

which, can be found in private APIs from the source code of django.db.models.options.

And refresh_from_db() only refreshes non-deferred concrete_fields when no field are provided.

An issue with refreshing related fields is that it can cause infinite loop, as confirmed in local shell.
Another issue is when relation is many-to-many or one-to-many, things become more complicated.
This is probably why django doesn't do it but recommend reload the relate field(s) explicitly when necessary.
The best we can do is to refresh only one-to-one or many-to-ones.
As for this case, let's reload GFK's only.

cslzchen · 2022-09-27T22:03:44Z

osf/models/user.py

@@ -576,7 +576,7 @@ def visible_contributor_to(self):
        """
        Nodes where user is a bibliographic contributor (group membership not factored in)
        """
-        return self.nodes.filter(is_deleted=False, contributor__visible=True, type__in=['osf.node', 'osf.registration'])
+        return self.nodes.filter(is_deleted=False, contributor__visible=True, contributor__user=self, type__in=['osf.node', 'osf.registration'])


The fix make sense since we need to make sure that the invisible contributor on the project must be the self user.

Without it, visible_contributor_to returns two projects (in unit tests) with the following contributor set for each.

<QuerySet [<Contributor(user=freddiemercury+8522846@cos.io, visible=True, permission=admin)>]> <QuerySet [<Contributor(user=freddiemercury+8822275@cos.io, visible=True, permission=admin)>, <Contributor(user=freddiemercury+8522846@cos.io, visible=False, permission=write)>]>

Apparently, if we only have contributor__visible=True, the query find nodes as long as one of the contributor is visible. I am not sure why this happens after the upgrade, might be an issue with the related manager.

In [7]: user.nodes Out[7]: <django.db.models.fields.related_descriptors.create_forward_many_to_many_manager.<locals>.ManyRelatedManager at 0x7fcbb4a19898>

Update/Example:

We have user 7773344 and 8079719; we have Project 9qnbu and x8jc3
Project 9qnbu has contributor 7773344 as visible
Project x8jc3 has contributor 7773344 & 8079719 as visible

return self.nodes.filter(is_deleted=False, contributor__visible=True, type__in=['osf.node', 'osf.registration'])

The above returns 3 items instead of 2. Apparently, contributor__visible=True is looped on each contributor.

<AbstractNodeQuerySet [ (title='Visionary radical matrix', category='project') with guid 'x8jc3', (title='Adaptive modular access', category='project') with guid '9qnbu', (title='Adaptive modular access', category='project') with guid '9qnbu' ]>

My concern is, even with contributor__user=self, it still loops each contributor on every project ... this makes sense why adding it fix the tests but it might be too costly?

cslzchen · 2022-09-29T00:07:02Z

admin_tests/preprints/test_views.py

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


hmm, this looks like something we need to fix on the permission side instead of the test itself

might be caused by DRF, where permission check now require all permissions instead of any

>> view_permission = Permission.objects.get(codename='view_preprint') >> user.user_permissions.add(view_permission) >> user.save() >> permission_required = ('osf.view_preprint',) >> user.has_perms(permission_required) >> True >> permission_required = ('osf.view_preprint', 'osf.change_preprint',) >> user.has_perms(permission_required) >> False

This is why test_change_preprint_provider passes while test_correct_view_permissions fail

Final thoughts: I agree with @john Tordoff that this is more on the test than the code. The behavior of user.check_perms is the same between 1.11 and 3.2. In addition, the redirection behavior when permission is denied depends on if user.is_authenticated is True. With it always being set to True, PermissionDenied should be raised when there no permission or when permission is incorrect. One of the failed test should be renamed from test_correct_view_permissions to test_incorrect_view_permissions to avoid confusion.

cslzchen · 2022-10-05T17:28:24Z

Refer to #10070,, #10072 and https://www.notion.so/cos/The-rest-of-the-fixes-ec6b2b2f96e74a2b902a1d57d2a8fe6a for all the final fixes. PR closed.

Johnetordoff and others added 30 commits July 27, 2022 15:17

remove post-migrate signals from migration stream (CenterForOpenScien…

7c3b9ad

…ce#9964) Co-authored-by: John Tordoff <>

move createcachetable to post migrate signal (CenterForOpenScience#9944)

4a8bd61

Use management command in post-migrate signal to create cache table instead of in Raw SQL in migrations Co-authored-by: John Tordoff <>

[ENG-3836] Investigate waffle flags (CenterForOpenScience#9950)

bf7eb21

* Create Waffle Flags and Switches as post_migrate_signal (with configurable values) Co-authored-by: John Tordoff <>

[ENG-3865] Create Licenses using post-migrate signal (CenterForOpenSc…

15f15e9

…ience#9961) * remove licenses from migration stream and only use post-migrate signal Co-authored-by: John Tordoff <>

[ENG-3868] Move blocked email domains to post-migrate signal (CenterF…

c2c5e4e

…orOpenScience#9958) * Fix signal and clean-up * don't use bulk create until ignore_conflicts exsists * allow BLACKLISTED_DOMAINS to be seamlessly overridden Co-authored-by: John Tordoff <>

[ENG-3836] Follow-up: Add PyYAML to Requirements (CenterForOpenScienc…

8052ea4

…e#9993)

Remove unused password change view/route (CenterForOpenScience#9985)

205e560

Co-authored-by: John Tordoff <>

Update import paths and fix deprecated modules for Django 3 (CenterFo…

1675f6f

…rOpenScience#9983) Co-authored-by: John Tordoff <>

Update postgres backend and remove failover router for Django 3 (Cent…

391af60

…erForOpenScience#10011) Co-authored-by: John Tordoff <>

Move file metadata population to unit test fixtures (CenterForOpenSci…

c9c7f44

…ence#9994) * Move datacite schema population out of migration stream * Move pytest fixuture data populator out of test class Co-authored-by: John Tordoff <> Co-authored-by: Longze Chen <cslzchen@gmail.com>

Remove password_reset from installed apps

721ef90

Merge remote-tracking branch 'upstream/develop' into feature/django_u…

75ffbb0

…pgrade

[Django Upgrade] Fix one sub-query slicing issue (CenterForOpenScienc…

285d9a6

…e#10012) Co-authored-by: John Tordoff <>

[Django Upgrade] Upgrade django-elasticsearch-metrics (CenterForOpenS…

06f6fe9

…cience#10021) Co-authored-by: John Tordoff <>

[Django Upgrade] Replace M2M direct assignment (CenterForOpenScience#…

724f59a

…10010) Co-authored-by: John Tordoff <>

[Django Upgrade] Upgrade markdown and mdx_del_ins (CenterForOpenScien…

afa62a7

…ce#9984) Co-authored-by: John Tordoff <>

[Django Upgrade] Move schema activeness/visibility update to pytest f…

d15c88f

…ixtures (CenterForOpenScience#10029)

Remove osf migrations

3f162f1

Remove addon migrations

f971149

Init migrations - osf

f41eb8a

Init migrations - addons

dab1bbc

Fix migration for NotableEmailDomain

994fcfd

Fix circular import with built-in admin migrations

a54bbbe

Add a RunSQL migration to update indexes

699fc1c

Merge pull request CenterForOpenScience#10028 from cslzchen/feature/c…

0dc9220

…lean_squash [Django Upgrade] [ENG-3998] Migration Clean squash + Update `RunSQL` Indexes

Merge remote-tracking branch 'upstream/develop' into feature/django_u…

cfb012e

…pgrade

Johnetordoff force-pushed the django-3-onmibus branch 3 times, most recently from cdda6e5 to 7b581ee Compare September 19, 2022 14:17

upgrade to end dependabot warnings

b58c17f

Johnetordoff force-pushed the django-3-onmibus branch from 014911f to b58c17f Compare September 19, 2022 14:22

Johnetordoff force-pushed the django-3-onmibus branch 4 times, most recently from b266ec8 to b5e989d Compare September 19, 2022 15:30

more dependabot updates

3f6a665

Johnetordoff force-pushed the django-3-onmibus branch from b5e989d to 3f6a665 Compare September 19, 2022 18:25

cslzchen changed the title ~~Django 3 The rest of the fixes~~ [Django Upgrade] The rest of the fixes Sep 22, 2022

cslzchen reviewed Sep 22, 2022

View reviewed changes

osf_tests/factories.py Show resolved Hide resolved

cslzchen force-pushed the feature/django_upgrade branch from 93081d4 to fc895e8 Compare September 22, 2022 18:48

cslzchen changed the title ~~[Django Upgrade] The rest of the fixes~~ [Django Upgrade] [Reference PR] The rest of the fixes Sep 22, 2022

cslzchen reviewed Sep 23, 2022

View reviewed changes

api/addons/urls.py Show resolved Hide resolved

This was referenced Sep 23, 2022

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

Merged

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

Merged

cslzchen reviewed Sep 27, 2022

View reviewed changes

cslzchen reviewed Sep 29, 2022

View reviewed changes

cslzchen closed this Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Johnetordoff commented Aug 30, 2022 •

edited

cslzchen Sep 22, 2022

Johnetordoff Sep 22, 2022

cslzchen Sep 22, 2022

Johnetordoff Sep 22, 2022

cslzchen Sep 23, 2022

cslzchen Sep 28, 2022

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022

cslzchen Oct 3, 2022

cslzchen Sep 27, 2022

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 29, 2022

cslzchen Sep 29, 2022 •

edited

cslzchen Sep 30, 2022 •

edited

cslzchen commented Oct 5, 2022 •

edited

	@pytest.fixture()
	def reg_schema(self):
	return RegistrationSchema.objects.get(
	name='OSF Preregistration',
	schema_version=SCHEMA_VERSION)

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

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

Conversation

Johnetordoff commented Aug 30, 2022 • edited

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

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 Sep 27, 2022 • edited

Choose a reason for hiding this comment

cslzchen Sep 27, 2022 • edited

Choose a reason for hiding this comment

cslzchen Sep 27, 2022 • 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

cslzchen Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen Sep 29, 2022 • edited

Choose a reason for hiding this comment

cslzchen Sep 30, 2022 • edited

Choose a reason for hiding this comment

cslzchen commented Oct 5, 2022 • edited

Johnetordoff commented Aug 30, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 27, 2022 •

edited

cslzchen Sep 29, 2022 •

edited

cslzchen Sep 30, 2022 •

edited

cslzchen commented Oct 5, 2022 •

edited