fix: fixed and improved some user interface issues. #1666

Junjiequan · 2024-11-20T10:29:48Z

Description

This PR contains a couple of small UI improvements and changes:

made large data files download instruction text configurable on the dataset/datafiles page
commented out sample editing feature temporarily, as the sample editing feature is broken and there's no strong need for the feature.
resized thumbnail image to prevent it from being cut off partially.
replaced red cross pdf file thumbnail with default pdf icon on dataset table
added tests for the changes.

Motivation

Fixes:

Please provide a list of the fixes implemented in this PR

Items added

Changes:

Please provide a list of the changes implemented by this PR

changes made

Tests included

Included for each change/fix?
Passing? (Merge will not be approved unless this is checked)

Documentation

swagger documentation updated [required]
official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

Does it require a specific version of the backend
which version of the backend is required:

Summary by Sourcery

Disable the sample editing feature due to bugs and the need for a centralized edit button. Refactor image URL retrieval logic to use the AttachmentService for consistency and maintainability. Update the datafiles component to dynamically replace file size warning messages with configurable values.

Bug Fixes:

Disable the sample editing feature due to existing bugs and the need for a centralized edit button.

Enhancements:

Refactor the method for retrieving image URLs by moving logic to the AttachmentService.

sourcery-ai

Hey @Junjiequan - I've reviewed your changes - here's some feedback:

Overall Comments:

Please provide a proper description of the changes in the PR description. Specifically list out: 1) The removal of sample editing functionality 2) The refactoring of image URL handling 3) The improvements to large file download messages
The documentation checkboxes are all unchecked. Since this PR removes user-facing functionality and changes UI elements, please update the relevant documentation and check the appropriate boxes.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/app/datasets/datafiles/datafiles.component.ts

src/app/shared/services/attachment.service.ts

…ataset table

martin-trajanovski · 2024-11-20T14:28:06Z

src/app/datasets/dataset-detail/dataset-detail.component.html

@@ -268,13 +268,17 @@
                <a (click)="onClickSample(sample.sampleId)">
                  <span>{{ sample.description }}</span>
                </a>
-                <span
+                <!-- NOTE: Sample editing is currently disabled.


Do we want to keep this or remove it completely from the code for now?

I agree. Maybe it's better off to remove this code as we are not certain if we need this feature or not, instead of polluting the source code.
What's your thoughts? @nitrosx

I have mixed feelings... THINKING THINKING...
Let's remove it!!!

martin-trajanovski · 2024-11-20T14:29:25Z

src/app/datasets/dataset-detail/dataset-detail.component.spec.ts

-      );
-    });
-  });
+  // NOTE: Sample editing is currently disabled.


Maybe it is better to remove this instead of just commenting it

Same as above. Remove it.
Let's clean up the code

martin-trajanovski · 2024-11-20T14:29:33Z

src/app/datasets/dataset-detail/dataset-detail.component.ts

-        });
-    }
-  }
+  // NOTE: Sample editing is currently disabled.


nitrosx

Code changes requested are not functional. They are in pseudo code, most likely need to be refactored.

nitrosx · 2024-11-22T09:15:04Z

src/app/app-config.service.ts

@@ -82,6 +82,7 @@ export interface AppConfig {
  searchPublicDataEnabled: boolean;
  searchSamples: boolean;
  sftpHost: string | null;
+  largeDataFileAccessInstruction?: string;


Change name to maxFileSizeWarning

nitrosx · 2024-11-22T09:16:16Z

src/app/datasets/datafiles/datafiles.component.html

-            "."
-          : ""
-      }}
+      <span [innerHTML]="hasTooLargeFilesMessage()"></span>


Change name to hasFileAboveMaxSizeWarning()

nitrosx · 2024-11-22T09:19:53Z

src/app/datasets/datafiles/datafiles.component.ts

+  customSourcefolder: string | null =
+    this.appConfig.largeDataFileAccessInstruction;


Change it to:

maxFileSizeWarning: string = this.appConfig.maxFileSizeWarning || `Some files are above the max size (<maxFileSize>)`

nitrosx · 2024-11-22T09:33:21Z

