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

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(
+      <DataFilesListing
+        api="tapis"
+        scheme="private"
+        system="test.system"
+        resultCount={4}
+        path="/"
+      />,
+      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(
+      <DataFilesListing
+        api="tapis"
+        scheme="private"
+        system="test.system"
+        resultCount={4}
+        path="/"
+      />,
+      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);
+  });
+});

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

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

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,
   },
 };
 

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: [
             {

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

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:{} "