From d1afe1907c1dc0d05d833d9329d610a8396f87a6 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Wed, 5 Feb 2025 15:41:38 +0200 Subject: [PATCH 01/22] reduced number of requests to gv --- osf/external/gravy_valet/request_helpers.py | 75 ++++++++++++--------- osf/external/gravy_valet/translations.py | 23 +++++-- osf/models/node.py | 15 +++-- osf/models/user.py | 14 ---- 4 files changed, 71 insertions(+), 56 deletions(-) diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index a86772868bb..5265d5d4add 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -1,14 +1,14 @@ -from urllib.parse import urlencode, urljoin, urlparse, urlunparse - -import logging import dataclasses +import enum +import logging import typing +from urllib.parse import urlencode, urljoin, urlparse, urlunparse -from . import auth_helpers import requests from requests.exceptions import RequestException from website import settings +from . import auth_helpers logger = logging.getLogger(__name__) @@ -27,15 +27,21 @@ RESOURCE_LIST_ENDPOINT = f'{API_BASE}resource-references' RESOURCE_DETAIL_ENDPOINT = f'{API_BASE}resource-references/{{pk}}' - -ACCOUNT_EXTERNAL_SERVICE_PATH = 'external_storage_service' -ACCOUNT_OWNER_PATH = 'base_account.account_owner' -ADDON_EXTERNAL_SERVICE_PATH = 'base_account.external_storage_service' +ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH = 'external_storage_service' +ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH = 'external_computing_service' ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH = 'external_citation_service' +ACCOUNT_EXTERNAL_SERVICE_ENDPOINT = f'{API_BASE}external-{{addon_type}}-services' +ACCOUNT_OWNER_PATH = 'base_account.account_owner' +ADDON_EXTERNAL_STORAGE_SERVICE_PATH = 'base_account.external_storage_service' ADDON_EXTERNAL_CITATIONS_SERVICE_PATH = 'base_account.external_citation_service' -ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH = 'external_computing_service' ADDON_EXTERNAL_COMPUTING_SERVICE_PATH = 'base_account.external_computing_service' + +class AddonType(enum.StrEnum): + STORAGE = enum.auto() + CITATION = enum.auto() + COMPUTING = enum.auto() + CITATION_ITEM_TYPE_ALIASES = { 'COLLECTION': 'folder', 'DOCUMENT': 'file', @@ -47,7 +53,7 @@ def get_account(gv_account_pk, requesting_user): # -> JSONAPIResultEntry return get_gv_result( endpoint_url=ACCOUNT_ENDPOINT.format(pk=gv_account_pk), requesting_user=requesting_user, - params={'include': ACCOUNT_EXTERNAL_SERVICE_PATH}, + params={'include': ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}, ) @@ -80,7 +86,7 @@ def get_addon(gv_addon_pk, requested_resource, requesting_user, addon_type: str) endpoint_url=ADDON_ENDPOINT.format(pk=gv_addon_pk, addon_type=addon_type), requesting_user=requesting_user, requested_resource=requested_resource, - params={'include': ADDON_EXTERNAL_SERVICE_PATH}, + params={'include': ADDON_EXTERNAL_STORAGE_SERVICE_PATH}, ) @@ -96,7 +102,7 @@ def iterate_accounts_for_user(requesting_user): # -> typing.Iterator[JSONAPIRes yield from iterate_gv_results( endpoint_url=user_result.get_related_link('authorized_storage_accounts'), requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_SERVICE_PATH}'} + params={'include': f'{ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}'} ) yield from iterate_gv_results( endpoint_url=user_result.get_related_link('authorized_citation_accounts'), @@ -110,7 +116,7 @@ def iterate_accounts_for_user(requesting_user): # -> typing.Iterator[JSONAPIRes ) -def iterate_addons_for_resource(requested_resource, requesting_user): # -> typing.Iterator[JSONAPIResultEntry] +def iterate_addons_for_resource(requested_resource, requesting_user, addon_type=None): # -> typing.Iterator[JSONAPIResultEntry] '''Returns an iterator of JSONAPIResultEntires representing all of the ConfiguredStorageAddons for a resource.''' resource_result = get_gv_result( endpoint_url=RESOURCE_LIST_ENDPOINT, @@ -120,24 +126,27 @@ def iterate_addons_for_resource(requested_resource, requesting_user): # -> typi ) if not resource_result: return None - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_storage_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_citation_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_CITATIONS_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_computing_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_COMPUTING_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) + if not addon_type or addon_type == AddonType.STORAGE: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_storage_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_STORAGE_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) + if not addon_type or addon_type == AddonType.CITATION: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_citation_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_CITATIONS_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) + if not addon_type or addon_type == AddonType.COMPUTING: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_computing_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_COMPUTING_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) def get_waterbutler_config(gv_addon_pk, requested_resource, requesting_user, addon_type): # -> JSONAPIResultEntry @@ -246,10 +255,12 @@ def _make_gv_request( assert not (request_method == 'GET' and json_data is not None) try: response = requests.request(url=endpoint_url, headers=auth_headers, params=params, method=request_method, json=json_data) - except RequestException: + except RequestException as e: + logger.error(f"Cannot reach GravyValet: {e}") return None if not response.ok: # log error to Sentry + logger.error(f"GV request failed with status code {response.status_code}: {response.content}") pass return response diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index 60f13af6748..69bfac201da 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -1,5 +1,4 @@ import dataclasses -import enum from dataclasses import asdict, InitVar from typing import TYPE_CHECKING @@ -10,10 +9,6 @@ if TYPE_CHECKING: from osf.models import OSFUser, Node -class AddonType(enum.StrEnum): - STORAGE = enum.auto() - CITATION = enum.auto() - COMPUTING = enum.auto() def make_ephemeral_user_settings(gv_account_data, requesting_user): include_path = f'external_{gv_account_data.resource_type.split('-')[1]}_service' @@ -40,6 +35,20 @@ def make_ephemeral_node_settings(gv_addon_data: gv_requests.JSONAPIResultEntry, wb_key=config.wb_key, ) +_services = None + +def get_external_services(requesting_user): + global _services + if _services: + return _services + _services = [] + for addon_type in gv_requests.AddonType: + srv = [EphemeralAddonConfig(service) for service in gv_requests.iterate_gv_results( + endpoint_url=gv_requests.ACCOUNT_EXTERNAL_SERVICE_ENDPOINT.format(addon_type=addon_type), + requesting_user=requesting_user, + )] + _services += srv + return _services @dataclasses.dataclass class EphemeralAddonConfig: @@ -53,6 +62,7 @@ class EphemeralAddonConfig: has_widget: bool = dataclasses.field(init=False, default=False) icon_url: str = dataclasses.field(init=False) wb_key: str = dataclasses.field(init=False) + type: str = dataclasses.field(init=False) def __post_init__(self, gv_data: gv_requests.JSONAPIResultEntry): self.short_name = gv_data.get_attribute('external_service_name') @@ -61,6 +71,7 @@ def __post_init__(self, gv_data: gv_requests.JSONAPIResultEntry): self.has_widget = gv_data.resource_type == 'external-citation-services' self.icon_url = gv_data.get_attribute('icon_url') self.wb_key = gv_data.get_attribute('wb_key') + self.type = gv_data.resource_type.split('-')[1] def to_json(self): return asdict(self) @@ -242,7 +253,7 @@ def after_fork(self, node: 'Node', fork, user: 'OSFUser', save=True): ) def get_settings_class(addon_type): - if addon_type == AddonType.STORAGE: + if addon_type == gv_requests.AddonType.STORAGE: return _get_storage_settings_class() return EphemeralNodeSettings diff --git a/osf/models/node.py b/osf/models/node.py index 4202b85f901..47e69c358c6 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -2471,18 +2471,25 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): try: gv_addons = request.gv_addons except AttributeError: - request.gv_addons = self._get_addons_from_gv(requesting_user_id) - gv_addons = request.gv_addons + requesting_user = OSFUser.load(requesting_user_id) + services = gv_translations.get_external_services(requesting_user) + for service in services: + if service.short_name == gv_pk: + break + else: + return None + gv_addons = request.gv_addons = self._get_addons_from_gv(requesting_user_id, service.type) for item in gv_addons: if item.short_name == gv_pk: return item - def _get_addons_from_gv(self, requesting_user_id): + def _get_addons_from_gv(self, requesting_user_id, service_type=None): requesting_user = OSFUser.load(requesting_user_id) all_node_addon_data = gv_requests.iterate_addons_for_resource( requested_resource=self, - requesting_user=requesting_user + requesting_user=requesting_user, + addon_type=service_type ) return [ gv_translations.make_ephemeral_node_settings( diff --git a/osf/models/user.py b/osf/models/user.py index 5cb055d30fd..55bccf27de8 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1956,20 +1956,6 @@ def check_spam(self, saved_fields, request_headers): return is_spam - def _get_addon_from_gv(self, gv_pk, requesting_user_id): - requesting_user = OSFUser.load(requesting_user_id) - if requesting_user and requesting_user != self: - raise ValueError('Cannot get user addons for a user other than self') - - gv_account_data = gv_requests.get_account( - gv_account_pk=gv_pk, - requesting_user=self, - ) - return gv_translations.make_ephemeral_node_settings( - gv_account_data=gv_account_data, - requesting_user=self, - ) - def _get_addons_from_gv(self, requesting_user_id): requesting_user = OSFUser.load(requesting_user_id) if requesting_user and requesting_user != self: From ecc4329510b0795c2981b7438876c69fcef9497b Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Wed, 5 Feb 2025 19:57:58 +0200 Subject: [PATCH 02/22] removed redundant request to GV --- api/nodes/utils.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/nodes/utils.py b/api/nodes/utils.py index bdcc49898b5..32caccd1467 100644 --- a/api/nodes/utils.py +++ b/api/nodes/utils.py @@ -37,9 +37,6 @@ def get_file_object(target, path, provider, request): obj = get_object_or_error(model, Q(target_object_id=target.pk, target_content_type=content_type, _id=path.strip('/')), request) return obj - if isinstance(target, AbstractNode) and not target.get_addon(provider) or not target.get_addon(provider).configured: - raise NotFound(f'The {provider} provider is not configured for this project.') - view_only = request.query_params.get('view_only', default=None) base_url = None if hasattr(target, 'osfstorage_region'): @@ -58,7 +55,7 @@ def get_file_object(target, path, provider, request): if waterbutler_request.status_code == 401: raise PermissionDenied - if waterbutler_request.status_code == 404: + if waterbutler_request.status_code == 404 or waterbutler_request.status_code == 400: raise NotFound if is_server_error(waterbutler_request.status_code): From a2fcdc6d9e43d8c879f894f70945f3d8e1953b19 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Wed, 5 Feb 2025 20:18:49 +0200 Subject: [PATCH 03/22] removed redundant requests for citation and compute addons --- api/nodes/views.py | 2 +- osf/models/mixins.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index 8bbe4df3595..a45718051e7 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -1555,7 +1555,7 @@ def get_queryset(self): return [ self.get_provider_item(addon, node=node) for addon - in node.get_addons() + in node.get_addons('storage') if addon.config.has_hgrid_files and addon.configured ] diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 34098d49114..7c82044fd1a 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -482,14 +482,14 @@ def get_addon_key(cls, config): def addons(self): return self.get_addons() - def get_addons(self): + def get_addons(self, service_type: str | None = None): request, user_id = get_request_and_user_id() if flag_is_active(request, features.ENABLE_GV): osf_addons = filter( lambda x: x is not None, (self.get_addon(addon) for addon in self.OSF_HOSTED_ADDONS) ) - return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id)) + return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id, service_type=service_type)) return [_f for _f in [ self.get_addon(config.short_name) From b63177a5c3864a03872bfd8cce9bb76b90472204 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Wed, 5 Feb 2025 15:19:58 -0500 Subject: [PATCH 04/22] fix tests --- api_tests/draft_nodes/views/test_draft_node_files_lists.py | 4 ++-- osf/external/gravy_valet/request_helpers.py | 2 +- osf/models/user.py | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api_tests/draft_nodes/views/test_draft_node_files_lists.py b/api_tests/draft_nodes/views/test_draft_node_files_lists.py index 6c6ff99e066..4aa1e06f325 100644 --- a/api_tests/draft_nodes/views/test_draft_node_files_lists.py +++ b/api_tests/draft_nodes/views/test_draft_node_files_lists.py @@ -388,7 +388,7 @@ def test_notfound_node_folder_returns_file(self): expect_errors=True, headers={'COOKIE': 'foo=bar;'} # Webtests doesnt support cookies? ) - assert res.status_code == 404 + assert res.status_code == 503 @responses.activate def test_waterbutler_server_error_returns_503(self): @@ -416,7 +416,7 @@ def test_waterbutler_invalid_data_returns_503(self): ) url = f'/{API_BASE}draft_nodes/{self.draft_node._id}/files/github/' res = self.app.get(url, auth=self.user.auth, expect_errors=True) - assert res.status_code == 503 + assert res.status_code == 404 @responses.activate def test_handles_unauthenticated_waterbutler_request(self): diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index 5265d5d4add..afc4ca8861d 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -90,7 +90,7 @@ def get_addon(gv_addon_pk, requested_resource, requesting_user, addon_type: str) ) -def iterate_accounts_for_user(requesting_user): # -> typing.Iterator[JSONAPIResultEntry] +def iterate_accounts_for_user(requesting_user, addon_type=None): # -> typing.Iterator[JSONAPIResultEntry] '''Returns an iterator of JSONAPIResultEntries representing all of the AuthorizedStorageAccounts for a user.''' user_result = get_gv_result( endpoint_url=USER_LIST_ENDPOINT, diff --git a/osf/models/user.py b/osf/models/user.py index 55bccf27de8..5a1183f9547 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1956,13 +1956,14 @@ def check_spam(self, saved_fields, request_headers): return is_spam - def _get_addons_from_gv(self, requesting_user_id): + def _get_addons_from_gv(self, requesting_user_id, service_type=None): requesting_user = OSFUser.load(requesting_user_id) if requesting_user and requesting_user != self: raise ValueError('Cannot get user addons for a user other than self') all_user_account_data = gv_requests.iterate_accounts_for_user( requesting_user=self, + addon_type=service_type, ) for account_data in all_user_account_data: yield gv_translations.make_ephemeral_user_settings( From 4363472dbc6f1fcb578620a472cdba3d1077609c Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Wed, 5 Feb 2025 20:37:12 -0500 Subject: [PATCH 05/22] fix some test --- .../views/test_draft_node_files_lists.py | 17 ++++++++++++++--- api_tests/nodes/views/test_node_files_list.py | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/api_tests/draft_nodes/views/test_draft_node_files_lists.py b/api_tests/draft_nodes/views/test_draft_node_files_lists.py index 4aa1e06f325..560058d69b5 100644 --- a/api_tests/draft_nodes/views/test_draft_node_files_lists.py +++ b/api_tests/draft_nodes/views/test_draft_node_files_lists.py @@ -3,6 +3,7 @@ import datetime import json from django.utils import timezone +import pytest from framework.auth.core import Auth @@ -376,19 +377,22 @@ def test_notfound_node_file_returns_folder(self): ) assert res.status_code == 404 + # This test is skipped because it was wrongly configured in the first place + # The reason OSF returns a 404 is not because WB returns a file when OSF expects a folder + # But because the addon itself is not configured for the node + @pytest.mark.skip('ENG-7256') @responses.activate def test_notfound_node_folder_returns_file(self): self._prepare_mock_wb_response( provider='github', files=[{'name': 'NewFile'}], folder=False, path='/') - url = f'/{API_BASE}draft_nodes/{self.draft_node._id}/files/github/' res = self.app.get( url, auth=self.user.auth, expect_errors=True, headers={'COOKIE': 'foo=bar;'} # Webtests doesnt support cookies? ) - assert res.status_code == 503 + assert res.status_code == 404 @responses.activate def test_waterbutler_server_error_returns_503(self): @@ -404,6 +408,8 @@ def test_waterbutler_server_error_returns_503(self): @responses.activate def test_waterbutler_invalid_data_returns_503(self): + # TODO: if WB returns 400, we would instead return 404 instead of 503 + # because of the change in get_file_object(); We should handle this more gracefully wb_url = waterbutler_api_url_for(self.draft_node._id, _internal=True, provider='github', path='/', meta=True, base_url=self.draft_node.osfstorage_region.waterbutler_url) self.add_github() responses.add( @@ -438,14 +444,19 @@ def test_handles_notfound_waterbutler_request(self): assert res.status_code == 404 assert 'detail' in res.json['errors'][0] + @responses.activate def test_handles_request_to_provider_not_configured_on_project(self): + self._prepare_mock_wb_response( + provider='box', status_code=400, + ) provider = 'box' url = '/{}draft_nodes/{}/files/{}/'.format( API_BASE, self.draft_node._id, provider) res = self.app.get(url, auth=self.user.auth, expect_errors=True) assert not self.draft_node.get_addon(provider) assert res.status_code == 404 - assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' + # TODO: ENG-7256 Handle this case more gracefully + # assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' @responses.activate def test_handles_bad_waterbutler_request(self): diff --git a/api_tests/nodes/views/test_node_files_list.py b/api_tests/nodes/views/test_node_files_list.py index 46afa3b3289..eebc628daec 100644 --- a/api_tests/nodes/views/test_node_files_list.py +++ b/api_tests/nodes/views/test_node_files_list.py @@ -386,7 +386,7 @@ def test_waterbutler_invalid_data_returns_503(self): ) url = f'/{API_BASE}nodes/{self.project._id}/files/github/' res = self.app.get(url, auth=self.user.auth, expect_errors=True) - assert res.status_code == 503 + assert res.status_code == 404 @responses.activate def test_handles_unauthenticated_waterbutler_request(self): From 9a066ecea2f3a84184dd6eeec6370308a620d94e Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Wed, 5 Feb 2025 21:19:05 -0500 Subject: [PATCH 06/22] fix remaining tests --- .../draft_nodes/views/test_draft_node_files_lists.py | 9 ++++++--- api_tests/nodes/views/test_node_files_list.py | 12 +++++++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/api_tests/draft_nodes/views/test_draft_node_files_lists.py b/api_tests/draft_nodes/views/test_draft_node_files_lists.py index 560058d69b5..bad74618834 100644 --- a/api_tests/draft_nodes/views/test_draft_node_files_lists.py +++ b/api_tests/draft_nodes/views/test_draft_node_files_lists.py @@ -380,12 +380,13 @@ def test_notfound_node_file_returns_folder(self): # This test is skipped because it was wrongly configured in the first place # The reason OSF returns a 404 is not because WB returns a file when OSF expects a folder # But because the addon itself is not configured for the node - @pytest.mark.skip('ENG-7256') + @pytest.mark.skip('TODO: ENG-7256') @responses.activate def test_notfound_node_folder_returns_file(self): self._prepare_mock_wb_response( provider='github', files=[{'name': 'NewFile'}], folder=False, path='/') + url = f'/{API_BASE}draft_nodes/{self.draft_node._id}/files/github/' res = self.app.get( url, auth=self.user.auth, @@ -408,8 +409,10 @@ def test_waterbutler_server_error_returns_503(self): @responses.activate def test_waterbutler_invalid_data_returns_503(self): - # TODO: if WB returns 400, we would instead return 404 instead of 503 - # because of the change in get_file_object(); We should handle this more gracefully + # TODO: ENG-7256 -if WB returns 400, we should return 503 + # However because of the change in get_file_object(), we can't distinguish + # between a 400 that's caused by an addon not found and a more general 400 meaning invalid data was passed + # We should handle this more gracefully wb_url = waterbutler_api_url_for(self.draft_node._id, _internal=True, provider='github', path='/', meta=True, base_url=self.draft_node.osfstorage_region.waterbutler_url) self.add_github() responses.add( diff --git a/api_tests/nodes/views/test_node_files_list.py b/api_tests/nodes/views/test_node_files_list.py index eebc628daec..ce01ef7e942 100644 --- a/api_tests/nodes/views/test_node_files_list.py +++ b/api_tests/nodes/views/test_node_files_list.py @@ -5,6 +5,7 @@ import responses from django.utils import timezone from waffle.testutils import override_flag +import pytest from framework.auth.core import Auth @@ -346,6 +347,10 @@ def test_notfound_node_file_returns_folder(self): ) assert res.status_code == 404 + # This test is skipped because it was wrongly configured in the first place + # The reason OSF returns a 404 is not because WB returns a file when OSF expects a folder + # But because the addon itself is not configured for the node + @pytest.mark.skip('TODO: ENG-7256') @responses.activate def test_notfound_node_folder_returns_file(self): self._prepare_mock_wb_response( @@ -408,14 +413,19 @@ def test_handles_notfound_waterbutler_request(self): assert res.status_code == 404 assert 'detail' in res.json['errors'][0] + @responses.activate def test_handles_request_to_provider_not_configured_on_project(self): + self._prepare_mock_wb_response( + provider='box', status_code=400, + ) provider = 'box' url = '/{}nodes/{}/files/{}/'.format( API_BASE, self.project._id, provider) res = self.app.get(url, auth=self.user.auth, expect_errors=True) assert not self.project.get_addon(provider) assert res.status_code == 404 - assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' + # TODO: ENG-7256 Handle this case more gracefully + # assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' @responses.activate def test_handles_bad_waterbutler_request(self): From be409520cd96dd65113baec852269d9a579fdaea Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Fri, 7 Feb 2025 12:00:32 -0500 Subject: [PATCH 07/22] address CR comments --- osf/external/gravy_valet/request_helpers.py | 33 +++++++++++---------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index afc4ca8861d..e5795c1c6be 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -99,21 +99,24 @@ def iterate_accounts_for_user(requesting_user, addon_type=None): # -> typing.It ) if not user_result: return None - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_storage_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_citation_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_computing_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH}'} - ) + if not addon_type or addon_type == AddonType.STORAGE: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_storage_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}'} + ) + if not addon_type or addon_type == AddonType.CITATION: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_citation_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH}'} + ) + if not addon_type or addon_type == AddonType.COMPUTING: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_computing_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH}'} + ) def iterate_addons_for_resource(requested_resource, requesting_user, addon_type=None): # -> typing.Iterator[JSONAPIResultEntry] From 80c7ca57216b5a34b7fdf4ef9a1f951d8f2e2a50 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Sun, 9 Feb 2025 01:18:27 -0500 Subject: [PATCH 08/22] very crude working version to reduce the amount of calls from OSF to GV during get_auth --- addons/base/views.py | 19 ++++++++--- osf/external/gravy_valet/translations.py | 42 ++++++++++++------------ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/addons/base/views.py b/addons/base/views.py index 54e091a0b2b..473528778ba 100644 --- a/addons/base/views.py +++ b/addons/base/views.py @@ -58,6 +58,7 @@ ) from osf.metrics import PreprintView, PreprintDownload from osf.utils import permissions +from osf.external.gravy_valet import request_helpers from website.profile.utils import get_profile_image_url from website.project import decorators from website.project.decorators import must_be_contributor_or_public, must_be_valid_project, check_contributor_auth @@ -227,15 +228,25 @@ def get_auth(auth, **kwargs): _check_resource_permissions(resource, auth, action) provider_name = waterbutler_data['provider'] + waterbutler_settings = None + waterbutler_credentials = None file_version = file_node = None if provider_name == 'osfstorage': file_version, file_node = _get_osfstorage_file_version_and_node( file_path=waterbutler_data.get('path'), file_version_id=waterbutler_data.get('version') ) - - waterbutler_settings, waterbutler_credentials = _get_waterbutler_configs( - resource=resource, provider_name=provider_name, file_version=file_version, - ) + waterbutler_settings, waterbutler_credentials = _get_waterbutler_configs( + resource=resource, provider_name=provider_name, file_version=file_version, + ) + else: + result = request_helpers.get_waterbutler_config( + gv_addon_pk=f'{waterbutler_data['nid']}:{waterbutler_data['provider']}', + requested_resource=AbstractNode.load(waterbutler_data['nid']), + requesting_user=auth.user, + addon_type='configured-storage-addons', + ) + waterbutler_settings = result.get_attribute('config') + waterbutler_credentials = result.get_attribute('credentials') _enqueue_metrics( file_version=file_version, diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index 60f13af6748..f77fa23f6d3 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -162,27 +162,27 @@ def before_page_load(self, *args, **kwargs): def folder_id(self): return self.gv_data.get_attribute('root_folder') - def serialize_waterbutler_credentials(self): - # sufficient for most OAuth services, including Box - # TODO: Define per-service translation (and/or common schemes) - if self._credentials is None: - self._fetch_wb_config() - return self._credentials - - def serialize_waterbutler_settings(self): - if self._config is None: - self._fetch_wb_config() - return self._config - - def _fetch_wb_config(self): - result = gv_requests.get_waterbutler_config( - gv_addon_pk=self.gv_data.resource_id, - requested_resource=self.configured_resource, - requesting_user=self.active_user, - addon_type=self.gv_data.resource_type, - ) - self._credentials = result.get_attribute('credentials') - self._config = result.get_attribute('config') + # def serialize_waterbutler_credentials(self): + # # sufficient for most OAuth services, including Box + # # TODO: Define per-service translation (and/or common schemes) + # if self._credentials is None: + # self._fetch_wb_config() + # return self._credentials + + # def serialize_waterbutler_settings(self): + # if self._config is None: + # self._fetch_wb_config() + # return self._config + + # def _fetch_wb_config(self): + # result = gv_requests.get_waterbutler_config( + # gv_addon_pk=self.gv_data.resource_id, + # requested_resource=self.configured_resource, + # requesting_user=self.active_user, + # addon_type=self.gv_data.resource_type, + # ) + # self._credentials = result.get_attribute('credentials') + # self._config = result.get_attribute('config') def create_waterbutler_log(self, *args, **kwargs): pass From 8b8072950b4b79a95187762623ef78ad4a4733b9 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 10 Feb 2025 19:01:55 +0200 Subject: [PATCH 09/22] fixed fetching rubeus files grid when gv is active --- website/project/views/file.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/website/project/views/file.py b/website/project/views/file.py index 56da1718765..780b26a740c 100644 --- a/website/project/views/file.py +++ b/website/project/views/file.py @@ -34,5 +34,7 @@ def collect_file_trees(auth, node, **kwargs): def grid_data(auth, node, **kwargs): """View that returns the formatted data for rubeus.js/hgrid """ + if flag_is_active(request, features.ENABLE_GV): + return {'data': {}} data = request.args.to_dict() return {'data': rubeus.to_hgrid(node, auth, **data)} From 2bbae5a1c9f11da7027420a88df040c6a1a1ecb1 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Mon, 10 Feb 2025 13:52:06 -0500 Subject: [PATCH 10/22] refinement --- addons/base/views.py | 4 +-- osf/external/gravy_valet/translations.py | 42 ++++++++++++------------ 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/addons/base/views.py b/addons/base/views.py index 473528778ba..b7cafc88c49 100644 --- a/addons/base/views.py +++ b/addons/base/views.py @@ -231,7 +231,7 @@ def get_auth(auth, **kwargs): waterbutler_settings = None waterbutler_credentials = None file_version = file_node = None - if provider_name == 'osfstorage': + if provider_name == 'osfstorage' or (not flag_is_active(request, features.ENABLE_GV)): file_version, file_node = _get_osfstorage_file_version_and_node( file_path=waterbutler_data.get('path'), file_version_id=waterbutler_data.get('version') ) @@ -241,7 +241,7 @@ def get_auth(auth, **kwargs): else: result = request_helpers.get_waterbutler_config( gv_addon_pk=f'{waterbutler_data['nid']}:{waterbutler_data['provider']}', - requested_resource=AbstractNode.load(waterbutler_data['nid']), + requested_resource=resource, requesting_user=auth.user, addon_type='configured-storage-addons', ) diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index f77fa23f6d3..60f13af6748 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -162,27 +162,27 @@ def before_page_load(self, *args, **kwargs): def folder_id(self): return self.gv_data.get_attribute('root_folder') - # def serialize_waterbutler_credentials(self): - # # sufficient for most OAuth services, including Box - # # TODO: Define per-service translation (and/or common schemes) - # if self._credentials is None: - # self._fetch_wb_config() - # return self._credentials - - # def serialize_waterbutler_settings(self): - # if self._config is None: - # self._fetch_wb_config() - # return self._config - - # def _fetch_wb_config(self): - # result = gv_requests.get_waterbutler_config( - # gv_addon_pk=self.gv_data.resource_id, - # requested_resource=self.configured_resource, - # requesting_user=self.active_user, - # addon_type=self.gv_data.resource_type, - # ) - # self._credentials = result.get_attribute('credentials') - # self._config = result.get_attribute('config') + def serialize_waterbutler_credentials(self): + # sufficient for most OAuth services, including Box + # TODO: Define per-service translation (and/or common schemes) + if self._credentials is None: + self._fetch_wb_config() + return self._credentials + + def serialize_waterbutler_settings(self): + if self._config is None: + self._fetch_wb_config() + return self._config + + def _fetch_wb_config(self): + result = gv_requests.get_waterbutler_config( + gv_addon_pk=self.gv_data.resource_id, + requested_resource=self.configured_resource, + requesting_user=self.active_user, + addon_type=self.gv_data.resource_type, + ) + self._credentials = result.get_attribute('credentials') + self._config = result.get_attribute('config') def create_waterbutler_log(self, *args, **kwargs): pass From caeb3ba918d6f224f7c4213494dd722760d4f4ab Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Tue, 11 Feb 2025 20:36:57 -0500 Subject: [PATCH 11/22] put user on the request context --- website/archiver/tasks.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index ccf3e52ca0a..2ada447bd05 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -35,6 +35,7 @@ AbstractNode, DraftRegistration, ) +from osf.utils.requests import get_current_request def create_app_context(): @@ -127,6 +128,8 @@ def stat_addon(addon_short_name, job_pk): create_app_context() job = ArchiveJob.load(job_pk) src, dst, user = job.info() + request = get_current_request() + request.user = user src_addon = src.get_addon(addon_name) if hasattr(src_addon, 'configured') and not src_addon.configured: # Addon enabled but not configured - no file trees, nothing to archive. From bc335054448433a74d9f2603341ec9bfb0def8fc Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Wed, 12 Feb 2025 22:08:06 -0500 Subject: [PATCH 12/22] fix --- website/archiver/tasks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index 2ada447bd05..0020ac565b3 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -208,6 +208,8 @@ def archive_addon(addon_short_name, job_pk): params['revision'] = 'latest' if addon_short_name.split('-')[-1] == 'draft' else 'latest-published' rename_suffix = ' (draft)' if addon_short_name.split('-')[-1] == 'draft' else ' (published)' addon_short_name = 'dataverse' + request = get_current_request() + request.user = user src_provider = src.get_addon(addon_short_name) folder_name_nfd, folder_name_nfc = normalize_unicode_filenames(src_provider.archive_folder_name) rename = f'{folder_name_nfd}{rename_suffix}' From 498173c70129660dd3bdbd56e5eae9494d4360dd Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Thu, 13 Feb 2025 16:38:22 -0500 Subject: [PATCH 13/22] add logger --- osf/models/node.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osf/models/node.py b/osf/models/node.py index 47e69c358c6..8029f797608 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -2468,6 +2468,10 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): request = get_current_request() # This is to avoid making multiple requests to GV # within the lifespan of one request on the OSF side + from osf.utils.requests import DummyRequest + if isinstance(request, DummyRequest): + logger.info(f'the request is {request}') + logger.info(f'the user of the request is {request.user}') try: gv_addons = request.gv_addons except AttributeError: From c433a6a726dd17376365525f5f9b994d504b345b Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Thu, 13 Feb 2025 19:38:28 -0500 Subject: [PATCH 14/22] log something to sentry --- osf/models/node.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/osf/models/node.py b/osf/models/node.py index 8029f797608..f5f6120826f 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -2468,10 +2468,8 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): request = get_current_request() # This is to avoid making multiple requests to GV # within the lifespan of one request on the OSF side - from osf.utils.requests import DummyRequest - if isinstance(request, DummyRequest): - logger.info(f'the request is {request}') - logger.info(f'the user of the request is {request.user}') + log_exception(f'the request is {request}') + log_exception(f'the user of the request is {request.user}') try: gv_addons = request.gv_addons except AttributeError: From b15c0411325cdf76255877751a0388108a67104f Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Thu, 13 Feb 2025 21:35:32 -0500 Subject: [PATCH 15/22] log message --- osf/models/node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osf/models/node.py b/osf/models/node.py index f5f6120826f..fc36c643625 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -33,7 +33,7 @@ from framework.auth import oauth_scopes from framework.celery_tasks.handlers import enqueue_task, get_task_from_queue from framework.exceptions import PermissionsError, HTTPError -from framework.sentry import log_exception +from framework.sentry import log_exception, log_message from osf.exceptions import (InvalidTagError, NodeStateError, TagNotFoundError) from .contributor import Contributor @@ -2468,8 +2468,8 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): request = get_current_request() # This is to avoid making multiple requests to GV # within the lifespan of one request on the OSF side - log_exception(f'the request is {request}') - log_exception(f'the user of the request is {request.user}') + log_message(f'the request is {request}') + log_message(f'the user of the request is {request.user}') try: gv_addons = request.gv_addons except AttributeError: From 24b99881ac5a0d77af0a6a13ef191500e19e1772 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Fri, 14 Feb 2025 01:48:32 -0500 Subject: [PATCH 16/22] try this now --- osf/models/node.py | 4 +--- website/archiver/tasks.py | 33 +++++++++++++++++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/osf/models/node.py b/osf/models/node.py index fc36c643625..47e69c358c6 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -33,7 +33,7 @@ from framework.auth import oauth_scopes from framework.celery_tasks.handlers import enqueue_task, get_task_from_queue from framework.exceptions import PermissionsError, HTTPError -from framework.sentry import log_exception, log_message +from framework.sentry import log_exception from osf.exceptions import (InvalidTagError, NodeStateError, TagNotFoundError) from .contributor import Contributor @@ -2468,8 +2468,6 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): request = get_current_request() # This is to avoid making multiple requests to GV # within the lifespan of one request on the OSF side - log_message(f'the request is {request}') - log_message(f'the user of the request is {request.user}') try: gv_addons = request.gv_addons except AttributeError: diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index 0020ac565b3..19b8b34b99c 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -12,6 +12,7 @@ from framework.exceptions import HTTPError from api.base.utils import waterbutler_api_url_for +from api.waffle.utils import flag_is_active from website.archiver import ( ARCHIVER_SUCCESS, @@ -35,7 +36,9 @@ AbstractNode, DraftRegistration, ) +from osf import features from osf.utils.requests import get_current_request +from osf.external.gravy_valet import request_helpers, translations def create_app_context(): @@ -108,6 +111,19 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): dst.save() archiver_signals.archive_fail.send(dst, errors=errors) +def get_addon_from_gv(src_node, addon_name, requesting_user): + addon_data = request_helpers.get_addon( + gv_addon_pk=f'{src_node._id}:{addon_name}', + requested_resource=src_node, + requesting_user=requesting_user, + addon_type='configured-storage-addons' + ) + return translations.make_ephemeral_node_settings( + gv_addon_data=addon_data, + requested_resource=src_node, + requesting_user=requesting_user + ) + @celery_app.task(base=ArchiverTask, ignore_result=False) @logged('stat_addon') @@ -128,9 +144,12 @@ def stat_addon(addon_short_name, job_pk): create_app_context() job = ArchiveJob.load(job_pk) src, dst, user = job.info() - request = get_current_request() - request.user = user - src_addon = src.get_addon(addon_name) + + src_addon = None + if addon_name != 'osfstorage' and flag_is_active(get_current_request(), features.ENABLE_GV): + src_addon = get_addon_from_gv(src, addon_name, user) + else: + src_addon = src.get_addon(addon_name) if hasattr(src_addon, 'configured') and not src_addon.configured: # Addon enabled but not configured - no file trees, nothing to archive. return AggregateStatResult(src_addon._id, addon_short_name) @@ -208,9 +227,11 @@ def archive_addon(addon_short_name, job_pk): params['revision'] = 'latest' if addon_short_name.split('-')[-1] == 'draft' else 'latest-published' rename_suffix = ' (draft)' if addon_short_name.split('-')[-1] == 'draft' else ' (published)' addon_short_name = 'dataverse' - request = get_current_request() - request.user = user - src_provider = src.get_addon(addon_short_name) + src_provider = None + if addon_short_name != 'osfstorage' and flag_is_active(get_current_request(), features.ENABLE_GV): + src_provider = get_addon_from_gv(src, addon_short_name, user) + else: + src_provider = src.get_addon(addon_short_name) folder_name_nfd, folder_name_nfc = normalize_unicode_filenames(src_provider.archive_folder_name) rename = f'{folder_name_nfd}{rename_suffix}' url = waterbutler_api_url_for(src._id, addon_short_name, _internal=True, base_url=src.osfstorage_region.waterbutler_url, **params) From 79b4b9cadaee585990dd4ea6c3522e9173f5f246 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Wed, 19 Feb 2025 01:52:44 -0500 Subject: [PATCH 17/22] add _get_fileobj_child_metadata --- osf/external/gravy_valet/translations.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index 69bfac201da..2ff9c48bfe5 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -2,6 +2,9 @@ from dataclasses import asdict, InitVar from typing import TYPE_CHECKING +from framework.exceptions import HTTPError +from rest_framework import status as http_status + import markupsafe from . import request_helpers as gv_requests @@ -252,6 +255,15 @@ def after_fork(self, node: 'Node', fork, user: 'OSFUser', save=True): addon_type=self.gv_data.resource_type ) + def _get_fileobj_child_metadata(self, filenode, user, cookie=None, version=None): + try: + return super()._get_fileobj_child_metadata(filenode, user, cookie=cookie, version=version) + except HTTPError as e: + # The Dataverse API returns a 404 if the dataset has no published files + if self.short_name == 'dataverse' and e.code == http_status.HTTP_404_NOT_FOUND and version == 'latest-published': + return [] + raise + def get_settings_class(addon_type): if addon_type == gv_requests.AddonType.STORAGE: return _get_storage_settings_class() From 608cb755ea9ac045d4f9c2f6d1a8c6a41825ad05 Mon Sep 17 00:00:00 2001 From: Vlad0n20 <137097005+Vlad0n20@users.noreply.github.com> Date: Thu, 20 Feb 2025 18:44:05 +0200 Subject: [PATCH 18/22] [ENG-6526] Add new admins/moderators for a given institution page (#10984) ## Purpose Add a new page - admins/moderators for a given institution ## Changes Add 2 new views in the admin app ## Ticket https://openscience.atlassian.net/browse/ENG-6526 --- admin/institutions/urls.py | 5 ++ admin/institutions/views.py | 73 +++++++++++++++++++ admin/templates/institutions/detail.html | 3 + .../institutions/edit_moderators.html | 63 ++++++++++++++++ 4 files changed, 144 insertions(+) create mode 100644 admin/templates/institutions/edit_moderators.html diff --git a/admin/institutions/urls.py b/admin/institutions/urls.py index bc052be7b83..3b9b3aaf1f9 100644 --- a/admin/institutions/urls.py +++ b/admin/institutions/urls.py @@ -15,4 +15,9 @@ re_path(r'^(?P[0-9]+)/cannot_delete/$', views.CannotDeleteInstitution.as_view(), name='cannot_delete'), re_path(r'^(?P[0-9]+)/nodes/$', views.InstitutionNodeList.as_view(), name='nodes'), re_path(r'^(?P[0-9]+)/register/$', views.InstitutionalMetricsAdminRegister.as_view(), name='register_metrics_admin'), + re_path(r'^(?P[0-9]+)/list_and_add_admin_or_moderator/$', views.InstitutionListAndAddAdminOrModerator.as_view(), + name='list_and_add_admin_or_moderator'), + re_path(r'^(?P[0-9]+)/remove_admins_and_moderators/$', views.InstitutionRemoveAdminOrModerator.as_view(), + name='remove_admins_and_moderators'), + ] diff --git a/admin/institutions/views.py b/admin/institutions/views.py index 2c7286fe242..b83460b6aa6 100644 --- a/admin/institutions/views.py +++ b/admin/institutions/views.py @@ -164,6 +164,79 @@ def get_context_data(self, **kwargs): kwargs.setdefault('logohost', settings.OSF_URL) return super().get_context_data(**kwargs) +class InstitutionAdminAndModeratorBaseView(PermissionRequiredMixin, ListView): + permission_required = 'osf.change_institution' + template_name = 'institutions/edit_moderators.html' + raise_exception = True + + def get_queryset(self): + return Institution.objects.get(id=self.kwargs['institution_id']) + + def get_context_data(self, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + context = super().get_context_data(**kwargs) + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + context['institution'] = institution + context['admins'] = admin_group.user_set.all() + context['moderators'] = institution.contributors.exclude(id__in=admin_group.user_set.values_list('id', flat=True)) + return context + + +class InstitutionListAndAddAdminOrModerator(InstitutionAdminAndModeratorBaseView): + + def get_permission_required(self): + if self.request.method == 'GET': + return ('osf.view_institution',) + return (self.permission_required,) + + def post(self, request, *args, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + data = dict(request.POST) + del data['csrfmiddlewaretoken'] # just to remove the key from the form dict + + target_user = OSFUser.load(data['add-moderators-form'][0]) + if target_user is None: + messages.error(request, f'User for guid: {data["add-moderators-form"][0]} could not be found') + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) + + if 'admin' in data: + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + admin_group.user_set.add(target_user) + target_type = 'admin' + else: + institution.contributors.add(target_user) + target_type = 'moderator' + + messages.success(request, f'The following {target_type} was successfully added: {target_user.fullname} ({target_user.username})') + + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) + +class InstitutionRemoveAdminOrModerator(InstitutionAdminAndModeratorBaseView): + + def post(self, request, *args, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + data = dict(request.POST) + del data['csrfmiddlewaretoken'] # just to remove the key from the form dict + + to_be_removed = list(data.keys()) + removed_admins = [admin.replace('Admin-', '') for admin in to_be_removed if 'Admin-' in admin] + removed_moderators = [moderator.replace('Moderator-', '') for moderator in to_be_removed if + 'Moderator-' in moderator] + moderators = OSFUser.objects.filter(id__in=removed_moderators) + admins = OSFUser.objects.filter(id__in=removed_admins) + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + admin_group.user_set.remove(*admins) + institution.contributors.remove(*moderators) + + if moderators: + moderator_names = ' ,'.join(moderators.values_list('fullname', flat=True)) + messages.success(request, f'The following moderators were successfully removed: {moderator_names}') + + if admins: + admin_names = ' ,'.join(admins.values_list('fullname', flat=True)) + messages.success(request, f'The following admins were successfully removed: {admin_names}') + + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) class DeleteInstitution(PermissionRequiredMixin, DeleteView): permission_required = 'osf.delete_institution' diff --git a/admin/templates/institutions/detail.html b/admin/templates/institutions/detail.html index 0f83e361ac7..196b6559e4f 100644 --- a/admin/templates/institutions/detail.html +++ b/admin/templates/institutions/detail.html @@ -59,6 +59,9 @@

Banner:

Create Moderator/Admin
+
+ Contributors list +
{% endif %} diff --git a/admin/templates/institutions/edit_moderators.html b/admin/templates/institutions/edit_moderators.html new file mode 100644 index 00000000000..ddef91f5ef4 --- /dev/null +++ b/admin/templates/institutions/edit_moderators.html @@ -0,0 +1,63 @@ +{% extends "base.html" %} +{% load static %} +{% load render_bundle from webpack_loader %} +{% block title %} + Institution contributors +{% endblock title %} +{% block content %} +
+
+ {% if messages %} +
    + {% for message in messages %} + {{ message }} + {% endfor %} +
+ {% endif %} +
+
+
+

{{ institution.name }}

+
+
+
+
+
+ {% csrf_token %} + + + + +
+
+
+
+
+
+
+ {% csrf_token %} + + + + + {% for moderator in moderators %} + + + + + + {% endfor %} + {% for admin in admins %} + + + + + + {% endfor %} +
NameType
{{ moderator.fullname }} ({{moderator.username}})Moderator
{{ admin.fullname }} ({{admin.username}})Admin
+ +
+
+
+
+{% endblock content %} \ No newline at end of file From 1d5c7e4954084666321413c5314cac87e4a94675 Mon Sep 17 00:00:00 2001 From: "Brian J. Geiger" Date: Thu, 20 Feb 2025 12:45:59 -0500 Subject: [PATCH 19/22] =?UTF-8?q?Revert=20"[ENG-6526]=20Add=20new=20admins?= =?UTF-8?q?/moderators=20for=20a=20given=20institution=20page=20(#1?= =?UTF-8?q?=E2=80=A6"=20(#10989)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 608cb755ea9ac045d4f9c2f6d1a8c6a41825ad05. --- admin/institutions/urls.py | 5 -- admin/institutions/views.py | 73 ------------------- admin/templates/institutions/detail.html | 3 - .../institutions/edit_moderators.html | 63 ---------------- 4 files changed, 144 deletions(-) delete mode 100644 admin/templates/institutions/edit_moderators.html diff --git a/admin/institutions/urls.py b/admin/institutions/urls.py index 3b9b3aaf1f9..bc052be7b83 100644 --- a/admin/institutions/urls.py +++ b/admin/institutions/urls.py @@ -15,9 +15,4 @@ re_path(r'^(?P[0-9]+)/cannot_delete/$', views.CannotDeleteInstitution.as_view(), name='cannot_delete'), re_path(r'^(?P[0-9]+)/nodes/$', views.InstitutionNodeList.as_view(), name='nodes'), re_path(r'^(?P[0-9]+)/register/$', views.InstitutionalMetricsAdminRegister.as_view(), name='register_metrics_admin'), - re_path(r'^(?P[0-9]+)/list_and_add_admin_or_moderator/$', views.InstitutionListAndAddAdminOrModerator.as_view(), - name='list_and_add_admin_or_moderator'), - re_path(r'^(?P[0-9]+)/remove_admins_and_moderators/$', views.InstitutionRemoveAdminOrModerator.as_view(), - name='remove_admins_and_moderators'), - ] diff --git a/admin/institutions/views.py b/admin/institutions/views.py index b83460b6aa6..2c7286fe242 100644 --- a/admin/institutions/views.py +++ b/admin/institutions/views.py @@ -164,79 +164,6 @@ def get_context_data(self, **kwargs): kwargs.setdefault('logohost', settings.OSF_URL) return super().get_context_data(**kwargs) -class InstitutionAdminAndModeratorBaseView(PermissionRequiredMixin, ListView): - permission_required = 'osf.change_institution' - template_name = 'institutions/edit_moderators.html' - raise_exception = True - - def get_queryset(self): - return Institution.objects.get(id=self.kwargs['institution_id']) - - def get_context_data(self, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - context = super().get_context_data(**kwargs) - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - context['institution'] = institution - context['admins'] = admin_group.user_set.all() - context['moderators'] = institution.contributors.exclude(id__in=admin_group.user_set.values_list('id', flat=True)) - return context - - -class InstitutionListAndAddAdminOrModerator(InstitutionAdminAndModeratorBaseView): - - def get_permission_required(self): - if self.request.method == 'GET': - return ('osf.view_institution',) - return (self.permission_required,) - - def post(self, request, *args, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - data = dict(request.POST) - del data['csrfmiddlewaretoken'] # just to remove the key from the form dict - - target_user = OSFUser.load(data['add-moderators-form'][0]) - if target_user is None: - messages.error(request, f'User for guid: {data["add-moderators-form"][0]} could not be found') - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) - - if 'admin' in data: - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - admin_group.user_set.add(target_user) - target_type = 'admin' - else: - institution.contributors.add(target_user) - target_type = 'moderator' - - messages.success(request, f'The following {target_type} was successfully added: {target_user.fullname} ({target_user.username})') - - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) - -class InstitutionRemoveAdminOrModerator(InstitutionAdminAndModeratorBaseView): - - def post(self, request, *args, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - data = dict(request.POST) - del data['csrfmiddlewaretoken'] # just to remove the key from the form dict - - to_be_removed = list(data.keys()) - removed_admins = [admin.replace('Admin-', '') for admin in to_be_removed if 'Admin-' in admin] - removed_moderators = [moderator.replace('Moderator-', '') for moderator in to_be_removed if - 'Moderator-' in moderator] - moderators = OSFUser.objects.filter(id__in=removed_moderators) - admins = OSFUser.objects.filter(id__in=removed_admins) - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - admin_group.user_set.remove(*admins) - institution.contributors.remove(*moderators) - - if moderators: - moderator_names = ' ,'.join(moderators.values_list('fullname', flat=True)) - messages.success(request, f'The following moderators were successfully removed: {moderator_names}') - - if admins: - admin_names = ' ,'.join(admins.values_list('fullname', flat=True)) - messages.success(request, f'The following admins were successfully removed: {admin_names}') - - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) class DeleteInstitution(PermissionRequiredMixin, DeleteView): permission_required = 'osf.delete_institution' diff --git a/admin/templates/institutions/detail.html b/admin/templates/institutions/detail.html index 196b6559e4f..0f83e361ac7 100644 --- a/admin/templates/institutions/detail.html +++ b/admin/templates/institutions/detail.html @@ -59,9 +59,6 @@

Banner:

Create Moderator/Admin
-
- Contributors list -
{% endif %} diff --git a/admin/templates/institutions/edit_moderators.html b/admin/templates/institutions/edit_moderators.html deleted file mode 100644 index ddef91f5ef4..00000000000 --- a/admin/templates/institutions/edit_moderators.html +++ /dev/null @@ -1,63 +0,0 @@ -{% extends "base.html" %} -{% load static %} -{% load render_bundle from webpack_loader %} -{% block title %} - Institution contributors -{% endblock title %} -{% block content %} -
-
- {% if messages %} -
    - {% for message in messages %} - {{ message }} - {% endfor %} -
- {% endif %} -
-
-
-

{{ institution.name }}

-
-
-
-
-
- {% csrf_token %} - - - - -
-
-
-
-
-
-
- {% csrf_token %} - - - - - {% for moderator in moderators %} - - - - - - {% endfor %} - {% for admin in admins %} - - - - - - {% endfor %} -
NameType
{{ moderator.fullname }} ({{moderator.username}})Moderator
{{ admin.fullname }} ({{admin.username}})Admin
- -
-
-
-
-{% endblock content %} \ No newline at end of file From 029eeefc54dc24a5627a89a8e7702db5164fd11c Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Mon, 24 Feb 2025 11:31:09 -0500 Subject: [PATCH 20/22] add CHANGELOG. Bump version no. --- CHANGELOG | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 680422ec889..4d6b889e04f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,10 @@ We follow the CalVer (https://calver.org/) versioning scheme: YY.MINOR.MICRO. +25.03.0 (2025-02-24) +==================== +- Addons and GV - BE Release + 25.02.0 (2025-01-27) ==================== - DOI Versioning - BE Release diff --git a/package.json b/package.json index 24ec2c5e63e..131c419878c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "OSF", - "version": "25.02.0", + "version": "25.03.0", "description": "Facilitating Open Science", "repository": "https://github.com/CenterForOpenScience/osf.io", "author": "Center for Open Science", From af2cf5a52d15065403ca91dec7d713e27878f92f Mon Sep 17 00:00:00 2001 From: Matt Frazier Date: Mon, 24 Feb 2025 17:23:48 -0500 Subject: [PATCH 21/22] Enable gravy feature flag by default - Disable refresh_addon_tokens by default --- osf/features.yaml | 2 +- website/settings/defaults.py | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/osf/features.yaml b/osf/features.yaml index 9872b142c2a..ab9cb0927bf 100644 --- a/osf/features.yaml +++ b/osf/features.yaml @@ -17,7 +17,7 @@ flags: note: This is used to enable GravyValet, the system responible for addons, this will remove the files widget on the project overview page. Will be used with EMBER_USER_SETTINGS_ADDONS and EMBER_NODE_SETTINGS_ADDONS to flip all UI elements to the new addons system. - everyone: null + everyone: true - flag_name: EMBER_FILE_PROJECT_DETAIL name: ember_file_project_detail_page diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 1bb36e6ce3e..b2d20d21e4a 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -584,15 +584,15 @@ class CeleryConfig: 'schedule': crontab(minute=0, hour=5), # Daily at 12 a.m. EST 'args': ('email_digest',), }, - 'refresh_addons': { - 'task': 'scripts.refresh_addon_tokens', - 'schedule': crontab(minute=0, hour=7), # Daily 2:00 a.m - 'kwargs': {'dry_run': False, 'addons': { - 'box': 60, # https://docs.box.com/docs/oauth-20#section-6-using-the-access-and-refresh-tokens - 'googledrive': 14, # https://developers.google.com/identity/protocols/OAuth2#expiration - 'mendeley': 14 # http://dev.mendeley.com/reference/topics/authorization_overview.html - }}, - }, + # 'refresh_addons': { # Handled by GravyValet now + # 'task': 'scripts.refresh_addon_tokens', + # 'schedule': crontab(minute=0, hour=7), # Daily 2:00 a.m + # 'kwargs': {'dry_run': False, 'addons': { + # 'box': 60, # https://docs.box.com/docs/oauth-20#section-6-using-the-access-and-refresh-tokens + # 'googledrive': 14, # https://developers.google.com/identity/protocols/OAuth2#expiration + # 'mendeley': 14 # http://dev.mendeley.com/reference/topics/authorization_overview.html + # }}, + # }, 'retract_registrations': { 'task': 'scripts.retract_registrations', 'schedule': crontab(minute=0, hour=5), # Daily 12 a.m From 6e00472a719cb151fdb6130759e21edc8878d507 Mon Sep 17 00:00:00 2001 From: Matt Frazier Date: Mon, 24 Feb 2025 19:52:56 -0500 Subject: [PATCH 22/22] Add default EphemeralUserSettings.public_id --- osf/external/gravy_valet/translations.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index 2ff9c48bfe5..d43740ebbdf 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -114,6 +114,10 @@ def gv_id(self): def can_be_merged(self): return True + @property + def public_id(self): + return None + @dataclasses.dataclass class EphemeralNodeSettings: """Minimalist dataclass for storing/translating the actually used properties of NodeSettings."""