src/app/datasets/datafiles/datafiles.component.ts

+  hasTooLargeFilesMessage() {
+    return `Some files are too big, but they can be downloaded 
+    at our sftp server: <strong>${this.sftpHost}</strong> 
+    at the folder: <strong>${
+      this.customSourcefolder || this.sourcefolder
+    }</strong>`;
+  }
+


Change it to:

hasFileAboveMaxSizeWarning() { const valueMapping = { '<sftpHost>' : this.sftpHost, '<sourceFolder>': this.sourceFolder // ... any other value that we think might be important ... } var warning = this.maxFileSizeWarning; Object.keys(valaueMapping).forEach((key) => { warning = warning.replace('/' + key + '/', valueMapping[key]; }); return warning; }

This way, site admin can configure the message to their liking. They also can use <sftHost> or <sourceFolder> as placemarks that will be substituted in.

The code is pseudo code. Most likely it needs to be adjusted.

One more thing, with the new data file actions, some facilities will configure them to perform action which do not have size restriction.

bpedersen2

Look good.

In the furture we probably need to adjust for different download methods etc.

sourcery-ai bot reviewed Nov 20, 2024

View reviewed changes

Junjiequan added 7 commits November 20, 2024 15:03

disabled sample edit

ee51ea4

enhanced large file download instructions and update config

1472fbe

commented out sample edit test

a51bbdb

fix the issue of the red cross icon appearing for PDF images in the d…

29d76ff

…ataset table

fix for thumbnail image being cut off

1f2fb15

fixes based on AI PR review

dceb6ae

added unit test for the change

1f9b433

Junjiequan force-pushed the SWAP-4247-change-sftp-text-on-scicat branch from 7cee42d to 1f9b433 Compare November 20, 2024 14:03

Junjiequan requested review from nitrosx, martin-trajanovski and bpedersen2 November 20, 2024 14:11

Junjiequan mentioned this pull request Nov 20, 2024

feat: make large file access warning text configurable with largeData… SciCatProject/scicat-backend-next#1517

Open

4 tasks

martin-trajanovski reviewed Nov 20, 2024

View reviewed changes

rename for large file message function

01e2e9c

nitrosx requested changes Nov 22, 2024

View reviewed changes

fix based on comments

bc3fcfe

Junjiequan requested a review from nitrosx November 26, 2024 08:37

fix failing test

31e29a1

bpedersen2 approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'master' into SWAP-4247-change-sftp-text-on-scicat

e06eddc

nitrosx approved these changes Nov 27, 2024

View reviewed changes

Merge branch 'master' into SWAP-4247-change-sftp-text-on-scicat

8b8f949

Junjiequan merged commit 0d37dfb into master Nov 28, 2024
6 checks passed

Junjiequan deleted the SWAP-4247-change-sftp-text-on-scicat branch November 28, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fixed and improved some user interface issues. #1666

fix: fixed and improved some user interface issues. #1666

Junjiequan commented Nov 20, 2024 •

edited

Loading

sourcery-ai bot left a comment

martin-trajanovski Nov 20, 2024

Junjiequan Nov 20, 2024 •

edited

Loading

nitrosx Nov 22, 2024

martin-trajanovski Nov 20, 2024

nitrosx Nov 22, 2024

martin-trajanovski Nov 20, 2024

nitrosx Nov 22, 2024

nitrosx left a comment

nitrosx Nov 22, 2024

nitrosx Nov 22, 2024

nitrosx Nov 22, 2024

nitrosx Nov 22, 2024

bpedersen2 left a comment

		customSourcefolder: string \| null =
		this.appConfig.largeDataFileAccessInstruction;

fix: fixed and improved some user interface issues. #1666

fix: fixed and improved some user interface issues. #1666

Conversation

Junjiequan commented Nov 20, 2024 • edited Loading

Description

Motivation

Fixes:

Changes:

Tests included

Documentation

official documentation info

Backend version

Summary by Sourcery

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Junjiequan Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitrosx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpedersen2 left a comment

Choose a reason for hiding this comment

Junjiequan commented Nov 20, 2024 •

edited

Loading

Junjiequan Nov 20, 2024 •

edited

Loading