From 46cf216f8006e4c2651191402580899c4aa88f93 Mon Sep 17 00:00:00 2001 From: Jacob Lowe <40873986+jalowe13@users.noreply.github.com> Date: Wed, 4 Dec 2024 15:35:46 -0600 Subject: [PATCH 1/4] Task/WP-726: Mutation hook: Mkdir (#997) * initial conversion to ts * liniting/formating * dispatch type change * extra dispatch removals * 500 error fix * removed redux test --- .../tests/DataFilesMkdirModal.test.jsx | 15 +--- .../src/hooks/datafiles/mutations/useMkdir.js | 34 ------- .../src/hooks/datafiles/mutations/useMkdir.ts | 89 +++++++++++++++++++ 3 files changed, 90 insertions(+), 48 deletions(-) delete mode 100644 client/src/hooks/datafiles/mutations/useMkdir.js create mode 100644 client/src/hooks/datafiles/mutations/useMkdir.ts diff --git a/client/src/components/DataFiles/DataFilesModals/tests/DataFilesMkdirModal.test.jsx b/client/src/components/DataFiles/DataFilesModals/tests/DataFilesMkdirModal.test.jsx index b1d3425ea..677e6817a 100644 --- a/client/src/components/DataFiles/DataFilesModals/tests/DataFilesMkdirModal.test.jsx +++ b/client/src/components/DataFiles/DataFilesModals/tests/DataFilesMkdirModal.test.jsx @@ -55,20 +55,7 @@ describe('DataFilesCopyModal', () => { const submitButton = getByText('Create Folder'); fireEvent.click(submitButton); }); - - expect(store.getActions()).toEqual([ - { - type: 'DATA_FILES_MKDIR', - payload: { - api: 'tapis', - scheme: 'private', - system: 'test.system', - path: '/', - dirname: 'abc123', - reloadCallback: expect.any(Function), - }, - }, - ]); + // TODO: New test needed for react redux call for mkdir }); it('Error message on invalid input', async () => { diff --git a/client/src/hooks/datafiles/mutations/useMkdir.js b/client/src/hooks/datafiles/mutations/useMkdir.js deleted file mode 100644 index c741ee192..000000000 --- a/client/src/hooks/datafiles/mutations/useMkdir.js +++ /dev/null @@ -1,34 +0,0 @@ -import { useSelector, useDispatch, shallowEqual } from 'react-redux'; - -function useMkdir() { - const dispatch = useDispatch(); - const status = useSelector( - (state) => state.files.operationStatus.mkdir, - shallowEqual - ); - - const setStatus = (newStatus) => { - dispatch({ - type: 'DATA_FILES_SET_OPERATION_STATUS', - payload: { status: newStatus, operation: 'mkdir' }, - }); - }; - - const mkdir = ({ api, scheme, system, path, dirname, reloadCallback }) => { - dispatch({ - type: 'DATA_FILES_MKDIR', - payload: { - api, - scheme, - system, - path, - dirname, - reloadCallback, - }, - }); - }; - - return { mkdir, status, setStatus }; -} - -export default useMkdir; diff --git a/client/src/hooks/datafiles/mutations/useMkdir.ts b/client/src/hooks/datafiles/mutations/useMkdir.ts new file mode 100644 index 000000000..0f2bc122a --- /dev/null +++ b/client/src/hooks/datafiles/mutations/useMkdir.ts @@ -0,0 +1,89 @@ +import { useSelector, useDispatch, shallowEqual } from 'react-redux'; +import { useMutation } from '@tanstack/react-query'; +import { apiClient } from 'utils/apiClient'; + +export async function mkdirUtil({ + api, + scheme, + system, + path, + dirname, +}: { + api: string; + scheme: string; + system: string; + path: string; + dirname: string; +}): Promise<{ name: string; path: string }> { + let apiPath = !path || path[0] === '/' ? path : `/${path}`; + if (apiPath === '/') { + apiPath = ''; + } + let url = `/api/datafiles/${api}/mkdir/${scheme}/${system}/${apiPath}/`; + url = url.replace(/\/{2,}/g, '/'); + const response = await apiClient.put<{ name: string; path: string }>(url, { + dir_name: dirname, + }); + + return response.data; +} + +function useMkdir() { + const dispatch = useDispatch(); + const status = useSelector( + (state: any) => state.files.operationStatus.mkdir, + shallowEqual + ); + + const setStatus = (newStatus: any) => { + dispatch({ + type: 'DATA_FILES_SET_OPERATION_STATUS', + payload: { status: newStatus, operation: 'mkdir' }, + }); + }; + + const { mutate } = useMutation({ mutationFn: mkdirUtil }); + + const mkdir = ({ + api, + scheme, + system, + path, + dirname, + reloadCallback, + }: { + api: string; + scheme: string; + system: string; + path: string; + dirname: string; + reloadCallback: any; + }) => { + mutate( + { + api, + scheme, + system, + path, + dirname, + }, + { + onSuccess: () => { + dispatch({ + type: 'DATA_FILES_TOGGLE_MODAL', + payload: { + operation: 'mkdir', + props: {}, + }, + }); + reloadCallback(); + }, + onError: () => {}, + } + ); + }; + + return { mkdir, status, setStatus }; +} + +export default useMkdir; From 51e2230f3fca51a1e39104b3a7216d0d154d8f38 Mon Sep 17 00:00:00 2001 From: Jacob Lowe <40873986+jalowe13@users.noreply.github.com> Date: Wed, 4 Dec 2024 15:39:24 -0600 Subject: [PATCH 2/4] Task/WP-727: Mutation hook: Upload file (#999) * initial ts convert * for each file and manual headers * removed unused dispatch * axios replacing fetch * mutateAsync --------- Co-authored-by: Jake Rosenberg --- .../hooks/datafiles/mutations/useUpload.js | 32 ----- .../hooks/datafiles/mutations/useUpload.ts | 119 ++++++++++++++++++ 2 files changed, 119 insertions(+), 32 deletions(-) delete mode 100644 client/src/hooks/datafiles/mutations/useUpload.js create mode 100644 client/src/hooks/datafiles/mutations/useUpload.ts diff --git a/client/src/hooks/datafiles/mutations/useUpload.js b/client/src/hooks/datafiles/mutations/useUpload.js deleted file mode 100644 index 3cdb9fd22..000000000 --- a/client/src/hooks/datafiles/mutations/useUpload.js +++ /dev/null @@ -1,32 +0,0 @@ -import { useSelector, useDispatch, shallowEqual } from 'react-redux'; - -function useUpload() { - const dispatch = useDispatch(); - const status = useSelector( - (state) => state.files.operationStatus.upload, - shallowEqual - ); - - const setStatus = (newStatus) => { - dispatch({ - type: 'DATA_FILES_SET_OPERATION_STATUS', - payload: { status: newStatus, operation: 'upload' }, - }); - }; - - const upload = ({ system, path, files, reloadCallback }) => { - dispatch({ - type: 'DATA_FILES_UPLOAD', - payload: { - system, - path, - files, - reloadCallback, - }, - }); - }; - - return { upload, status, setStatus }; -} - -export default useUpload; diff --git a/client/src/hooks/datafiles/mutations/useUpload.ts b/client/src/hooks/datafiles/mutations/useUpload.ts new file mode 100644 index 000000000..c06d2dfa7 --- /dev/null +++ b/client/src/hooks/datafiles/mutations/useUpload.ts @@ -0,0 +1,119 @@ +import { useSelector, useDispatch, shallowEqual } from 'react-redux'; +import { apiClient } from 'utils/apiClient'; +import Cookies from 'js-cookie'; +import truncateMiddle from 'utils/truncateMiddle'; +import { useMutation } from '@tanstack/react-query'; + +export async function uploadUtil({ + api, + scheme, + system, + path, + file, +}: { + api: string; + scheme: string; + system: string; + path: string; + file: FormData; +}): Promise<{ file: any; path: string }> { + let apiPath = !path || path[0] === '/' ? path : `/${path}`; + if (apiPath === '/') { + apiPath = ''; + return { file, path: apiPath }; + } + const formData = new FormData(); + const fileField = file.get('uploaded_file') as Blob; + formData.append('uploaded_file', fileField); + let url = `/api/datafiles/${api}/upload/${scheme}/${system}/${apiPath}/`; + url.replace(/\/{2,}/g, '/'); + const response = await apiClient.post(url, formData, { + headers: { + 'X-CSRFToken': Cookies.get('csrftoken') || '', + }, + withCredentials: true, + }); + return response.data; +} + +function useUpload() { + const dispatch = useDispatch(); + const status = useSelector( + (state: any) => state.files.operationStatus.upload, + shallowEqual + ); + + const setStatus = (newStatus: any) => { + dispatch({ + type: 'DATA_FILES_SET_OPERATION_STATUS', + payload: { status: newStatus, operation: 'upload' }, + }); + }; + + const { mutateAsync } = useMutation({ mutationFn: uploadUtil }); + + const upload = ({ + system, + path, + files, + reloadCallback, + }: { + system: string; + path: string; + files: { data: File; id: string }[]; + reloadCallback: () => void; + }) => { + const api = 'tapis'; + const scheme = 'private'; + const uploadCalls: Promise[] = files.map((fileObj) => { + const { data: file, id: index } = fileObj; + dispatch({ + type: 'DATA_FILES_SET_OPERATION_STATUS_BY_KEY', + payload: { status: 'UPLOADING', key: index, operation: 'upload' }, + }); + const formData = new FormData(); + formData.append('uploaded_file', file); + return mutateAsync( + { + api, + scheme, + system, + path, + file: formData, + }, + { + onSuccess: () => { + dispatch({ + type: 'DATA_FILES_SET_OPERATION_STATUS_BY_KEY', + payload: { status: 'SUCCESS', key: index, operation: 'upload' }, + }); + }, + onError: (error) => { + console.log('The Error is', error); + dispatch({ + type: 'DATA_FILES_SET_OPERATION_STATUS_BY_KEY', + payload: { status: 'ERROR', key: index, operation: 'upload' }, + }); + }, + } + ); + }); + Promise.all(uploadCalls).then(() => { + dispatch({ + type: 'DATA_FILES_TOGGLE_MODAL', + payload: { operation: 'upload', props: {} }, + }); + dispatch({ + type: 'ADD_TOAST', + payload: { + message: `${ + files.length > 1 ? `${files.length} files` : 'File' + } uploaded to ${truncateMiddle(path, 20) || '/'}`, + }, + }); + reloadCallback(); + }); + }; + return { upload, status, setStatus }; +} +export default useUpload; From 84d30e510925d15db2e919d609ac01df3b6f60fe Mon Sep 17 00:00:00 2001 From: Jacob Lowe <40873986+jalowe13@users.noreply.github.com> Date: Thu, 5 Dec 2024 10:38:14 -0600 Subject: [PATCH 3/4] Task/WP-68: Update DataFiles Unit Tests (#992) * WP-68: Initial update of datafiles unit tests * WP-68: Investigation into System Definition and State * showViewPath Coverage * unauth init * attribute error test coverage * put unauth request test * post unauth and handler exception tests * LinkView Exception Tests * post handler exception coverage fix * restore original mocks * linting --- .../DataFilesListing.test.jsx | 99 +++++++++++++++++ .../DataFilesSidebar.test.jsx | 2 +- .../DataFilesSystemSelector.test.jsx | 2 +- .../fixtures/DataFiles.systems.fixture.js | 35 ++++-- .../DataFiles/tests/DataFiles.test.jsx | 3 +- server/portal/apps/auth/views_unit_test.py | 41 ++++--- server/portal/apps/datafiles/views.py | 4 +- .../portal/apps/datafiles/views_unit_test.py | 105 +++++++++++++++++- 8 files changed, 258 insertions(+), 33 deletions(-) diff --git a/client/src/components/DataFiles/DataFilesListing/DataFilesListing.test.jsx b/client/src/components/DataFiles/DataFilesListing/DataFilesListing.test.jsx index 6a75abfd5..3d4a3ea9a 100644 --- a/client/src/components/DataFiles/DataFilesListing/DataFilesListing.test.jsx +++ b/client/src/components/DataFiles/DataFilesListing/DataFilesListing.test.jsx @@ -324,3 +324,102 @@ describe('DataFilesListing - Section Name Determination', () => { ).toBeInTheDocument(); }); }); +describe('DataFilesListing - showViewPath', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + it('renders the "Path" column when showViewPath is true', () => { + const testfile = { + system: 'test.system', + path: '/path/to/file', + name: 'testfile', + format: 'file', + length: 4096, + lastModified: '2019-06-17T15:49:53-05:00', + _links: { self: { href: 'href.test' } }, + }; + const history = createMemoryHistory(); + history.push('/workbench/data/tapis/private/test.system/'); + const store = mockStore({ + ...initialMockState, + files: { + ...initialMockState.files, + listing: { FilesListing: [testfile] }, + }, + workbench: { + config: { + viewPath: true, + }, + }, + }); + // Spy on useMemo to capture the cells array + const useMemoSpy = vi + .spyOn(React, 'useMemo') + .mockImplementation((fn) => fn()); + const { getByText } = renderComponent( + , + store, + history + ); + // Path cell is added + expect(getByText('Path')).toBeDefined(); + // Check the length of the cells array + const cellsArray = useMemoSpy.mock.results.find((result) => + Array.isArray(result.value) + ).value; + expect(cellsArray.length).toBe(6); + }); + it('does not render the "Path" column when showViewPath is false', () => { + const testfile = { + system: 'test.system', + path: '/path/to/file', + name: 'testfile', + format: 'file', + length: 4096, + lastModified: '2019-06-17T15:49:53-05:00', + _links: { self: { href: 'href.test' } }, + }; + const history = createMemoryHistory(); + history.push('/workbench/data/tapis/private/test.system/'); + const store = mockStore({ + ...initialMockState, + files: { + ...initialMockState.files, + listing: { FilesListing: [testfile] }, + }, + workbench: { + config: { + viewPath: false, + }, + }, + }); + // Spy on useMemo to capture the cells array + const useMemoSpy = vi + .spyOn(React, 'useMemo') + .mockImplementation((fn) => fn()); + const { queryByText } = renderComponent( + , + store, + history + ); + // Path should not exist as a new cell + expect(queryByText('Path')).toBeNull(); + // Check the length of the cells array + const cellsArray = useMemoSpy.mock.results.find((result) => + Array.isArray(result.value) + ).value; + expect(cellsArray.length).toBe(5); + }); +}); diff --git a/client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.test.jsx b/client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.test.jsx index 36eb90b2c..8cf95930b 100644 --- a/client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.test.jsx +++ b/client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.test.jsx @@ -62,7 +62,7 @@ describe('DataFilesSidebar', () => { ).toEqual( '/workbench/data/tapis/private/longhorn.home.username/home/username/' ); - expect(queryByText(/My Data \(Work\)/)).toBeNull(); + expect(queryByText(/My Data \(Work\)/)).toBeDefined(); }); it('disables creating new shared workspaces in read only shared workspaces', async () => { diff --git a/client/src/components/DataFiles/DataFilesSystemSelector/DataFilesSystemSelector.test.jsx b/client/src/components/DataFiles/DataFilesSystemSelector/DataFilesSystemSelector.test.jsx index 5aaaeb155..5e5312b78 100644 --- a/client/src/components/DataFiles/DataFilesSystemSelector/DataFilesSystemSelector.test.jsx +++ b/client/src/components/DataFiles/DataFilesSystemSelector/DataFilesSystemSelector.test.jsx @@ -31,7 +31,7 @@ describe('DataFilesSystemSelector', () => { store, history ); - expect(queryByText(/My Data \(Work\)/)).toBeNull(); + expect(queryByText(/My Data \(Work\)/)).toBeDefined(); expect(queryByText(/My Data \(Frontera\)/)).toBeDefined(); expect(queryByText(/My Data \(Longhorn\)/)).toBeDefined(); expect(queryByText(/Google Drive/)).toBeDefined(); diff --git a/client/src/components/DataFiles/fixtures/DataFiles.systems.fixture.js b/client/src/components/DataFiles/fixtures/DataFiles.systems.fixture.js index d1a019d07..bed61ad27 100644 --- a/client/src/components/DataFiles/fixtures/DataFiles.systems.fixture.js +++ b/client/src/components/DataFiles/fixtures/DataFiles.systems.fixture.js @@ -1,5 +1,7 @@ /* TODOv3 update this fixture https://jira.tacc.utexas.edu/browse/WP-68*/ - +// Updated fixture changes from endpoint https://cep.test/api/datafiles/systems/list/ +// Removed from configuration: hidden, keyservice +// Removed from storage and defintions array: errorMessage, loading const systemsFixture = { storage: { configuration: [ @@ -9,10 +11,8 @@ const systemsFixture = { scheme: 'private', api: 'tapis', icon: null, - hidden: true, homeDir: '/home/username', default: true, - keyservice: true, }, { name: 'My Data (Frontera)', @@ -65,13 +65,30 @@ const systemsFixture = { integration: 'portal.apps.googledrive_integration', }, ], - error: false, - errorMessage: null, - loading: false, + /* + * The following needs to be mirrored for the storage and definitions + + These are included in the datafiles reducers but pass tests without these + This means that tests need to be more comprehensive to catch this or removed + + Definitions that use variables other than list are used in: + - DataFilesTable.jsx:45 for error + + state.systems.definitions.* is not called for anything else other than error + These would need to be removed then + - errorMessage + - loading + */ + + //error: false, + //errorMessage: null, + //loading: false, defaultHost: 'frontera.tacc.utexas.edu', defaultSystem: 'frontera', }, + // This definitions is required for the tests, some can be removed. Referencing datafiles.reducers.js definitions: { + // For DataFilesTable and DataFilesShowPathModal it requires the id from this list list: [ { id: 'frontera.home.username', @@ -90,9 +107,9 @@ const systemsFixture = { effectiveUserId: 'username', }, ], - error: false, - errorMessage: null, - loading: false, + error: false, // Commenting this out results in an error + //errorMessage: null, + //loading: false, }, }; diff --git a/client/src/components/DataFiles/tests/DataFiles.test.jsx b/client/src/components/DataFiles/tests/DataFiles.test.jsx index bdcc70118..8436914bf 100644 --- a/client/src/components/DataFiles/tests/DataFiles.test.jsx +++ b/client/src/components/DataFiles/tests/DataFiles.test.jsx @@ -49,7 +49,7 @@ describe('DataFiles', () => { //); expect(getAllByText(/My Data \(Frontera\)/)).toBeDefined(); expect(getByText(/My Data \(Longhorn\)/)).toBeDefined(); - expect(queryByText(/My Data \(Work\)/)).toBeNull(); + expect(queryByText(/My Data \(Work\)/)).toBeDefined(); // Changed to defined, hidden attribute removed and would be defined by default }); it('should not render Data Files with no systems', () => { @@ -62,6 +62,7 @@ describe('DataFiles', () => { }, }, systems: { + // TODO: Remove rest of unused variables storage: { configuration: [ { diff --git a/server/portal/apps/auth/views_unit_test.py b/server/portal/apps/auth/views_unit_test.py index 5a9aa8815..22abc6f88 100644 --- a/server/portal/apps/auth/views_unit_test.py +++ b/server/portal/apps/auth/views_unit_test.py @@ -1,6 +1,7 @@ -from django.conf import settings import pytest +from django.conf import settings from django.urls import reverse + from portal.apps.auth.views import launch_setup_checks TEST_STATE = "ABCDEFG123456" @@ -9,33 +10,39 @@ def test_auth_tapis(client, mocker): - mocker.patch('portal.apps.auth.views._get_auth_state', return_value=TEST_STATE) + mocker.patch("portal.apps.auth.views._get_auth_state", return_value=TEST_STATE) response = client.get("/auth/tapis/", follow=False) - tapis_authorize = f"{settings.TAPIS_TENANT_BASEURL}/v3/oauth2/authorize" \ + tapis_authorize = ( + f"{settings.TAPIS_TENANT_BASEURL}/v3/oauth2/authorize" f"?client_id=test&redirect_uri=http://testserver/auth/tapis/callback/&response_type=code&state={TEST_STATE}" + ) assert response.status_code == 302 assert response.url == tapis_authorize - assert client.session['auth_state'] == TEST_STATE + assert client.session["auth_state"] == TEST_STATE def test_tapis_callback(client, mocker, regular_user, tapis_tokens_create_mock): - mock_authenticate = mocker.patch('portal.apps.auth.views.authenticate') - mock_tapis_token_post = mocker.patch('portal.apps.auth.views.requests.post') - mock_launch_setup_checks = mocker.patch('portal.apps.auth.views.launch_setup_checks') + mock_authenticate = mocker.patch("portal.apps.auth.views.authenticate") + mock_tapis_token_post = mocker.patch("portal.apps.auth.views.requests.post") + mock_launch_setup_checks = mocker.patch( + "portal.apps.auth.views.launch_setup_checks" + ) # add auth to session session = client.session - session['auth_state'] = TEST_STATE + session["auth_state"] = TEST_STATE session.save() mock_tapis_token_post.return_value.json.return_value = tapis_tokens_create_mock mock_tapis_token_post.return_value.status_code = 200 mock_authenticate.return_value = regular_user - response = client.get(f"/auth/tapis/callback/?state={TEST_STATE}&code=83163624a0bc41c4a376e0acb16a62f9") + response = client.get( + f"/auth/tapis/callback/?state={TEST_STATE}&code=83163624a0bc41c4a376e0acb16a62f9" + ) assert response.status_code == 302 assert response.url == settings.LOGIN_REDIRECT_URL assert mock_launch_setup_checks.call_count == 1 @@ -44,31 +51,35 @@ def test_tapis_callback(client, mocker, regular_user, tapis_tokens_create_mock): def test_tapis_callback_no_code(client): # add auth to session session = client.session - session['auth_state'] = TEST_STATE + session["auth_state"] = TEST_STATE session.save() response = client.get(f"/auth/tapis/callback/?state={TEST_STATE}") assert response.status_code == 302 - assert response.url == reverse('portal_accounts:logout') + assert response.url == reverse("portal_accounts:logout") def test_tapis_callback_mismatched_state(client): # add auth to session session = client.session - session['auth_state'] = "TEST_STATE" + session["auth_state"] = "TEST_STATE" session.save() response = client.get("/auth/tapis/callback/?state=bar") assert response.status_code == 400 def test_launch_setup_checks(regular_user, mocker): - mock_execute_setup_steps = mocker.patch('portal.apps.auth.views.execute_setup_steps') + mock_execute_setup_steps = mocker.patch( + "portal.apps.auth.views.execute_setup_steps" + ) launch_setup_checks(regular_user) - mock_execute_setup_steps.apply_async.assert_called_with(args=[regular_user.username]) + mock_execute_setup_steps.apply_async.assert_called_with( + args=[regular_user.username] + ) def test_launch_setup_checks_already_onboarded(regular_user, mocker): regular_user.profile.setup_complete = True - mock_index_allocations = mocker.patch('portal.apps.auth.views.index_allocations') + mock_index_allocations = mocker.patch("portal.apps.auth.views.index_allocations") launch_setup_checks(regular_user) mock_index_allocations.apply_async.assert_called_with(args=[regular_user.username]) diff --git a/server/portal/apps/datafiles/views.py b/server/portal/apps/datafiles/views.py index 69fa02fdb..181115272 100644 --- a/server/portal/apps/datafiles/views.py +++ b/server/portal/apps/datafiles/views.py @@ -137,7 +137,7 @@ def put(self, request, operation=None, scheme=None, try: client = request.user.tapis_oauth.client except AttributeError: - return HttpResponseForbidden + return HttpResponseForbidden("This data requires authentication to view.") try: METRICS.info("user:{} op:{} api:tapis scheme:{} " @@ -160,7 +160,7 @@ def post(self, request, operation=None, scheme=None, try: client = request.user.tapis_oauth.client except AttributeError: - return HttpResponseForbidden() + return HttpResponseForbidden("This data requires authentication to upload.") try: METRICS.info("user:{} op:{} api:tapis scheme:{} " diff --git a/server/portal/apps/datafiles/views_unit_test.py b/server/portal/apps/datafiles/views_unit_test.py index 85b3e11f1..85f1bf42a 100644 --- a/server/portal/apps/datafiles/views_unit_test.py +++ b/server/portal/apps/datafiles/views_unit_test.py @@ -1,13 +1,14 @@ -import pytest import json import logging import os -from mock import patch, MagicMock -from tapipy.errors import InternalServerError -from portal.apps.datafiles.models import Link + +import pytest from django.conf import settings +from mock import MagicMock, patch +from tapipy.errors import InternalServerError from tapipy.tapis import TapisResult +from portal.apps.datafiles.models import Link pytestmark = pytest.mark.django_db @@ -129,6 +130,21 @@ def test_link_post(postits_create, authenticated_user, client): ) +def test_link_post_already_exists(postits_create, authenticated_user, client): + result = client.post("/api/datafiles/link/tapis/system/path") + assert json.loads(result.content)["data"] == "https://tenant/uuid" + assert Link.objects.all()[0].get_uuid() == "uuid" + postits_create.assert_called_with( + systemId="system", + path="path", + allowedUses=-1, + validSeconds=31536000 + ) + result = client.post("/api/datafiles/link/tapis/system/path") + assert result.status_code == 400 + assert result.json() == {"message": "Link for this file already exists"} + + def test_link_delete(postits_create, authenticated_user, mock_tapis_client, client): mock_tapis_client.files.deletePostIt.return_value = "OK" client.post("/api/datafiles/link/tapis/system/path") @@ -138,6 +154,13 @@ def test_link_delete(postits_create, authenticated_user, mock_tapis_client, clie assert len(Link.objects.all()) == 0 +def test_link_delete_dne(authenticated_user, mock_tapis_client, client): + mock_tapis_client.files.deletePostIt.return_value = "Bad Request" + result = client.delete("/api/datafiles/link/tapis/system/path") + assert result.status_code == 400 + assert result.json() == {"message": "Post-it does not exist"} + + def test_link_put(postits_create, authenticated_user, mock_tapis_client, client): mock_tapis_client.files.deletePostIt.return_value = "OK" Link.objects.create( @@ -149,6 +172,13 @@ def test_link_put(postits_create, authenticated_user, mock_tapis_client, client) assert Link.objects.all()[0].get_uuid() == "uuid" +def test_link_put_dne(postits_create, authenticated_user, mock_tapis_client, client): + mock_tapis_client.files.deletePostIt.return_value = "Bad Request" + result = client.put("/api/datafiles/link/tapis/system/path") + assert result.status_code == 400 + assert result.json() == {"message": "Could not find pre-existing link"} + + def test_get_system(client, authenticated_user, mock_tapis_client, agave_storage_system_mock): mock_tapis_client.systems.getSystem.return_value = TapisResult(**agave_storage_system_mock) @@ -204,6 +234,24 @@ def test_tapis_file_view_get_is_logged_for_metrics(mock_indexer, client, authent authenticated_user.username)) +@patch('portal.libs.agave.operations.tapis_indexer') +@patch( + "django.conf.settings.PORTAL_DATAFILES_STORAGE_SYSTEMS", + [{"scheme": "public", "system": "public.system", "homeDir": "/public/home/"}], +) +def test_tapis_file_view_get_unauthorized( + mock_indexer, + client, +): + mock_user = MagicMock() + mock_user.tapis_oauth = 0 + + with patch('django.contrib.auth.get_user', return_value=mock_user): + response = client.get("/api/datafiles/tapis/listing/private/frontera.home.username/test.txt/?length=1234") + assert response.status_code == 403 + assert response.json() == {'message': 'This data requires authentication to view.'} + + @patch('portal.libs.agave.operations.tapis_indexer') def test_tapis_file_view_put_is_logged_for_metrics(mock_indexer, client, authenticated_user, mock_tapis_client, tapis_file_listing_mock, logging_metric_mock): @@ -221,6 +269,32 @@ def test_tapis_file_view_put_is_logged_for_metrics(mock_indexer, client, authent "system:frontera.home.username path:test.txt body:{}".format(authenticated_user.username, body)) +@patch('portal.libs.agave.operations.tapis_indexer') +@patch('portal.apps.datafiles.views.tapis_put_handler') +def test_tapis_file_view_put_is_logged_for_metrics_exception(mock_put_handler, mock_indexer, client, authenticated_user, mock_tapis_client): + mock_put_handler.side_effect = Exception("Exception in Metrics info or Tapis Put Handler views.py:142") + body = {"dest_path": "/testfol", "dest_system": "frontera.home.username"} + response = client.put("/api/datafiles/tapis/move/private/frontera.home.username/test.txt/", + content_type="application/json", + data=body) + assert response.status_code == 500 + + +@patch('portal.libs.agave.operations.tapis_indexer') +def test_tapis_file_view_put_is_unauthorized(mock_indexer, client): + mock_user = MagicMock() + mock_user.tapis_oauth = 0 + with patch('django.contrib.auth.get_user', return_value=mock_user): + body = {"dest_path": "/testfol", "dest_system": "frontera.home.username"} + response = client.put( + "/api/datafiles/tapis/move/private/frontera.home.username/test.txt/", + content_type="application/json", + data=body, + ) + assert response.status_code == 403 + assert response.content == b"This data requires authentication to view." + + @patch('portal.libs.agave.operations.tapis_indexer') def test_tapis_file_view_post_is_logged_for_metrics(mock_indexer, client, authenticated_user, mock_tapis_client, logging_metric_mock, @@ -240,6 +314,29 @@ def test_tapis_file_view_post_is_logged_for_metrics(mock_indexer, client, authen "system:frontera.home.username path:/ filename:text_file.txt".format(authenticated_user.username)) +@patch('portal.libs.agave.operations.tapis_indexer') +def test_tapis_file_view_post_is_unauthorized(mock_indexer, text_file_fixture, client): + mock_user = MagicMock() + mock_user.tapis_oauth = 0 + with patch('django.contrib.auth.get_user', return_value=mock_user): + response = client.post("/api/datafiles/tapis/upload/private/frontera.home.username/", data={"uploaded_file": text_file_fixture}) + assert response.status_code == 403 + assert response.content == b"This data requires authentication to upload." + + +@patch('portal.libs.agave.operations.tapis_indexer') +@patch('portal.apps.datafiles.views.tapis_post_handler') +def test_tapis_file_view_post_is_logged_for_metrics_exception(mock_post_handler, mock_indexer, client, authenticated_user, mock_tapis_client, + logging_metric_mock, tapis_file_mock, requests_mock, text_file_fixture): + mock_post_handler.side_effect = Exception("Exception in Metrics info or Tapis Put Handler views.py:175") + mock_tapis_client.files.insert.return_value = tapis_file_mock + + response = client.post("/api/datafiles/tapis/upload/private/frontera.home.username/", + data={"uploaded_file": text_file_fixture}) + + assert response.status_code == 500 + + POSTIT_HREF = "https://tapis.example/postit/something" From f5c373f2a41ada6110fef9ac701701f541364e0a Mon Sep 17 00:00:00 2001 From: Jacob Lowe <40873986+jalowe13@users.noreply.github.com> Date: Fri, 6 Dec 2024 09:56:05 -0600 Subject: [PATCH 4/4] Bug/WP-418: site search error when user does not have community file listing access. (#1005) * file fix starting point * ssh_op_err1 exception addition and test * comment fix * linting --- .../PublicData/PublicData.module.css | 1 + server/portal/apps/site_search/api/views.py | 12 ++++++-- .../apps/site_search/views_unit_test.py | 30 +++++++++++++++---- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/client/src/components/PublicData/PublicData.module.css b/client/src/components/PublicData/PublicData.module.css index 1ee1414a5..f9572db12 100644 --- a/client/src/components/PublicData/PublicData.module.css +++ b/client/src/components/PublicData/PublicData.module.css @@ -7,3 +7,4 @@ /* FAQ: Public pages, like `PublicData` and `SiteSearch`, have no sidebar */ /* padding-left: 1.5em; /* ~24px (20px * design * 1.2 design-to-app ratio) */ } + diff --git a/server/portal/apps/site_search/api/views.py b/server/portal/apps/site_search/api/views.py index 5eff6f31a..fa7fab86c 100644 --- a/server/portal/apps/site_search/api/views.py +++ b/server/portal/apps/site_search/api/views.py @@ -102,10 +102,18 @@ def get(self, request, *args, **kwargs): return JsonResponse(response) def _handle_tapis_ssh_exception(self, e): - if 'SSH_POOL_MISSING_CREDENTIALS' in str(e) or 'SSH_FX_PERMISSION_DENIED' in str(e): + if ( + "SSH_POOL_MISSING_CREDENTIALS" in str(e) + or "SSH_FX_PERMISSION_DENIED" in str(e) + or "FILES_CLIENT_SSH_OP_ERR1" in str(e) + ): # in case of these error types, user is not authenticated # or does not have access do not fail the entire search # request, log the issue. - logger.exception("Error retrieving search results due to TAPIS SSH related error: {}".format(str(e))) + logger.exception( + "Error retrieving search results due to TAPIS SSH related error: {}".format( + str(e) + ) + ) else: raise diff --git a/server/portal/apps/site_search/views_unit_test.py b/server/portal/apps/site_search/views_unit_test.py index aa775a49f..71834fbac 100644 --- a/server/portal/apps/site_search/views_unit_test.py +++ b/server/portal/apps/site_search/views_unit_test.py @@ -1,18 +1,36 @@ +from unittest.mock import patch + +from tapipy.errors import BaseTapyException + +from portal.apps.site_search.api.views import SiteSearchApiView + + def test_search_unauthenticated(client, regular_user): - response = client.get('/search/') + response = client.get("/search/") assert response.status_code == 200 - assert response.context['setup_complete'] is False + assert response.context["setup_complete"] is False def test_search_authenticated_without_setup_complete(client, authenticated_user): - response = client.get('/search/') + response = client.get("/search/") assert response.status_code == 200 - assert response.context['setup_complete'] is False + assert response.context["setup_complete"] is False def test_search_authenticated_with_setup_complete(client, authenticated_user): authenticated_user.profile.setup_complete = True authenticated_user.profile.save() - response = client.get('/search/') + response = client.get("/search/") assert response.status_code == 200 - assert response.context['setup_complete'] + assert response.context["setup_complete"] + + +@patch("portal.apps.site_search.api.views.logger") +def test_handle_tapis_ssh_exception_files_client_ssh_op_err1(mock_logger): + view = SiteSearchApiView() + message = "FILES_CLIENT_SSH_OP_ERR1" + exception = BaseTapyException(message) + view._handle_tapis_ssh_exception(exception) + mock_logger.exception.assert_called_once_with( + f"Error retrieving search results due to TAPIS SSH related error: message: {message}" + )