-
Notifications
You must be signed in to change notification settings - Fork 327
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
[CR][ENG-5681] Great Big Python Upgrade #10648
Conversation
[ENG-5336] Closes #10566
- bump version for the transitions module; - add new modules: pillow, geoip2;
- added a new parameter to specify the encoding of strings in constants for nameparser module;
* update github deps to pull from COS repos
…rade [ENG-5332] Closes #10560
* Merge develop into feature/python-upgrade * Update module usage changes * Run pyupgrade over new code
[ENG-5364] Closes #10574
- replace ssl_cert_reqs with tlsAllowInvalidCertificates - remove dep from reqs.txt; update in addons/wiki/reqs.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This GitHub page is becoming unresponsive due to all the open files, so I'll be chunking the review. This chunk covers .dotfiles
through /addons
, the next chunk will begin with /admin
. No need to respond in whole or in part to the CR until all the chunks are through.
So far, generally looks good. Mostly nit-picking about f-string consistency, but a couple open questions, and a couple potential concerns.
@@ -3,7 +3,7 @@ | |||
language: python | |||
|
|||
python: | |||
- "3.6" | |||
- "3.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.12?
Also, since we're using GHA in favor of TCI, this file could probably be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be covered by Phase X ticket: https://openscience.atlassian.net/browse/ENG-5753
@@ -334,7 +334,7 @@ resetting docker. To back up your database, follow the following sequence of com | |||
|
|||
1. Install Postgres on your local machine, outside of docker. (eg `brew install postgres`) To avoid migrations, the | |||
version you install must match the one used by the docker container. | |||
([as of this writing](https://github.com/CenterForOpenScience/osf.io/blob/ce1702cbc95eb7777e5aaf650658a9966f0e6b0c/docker-compose.yml#L53), Postgres 9.6) | |||
([as of this writing](https://github.com/CenterForOpenScience/osf.io/blob/ce1702cbc95eb7777e5aaf650658a9966f0e6b0c/docker-compose.yml#L53), Postgres 15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit SHA still points to 9.6, it should probably be updated, but perhaps not until squashing this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be covered by Phase X ticket: https://openscience.atlassian.net/browse/ENG-5890
addons/base/generic_views.py
Outdated
'name': path.replace('All Files', '') if path != '/' else '/ (Full {})'.format( | ||
addon_full_name | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In keeping with other f-string's, maybe something like
'name': path.replace('All Files', '') if path != '/' else f'/ (Full {addon_full_name})',
addons/base/models.py
Outdated
'Because you have not configured the {addon} add-on, your authentication will not be ' | ||
'transferred to the forked {category}. You may authorize and configure the {addon} add-on ' | ||
'in the new fork on the settings page.' | ||
).format( | ||
addon=self.config.full_name, | ||
category=node.project_or_component, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this (and other similar messages, both in this file and in specific <addon>/models.py
files) use f-strings as well, rather than .format
calls?
assert_false(fork.get_addon(self.short_name).has_auth) | ||
assert_false(self.user_settings.verify_oauth_access(fork, self.external_account)) | ||
assert not fork.get_addon(self.short_name).has_auth | ||
assert not self.user_settings.verify_oauth_access(fork, self.external_account) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to leave a nit
here about how False is not None
, which could lead to expectation discrepancies, but I checked the source for assert_false
and saw that it didn't care about that either. Carry on.
addons/s3/tests/test_view.py
Outdated
|
||
@mock.patch('addons.s3.views.utils.create_bucket') | ||
def test_create_bucket_fail(self, mock_make): | ||
error = S3ResponseError(418, 'because Im a test') | ||
error = NoCredentialsError(operation_name='create_bucket') | ||
error.message = 'This should work' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error.message = 'This should work' |
addons/wiki/models.py
Outdated
'Page already exists with name {}'.format( | ||
new_name, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Page already exists with name {}'.format( | |
new_name, | |
) | |
f'Page already exists with name {new_name}' |
[ENG-5772] Closes #10651
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page is slowing down again, so this second chunk covers through api_tests/base
.
Will pick back up with api_tests/brands
📃
@@ -127,13 +125,6 @@ | |||
# Custom user model (extends AbstractBaseUser) | |||
AUTH_USER_MODEL = 'osf.OSFUser' | |||
|
|||
# TODO: Are there more granular ways to configure reporting specifically related to the API? | |||
RAVEN_CONFIG = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a new way to wire admin up to Sentry?
enabled = (not settings.DEV_MODE) and settings.SENTRY_DSN | ||
if enabled: | ||
init( | ||
dsn=settings.SENTRY_DSN, | ||
integrations=[CeleryIntegration(), DjangoIntegration(), FlaskIntegration()], | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it'd be better located in api/base/wsgi.py
if isinstance(value, bool) or value is None: | ||
return value | ||
return str(value).lower() in TRUTHY | ||
|
||
|
||
def is_falsy(value): | ||
return value in FALSY | ||
if isinstance(value, bool) or value is None: | ||
return not value | ||
return str(value).lower() in FALSY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much it matters, but the behavior here isn't quite the same -- None
can now be returned by is_truthy
.
Additionally, the conditionals could be removed because, e.g. str(True).lower() == 'true'
, which is in TRUTHY
already.
Previously, is_truthy(None) == is_falsy(None) == False
, because None
was not in either of the source field values, so I'm not sure if we need to handle it. If we do, could be accomplished by defining FALSY = fields.BooleanField.FALSE_VALUES | {'none'}
@@ -72,6 +72,7 @@ def get_bannable_urls(instance): | |||
return bannable_urls, parsed_absolute_url.hostname | |||
|
|||
|
|||
# this task is not runnable with celery as instance is not json serializable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be resolved in some way? It may not matter, with Varnish disabled
@@ -260,7 +262,9 @@ class BaseFileSerializer(JSONAPISerializer): | |||
|
|||
def absolute_url(self, obj): | |||
if obj.is_file: | |||
url = furl.furl(settings.DOMAIN).set( | |||
# NOTE: furl encoding to be verified later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I commented on this previously, but anything to do about this note?
api_tests/base/test_auth.py
Outdated
assert_not_in(self.user.username, res.json) | ||
assert res.status_code == 200 | ||
assert 'email' not in res.json['data']['attributes'] | ||
assert self.user.username not in res.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correctly "translated," but it seems like this test should actually be checking username not in json.dumps(res.json)
to verify it's not in there at all.
api_tests/base/test_auth.py
Outdated
assert_not_in(self.user2.username, res.json) | ||
assert res.status_code == 200 | ||
assert 'email' not in res.json['data']['attributes'] | ||
assert self.user2.username not in res.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto above
@@ -263,7 +245,7 @@ def test_InvalidFilterOperator_parameterizes_valid_operators(self): | |||
r'one of (?P<ops>.+)\.$', | |||
err.detail | |||
).groupdict()['ops'] | |||
assert_equal(ops, 'contains, icontains, eq, ne') | |||
assert ops == 'contains, icontains, eq, ne' | |||
|
|||
def test_parse_query_params_supports_multiple_filters(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be using pytest.mark.skip
rather than commenting the whole thing out. I'm also surprised the lack of a pass
didn't anger Flake
api_tests/base/test_utils.py
Outdated
'push_status_message() should have re-raised the ' | ||
'original RuntimeError with the original message.') | ||
assert str(e) == exception_message, ( | ||
'push_status_message() should have re-raised the ' 'original RuntimeError with the original message.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'push_status_message() should have re-raised the ' 'original RuntimeError with the original message.' | |
'push_status_message() should have re-raised the original RuntimeError with the original message.' |
api_tests/base/test_utils.py
Outdated
False, 'Unexpected Exception from push_status_message when called ' | ||
'from the v2 API with type "error"') | ||
assert False, ( | ||
'Unexpected Exception from push_status_message when called ' 'from the v2 API with type "error"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Unexpected Exception from push_status_message when called ' 'from the v2 API with type "error"' | |
'Unexpected Exception from push_status_message when called from the v2 API with type "error"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api_tests/
chunk is done, next chunk will resume at conftest.py
return '/{}draft_registrations/{}/relationships/institutions/'.format( | ||
API_BASE, node._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return '/{}draft_registrations/{}/relationships/institutions/'.format( | |
API_BASE, node._id) | |
return f'/{API_BASE}draft_registrations/{node._id}/relationships/institutions/' |
@@ -58,6 +59,7 @@ def primary_file(self, preprint): | |||
return preprint.primary_file | |||
|
|||
def test_file_serializer(self, file_one, node, node_folder): | |||
# TODO: check in qa url encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
? Several more reading the same in this file
@@ -67,7 +67,7 @@ def make_payload( | |||
}, | |||
settings.JWT_SECRET, | |||
algorithm='HS256' | |||
), | |||
).encode(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked about .encode
additions in an earlier chunk, but failed to mention that /osf/utils/fields
has an ensure_bytes
helper. It may or may not be preferable in some situations
'owner': | ||
{'login': 'test name'} | ||
})) | ||
mock_repo = Repository.from_json( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case where mock GH data could be DRY'd up a bit
), 'Permissions incorrect for {}. Should not have {} permission.'.format( | ||
user_id, api_perm | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
), 'Permissions incorrect for {}. Should not have {} permission.'.format( | |
user_id, api_perm | |
) | |
), f'Permissions incorrect for {user_id}. Should not have {api_perm} permission.' |
|
||
def test_reindex_provider_registration(self, mock_update_share, registration_provider, registration): | ||
call_command('reindex_provider', f'--providers={registration_provider._id}') | ||
assert mock_update_share.called_once_with(registration) | ||
mock_update_share.assert_called_with(registration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock_update_share.assert_called_with(registration) | |
mock_update_share.assert_called_once_with(registration) |
@@ -184,6 +184,7 @@ def test_skips_no_settings(self, node, user): | |||
on_node_updated(node._id, user._id, False, {'is_public'}) | |||
assert len(responses.calls) == 0 | |||
|
|||
@mark.skip('Synchronous retries not supported if celery >=5.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any wat to continue testing this behavior? If not, we might just want to remove these
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | ||
project_two._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this will lint
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | |
project_two._id) | |
assert ( | |
res.json['errors'][0]['detail'] == | |
f"The node {project_two._id} cannot be affiliated with this View Only Link because the node you're trying to affiliate is not descended from the node that the View Only Link is attached to." | |
) |
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | ||
component_one._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this will lint
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | |
component_one._id) | |
assert ( | |
res.json['errors'][0]['detail'] == | |
f"The node {component_one._id} cannot be affiliated with this View Only Link because the node you're trying to affiliate is not descended from the node that the View Only Link is attached to." | |
) |
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | ||
project_two._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this will lint
assert res.json['errors'][0]['detail'] == 'The node {} cannot be affiliated with this View Only Link because the node you\'re trying to affiliate is not descended from the node that the View Only Link is attached to.'.format( | |
project_two._id) | |
assert ( | |
res.json['errors'][0]['detail'] == | |
f"The node {project_two._id} cannot be affiliated with this View Only Link because the node you're trying to affiliate is not descended from the node that the View Only Link is attached to." | |
) |
* Update admin's django-webpack-loader (djwl) dep to point to a forked version. Recent versions of djwl expect a newer format for webpack-stats.json than is generated by the pinned version of the js package webpack-bundle-loader (webbt). The forked version of djwl supports both the older and the newer formats for webpack-stats.json.
[ENG-5818] [ENG-5872]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New chunk is in, this covers everything through osf_tests/
. Next chunk will start at poetry.lock
and also cover any updates to already-viewed files.
docker-compose.yml
Outdated
@@ -77,13 +77,13 @@ services: | |||
stdin_open: true | |||
|
|||
postgres: | |||
image: postgres:9.6 | |||
image: postgres:15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image: postgres:15 | |
image: postgres:15.4 |
@@ -77,13 +77,13 @@ services: | |||
stdin_open: true | |||
|
|||
postgres: | |||
image: postgres:9.6 | |||
image: postgres:15 | |||
command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole command
block should now be unnecessary, using a stock image
rm -Rf /python3.6/* && | ||
cp -Rf -p /usr/lib/python3.6 / | ||
- python -m venv /tmp/venv | ||
&& /tmp/venv/bin/pip install poetry==1.8.0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a me thing, but I can't seem to find poetry==1.8
~/D/c/osf.io ❯❯❯ dlog requirements (osf312) remotes/origin/pr/10648 ✭ ✱ ◼
osfio-requirements-1 | Collecting poetry==1.8.0
osfio-requirements-1 | Could not find a version that satisfies the requirement poetry==1.8.0 (from versions: 0.1.0, 0.2.0, 0.3.0, 0.4.0, 0.4.0.post1, 0.4.1, 0.4.2, 0.5.0b1, 0.5.0b2, 0.5.0, 0.6.0, 0.6.1, 0.6.2, 0.6.3b1, 0.6.3b2, 0.6.3b3, 0.6.3b4, 0.6.3b5, 0.6.3b6, 0.6.3b7, 0.6.3, 0.6.4b1, 0.6.4, 0.6.5, 0.7.0b1, 0.7.0b2, 0.7.0b3, 0.7.0b4, 0.7.0, 0.7.1, 0.8.0a0, 0.8.0a1, 0.8.0a2, 0.8.0a3, 0.8.0a4, 0.8.0, 0.8.1a0, 0.8.1, 0.8.2, 0.8.3, 0.8.4, 0.8.5a0, 0.8.5, 0.8.6, 0.9.0a0, 0.9.0a1, 0.9.0a2, 0.9.0a3, 0.9.0, 0.9.1, 0.10.0a0, 0.10.0a1, 0.10.0a2, 0.10.0a3, 0.10.0, 0.10.1, 0.10.2, 0.10.3, 0.11.0a0, 0.11.0a1, 0.11.0a2, 0.11.0a3, 0.11.0a4, 0.11.0, 0.11.1, 0.11.2, 0.11.3, 0.11.4, 0.11.5, 0.12.0a0, 0.12.0a1, 0.12.0a2, 0.12.0a3, 0.12.0a4, 0.12.0a5, 0.12.0, 0.12.1, 0.12.2, 0.12.3, 0.12.4, 0.12.5, 0.12.6, 0.12.7, 0.12.8, 0.12.9, 0.12.10, 0.12.11, 0.12.12, 0.12.13, 0.12.14, 0.12.15, 0.12.16, 0.12.17, 1.0.0a0, 1.0.0a1, 1.0.0a2, 1.0.0a3, 1.0.0a4, 1.0.0a5, 1.0.0b1, 1.0.0b2, 1.0.0b3, 1.0.0b4, 1.0.0b5, 1.0.0b6, 1.0.0b7, 1.0.0b8, 1.0.0b9, 1.0.0, 1.0.1, 1.0.2, 1.0.3, 1.0.4, 1.0.5, 1.0.6, 1.0.7, 1.0.8, 1.0.9, 1.0.10, 1.1.0a1, 1.1.0a2, 1.1.0a3, 1.1.0b1, 1.1.0b2, 1.1.0b3, 1.1.0b4, 1.1.0rc1, 1.1.0, 1.1.1, 1.1.2, 1.1.3, 1.1.4, 1.1.5, 1.1.6, 1.1.7, 1.1.8, 1.1.9, 1.1.10, 1.1.11, 1.1.12, 1.1.13, 1.1.14, 1.1.15, 1.2.0a1, 1.2.0a2)
osfio-requirements-1 | No matching distribution found for poetry==1.8.0
framework/auth/core.py
Outdated
'initials': lambda user: '{surname}, {initial}.'.format( | ||
surname=user.family_name, | ||
initial=user.given_name_initial, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'initials': lambda user: '{surname}, {initial}.'.format( | |
surname=user.family_name, | |
initial=user.given_name_initial, | |
), | |
'initials': lambda user: f'{user.family_name}, {user.given_name_initial}.', |
framework/auth/views.py
Outdated
status_message = ('If there is an OSF account associated with this unconfirmed email address {}, ' | ||
'a confirmation email has been resent to it. If you do not receive an email and believe ' | ||
'you should have, please contact OSF Support.').format(clean_email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status_message = ('If there is an OSF account associated with this unconfirmed email address {}, ' | |
'a confirmation email has been resent to it. If you do not receive an email and believe ' | |
'you should have, please contact OSF Support.').format(clean_email) | |
status_message = (f'If there is an OSF account associated with this unconfirmed email address {clean_email}, ' | |
'a confirmation email has been resent to it. If you do not receive an email and believe ' | |
'you should have, please contact OSF Support.' |
osf_tests/test_user.py
Outdated
@@ -592,7 +591,7 @@ def test_get_confirmation_url_when_token_is_expired_force(self, random_string): | |||
random_string.return_value = '54321' | |||
|
|||
url = u.get_confirmation_url('foo@bar.com', force=True) | |||
expected = '{0}confirm/{1}/{2}/'.format(settings.DOMAIN, u._id, '54321') | |||
expected = '{}confirm/{}/{}/'.format(settings.DOMAIN, u._id, '54321') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected = '{}confirm/{}/{}/'.format(settings.DOMAIN, u._id, '54321') | |
expected = f'{settings.DOMAIN}confirm/{u._id}/54321/' |
osf_tests/test_user.py
Outdated
assert ( | ||
summary['user_display_name'] == | ||
'Johnson' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert ( | |
summary['user_display_name'] == | |
'Johnson' | |
) | |
assert summary['user_display_name'] == 'Johnson' |
osf_tests/test_user.py
Outdated
assert ( | ||
summary['user_display_name'] == | ||
'Rock' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert ( | |
summary['user_display_name'] == | |
'Rock' | |
) | |
assert summary['user_display_name'] == 'Rock' |
osf_tests/test_user.py
Outdated
assert ( | ||
user.get_activity_points() == get_total_activity_count(user._primary_key) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert ( | |
user.get_activity_points() == get_total_activity_count(user._primary_key) | |
) | |
assert user.get_activity_points() == get_total_activity_count(user._primary_key) |
osf_tests/test_user.py
Outdated
assert ( | ||
UserSessionMap.objects.filter(user=user).count() == 0 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert ( | |
UserSessionMap.objects.filter(user=user).count() == 0 | |
) | |
assert UserSessionMap.objects.filter(user=user).count() == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final chunk is complete
Overall, mostly very minor issues, with a couple noted exceptions. I also tried to keep a running list of places where things changed just enough that they might require special attention during QA:
- OwnCloud urls with scheme (e.g. http://place.cloud)
- S3, broadly
- 2FA, ensure existing configs don't break across upgrade
- Admin config for sentry
- Django-Sendgrid emails working
- Old webtest forms -- resend confirmation, forgot password, etc
With that, pass complete 😩
pyproject.toml
Outdated
elasticsearch2 = "2.5.1" | ||
elasticsearch = "6.8.2" # max version to support elasticsearch6 | ||
elasticsearch-dsl = "6.4.0" # max version to support elasticsearch6 | ||
elastic-transport = "8.13.0" # hopefully, this won't break things, more thorough is needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any more thorough still needed?
pyproject.toml
Outdated
django-celery-results = "2.5.1" | ||
pyjwe = "1.0.0" | ||
# Required by pyjwe and ndg-httpsclient | ||
# Building wheel for cryptography >= 3.4.0 requires a Rust version incompatible with Docker base image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be no longer true
pyproject.toml
Outdated
jsonschema = "4.21.1" | ||
django-guardian = "2.4.0" | ||
# Admin requirements | ||
django-webpack-loader = {git = "https://github.com/felliott/django-webpack-loader.git", rev = "af8438c2da909ec9f2188a6c07c9d2caad0f7e93"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this to COS-forks?
requirements.txt
Outdated
hashids==1.3.1 | ||
pyjwt==2.8.0 | ||
django-celery-beat==2.6.0 | ||
django-celery-results==2.5.1 | ||
pyjwe==1.0.0 | ||
# Required by pyjwe and ndg-httpsclient | ||
# Building wheel for cryptography >= 3.4.0 requires a Rust version incompatible with Docker base image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, guessing this comment is now incorrect
requirements.txt
Outdated
django-webpack-loader==0.5.0 | ||
sendgrid-django==2.0.0 | ||
# django-webpack-loader==3.1.0 | ||
git+https://github.com/felliott/django-webpack-loader@feature/old-style-peanut-butter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, should this be in COS-Forks?
tests/test_views.py
Outdated
|
||
def test_external_login_confirm_email_get_with_another_user_logged_in(self): | ||
# TODO: check in qa url encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO still relevant?
tests/test_views.py
Outdated
self.auth = Auth(self.user) | ||
self.auth = (self.user.username, password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this change was necessary, it seems like it may change test behavior
tests/test_views.py
Outdated
|
||
@mock.patch('website.mails.send_mail') | ||
def test_external_login_confirm_email_get_create(self, mock_welcome): | ||
assert_false(self.user.is_registered) | ||
# TODO: check in qa url encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto re. TODO
tests/test_views.py
Outdated
|
||
# successfully reset password | ||
@pytest.mark.enable_enqueue_task | ||
@mock.patch('framework.auth.cas.CasClient.service_validate') | ||
def test_can_reset_password_if_form_success(self, mock_service_validate): | ||
# TODO: check in qa url encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto re. TODO
tests/test_views.py
Outdated
@@ -5097,12 +5043,11 @@ def test_reset_password_get_returns_403(self): | |||
) | |||
res = self.app.get( | |||
reset_password_get_url, | |||
expect_errors=True, | |||
headers={ | |||
headers={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headers={ | |
headers={ |
* many f-string updates * remove/update outdated comments * remove unnecessary kwargs * fix furl usage + use ensure_str
* resolve conflicts in docker-compose * update furl.furl in addons.base.views
* Convert categories from a list of strings to a list of `Category` objects * `mail.send` now returns a Response object rather than a tuple of status code and message. * Update tests to reflect above [ENG-5945]
* This was old code that was intended to configure message signing, but was actually a no-op. It still doesn't do anything but now throws a configuration error, so give it the boot.
* newrelic-admin is failing to start on py3.11+, so fall back to manually init-ing it in the wsgi code. See: https://stackoverflow.com/a/75665547
* it's now kebabed rather than squished
into feature/preprints-affiliations * 'develop' of https://github.com/CenterForOpenScience/osf.io: Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) # Conflicts: # api/nodes/serializers.py # osf/models/mixins.py
into feature/preprints-affiliations * 'develop' of https://github.com/CenterForOpenScience/osf.io: Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) # Conflicts: # api/nodes/serializers.py # osf/models/mixins.py
into feature/preprints-affiliations * 'develop' of https://github.com/CenterForOpenScience/osf.io: Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) # Conflicts: # api/nodes/serializers.py # osf/models/mixins.py
into fix-preprint-state-machince2 * 'develop' of https://github.com/CenterForOpenScience/osf.io: update dataverse dep revision to get changes Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) # Conflicts: # api/actions/serializers.py # api/providers/workflows.py # api_tests/identifiers/views/test_identifier_detail.py # api_tests/preprints/views/test_preprint_list.py # api_tests/reviews/mixins/filter_mixins.py # osf/utils/machines.py # osf/utils/workflows.py # osf_tests/test_schema_responses.py
into fix-preprint-state-machince2 * 'develop' of https://github.com/CenterForOpenScience/osf.io: update dataverse dep revision to get changes Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) # Conflicts: # api/actions/serializers.py # api/providers/workflows.py # api_tests/identifiers/views/test_identifier_detail.py # api_tests/preprints/views/test_preprint_list.py # api_tests/reviews/mixins/filter_mixins.py # osf/utils/machines.py # osf/utils/workflows.py # osf_tests/test_schema_responses.py
into remove-quickfiles-code * 'develop' of https://github.com/CenterForOpenScience/osf.io: (166 commits) update dataverse dep revision to get changes Update CHANGELOG, bump version [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648) Revert "[ENG-3685] Add permissions for withdrawn registration files (CenterForOpenScience#10650)" (CenterForOpenScience#10666) Check Registration READ perms on the Registration - Do not record download metrics for renders Fix signature Allow DOI metadata updates to be queued [ENG-3685] Add permissions for withdrawn registration files (CenterForOpenScience#10650) Update CHANGELOG, bump version [ENG-5030] Preprints Phase 2 - BE (CenterForOpenScience#10617) Update CHANGELOG, bump version Ensure Assumed-HAM users do not get autobanned [ENG-5762] Get GV set up in osf docker configs (CenterForOpenScience#10643) [ENG-5718] Use `make_auth` to avoid assumptions about `auth.user` (CenterForOpenScience#10647) [ENG-5699] Framework for getting Addon Info from GV (CenterForOpenScience#10641) [ENG-5178] Allow unauthenticated users to see public files (CenterForOpenScience#10645) Fix RelationshipPostMakesNoChanges exception in project creation (CenterForOpenScience#10644) [ENG - 5008] Support Unicode and special characters in file names during archiving (CenterForOpenScience#10627) Set Default Resource Type for Registrations to "Study Registration" (CenterForOpenScience#10636) Configurable GV Mock + HMAC Auth (CenterForOpenScience#10623) ... # Conflicts: # addons/base/views.py # api/caching/tasks.py # api_tests/files/views/test_file_detail.py # api_tests/wb/views/test_wb_hooks.py # osf/management/commands/data_storage_usage.py # osf/management/commands/reindex_quickfiles.py # osf/management/commands/transfer_quickfiles_to_projects.py # osf/models/__init__.py # osf/models/private_link.py # osf/models/quickfiles.py # osf/models/user.py # scripts/fix_merged_user_quickfiles.py # tests/test_views.py # website/mails/mails.py # website/search/elastic_search.py # website/settings/defaults.py
- Bump base python version from py3.6 to py3.12. - Switch to using poetry for dependency management - Bump most (not all) dependencies to their maximum version as of mid-March. - Significantly update Dockerfile - Upgrade Django to v4.2 - Generate test summary reports in CI --------- Co-authored-by: Oleh Paduchak <opaduchak@exoft.net> Co-authored-by: Mariia Lychko <lychko.mariia@gmail.com> Co-authored-by: Longze Chen <cslzchen@gmail.com>
Purpose
Double our pythons, Lacoön-style, by bumping from py3.6 to py3.12. Much dependency upgrades included!
Changes
poetry
for dependency management1240 files changed, 23012 insertions(+), 22362 deletions(-)
future
QA Notes
Full QA regression required, alongside selenium runs
Documentation
invoke test_addons_api
is nowinvoke test-addons-api
when called on the command line.Side Effects
None whatsoever.
Ticket
This ticket: https://openscience.atlassian.net/browse/ENG-5681
Epic link: https://openscience.atlassian.net/browse/ENG-4145