Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat!: add a convenient way to add an openBIS cloud storage #3238

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat!: add a convenient way to add an openBIS cloud storage #3238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29b9d3c
feat!: add a convenient way to add an openBIS cloud storage
olloz26 Jul 11, 2024
12fb579
feat: implement review suggestions
olloz26 Sep 2, 2024
58b3cad
docs: correct a comment
olloz26 Sep 2, 2024
a6d93d4
fix: build errors
olloz26 Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@
"onloadend",
"onopen",
"openapi",
"openbis",
"papermill",
"pathname",
"pdfjs",
Expand Down
76 changes: 56 additions & 20 deletions client/src/features/project/components/cloudStorage/AddOrEditCloudStorage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ import {
parseCloudStorageConfiguration,
} from "../../utils/projectCloudStorage.utils";
import { ExternalLink } from "../../../../components/ExternalLinks";
import { WarnAlert } from "../../../../components/Alert";
import { InfoAlert, WarnAlert } from "../../../../components/Alert";

import styles from "./CloudStorage.module.scss";

Expand Down Expand Up @@ -94,7 +94,6 @@ export default function AddOrEditCloudStorage({
if (ContentByStep)
return (
<>
<AddStorageAdvancedToggle state={state} setState={setState} />
<AddStorageBreadcrumbNavbar state={state} setState={setState} />
<ContentByStep
schema={schema}
Expand Down Expand Up @@ -124,9 +123,6 @@ export function AddOrEditCloudStorageV2({
if (ContentByStep)
return (
<>
<div className={cx("d-flex", "justify-content-end")}>
<AddStorageAdvancedToggle state={state} setState={setState} />
</div>
<ContentByStep
schema={schema}
state={state}
Expand Down Expand Up @@ -271,7 +267,13 @@ interface AddStorageAdvancedForm {
sourcePath: string;
configuration: string;
}
function AddStorageAdvanced({ storage, setStorage }: AddStorageStepProps) {

function AddStorageAdvanced({
setStorage,
setState,
state,
storage,
}: AddStorageStepProps) {
const {
control,
formState: { errors },
Expand Down Expand Up @@ -307,6 +309,7 @@ function AddStorageAdvanced({ storage, setStorage }: AddStorageStepProps) {

return (
<form className="form-rk-green" data-cy="cloud-storage-edit-advanced">
<AddStorageAdvancedToggle state={state} setState={setState} />
<div className="mb-3">
<Controller
name="sourcePath"
Expand Down Expand Up @@ -643,10 +646,13 @@ function AddStorageType({
() => getSchemaStorage(schema, !state.showAllSchema, storage.schema),
[schema, state.showAllSchema, storage.schema]
);
const setFinalSchema = (value: string) => {
setStorage({ schema: value });
const setFinalSchema = (schema: CloudStorageSchema) => {
setStorage({
schema: schema.prefix,
convenientMode: schema.convenientMode,
});
if (state.showAllSchema) setState({ showAllSchema: false });
hasProviderShortlist(value) && scrollToProvider();
hasProviderShortlist(schema.prefix) && scrollToProvider();
};

const schemaItems = availableSchema.map((s, index) => {
Expand All @@ -660,7 +666,7 @@ function AddStorageType({
key={s.name}
value={s.prefix}
tag="div"
onClick={() => setFinalSchema(s.prefix)}
onClick={() => setFinalSchema(s)}
data-cy={`data-storage-${s.prefix}`}
>
<p className="mb-0">
Expand Down Expand Up @@ -994,17 +1000,24 @@ function AddStorageOptions({
<div className={cx("form-text", "text-muted")}>{sourcePathHelp.help}</div>
</div>
);

const selectedSchema = useMemo(
() => getSchemaStorage(schema, !state.showAllSchema, storage.schema),
[schema, state.showAllSchema, storage.schema]
).find((s) => s.prefix === storage.schema);
return (
<form className="form-rk-green" data-cy="cloud-storage-edit-options">
<h5>Options</h5>
<p>
Please fill in all the options required to connect to your storage. Mind
that the specific fields required depend on your storage configuration.
</p>
<AddStorageAdvancedToggle state={state} setState={setState} />
{sourcePath}
{optionItems}
{advancedOptions}
{(selectedSchema &&
"convenientMode" in selectedSchema &&
selectedSchema.convenientMode) ||
advancedOptions}
</form>
);
}
Expand All @@ -1017,7 +1030,12 @@ interface AddStorageMountForm {
readOnly: boolean;
}
type AddStorageMountFormFields = "name" | "mountPoint" | "readOnly";
function AddStorageMount({ setStorage, storage }: AddStorageStepProps) {
function AddStorageMount({
schema,
state,
setStorage,
storage,
}: AddStorageStepProps) {
const {
control,
formState: { errors, touchedFields },
Expand All @@ -1043,6 +1061,14 @@ function AddStorageMount({ setStorage, storage }: AddStorageStepProps) {
setStorage({ ...getValues() });
};

const selectedSchema = useMemo(
() => getSchemaStorage(schema, !state.showAllSchema, storage.schema),
[schema, state.showAllSchema, storage.schema]
).find((s) => s.prefix === storage.schema);
if (selectedSchema && selectedSchema.readOnly) {
storage.readOnly = true;
}
Comment on lines +1064 to +1070
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea to allow only read-only in every case? How do users change data there?

If that's the case, we should probably improve the UX because, with the current solution, we show a ticked read-only box that looks like an element users can interact with, but clicking doesn't do anything. I suggest:

  • Making the readOnly input... "readOnly" -- sorry for the pun, I mean that the element <input id="readonly [...] /> should have something like <input id="readonly [...] readOnly={selectedSchema.readOnly ?? false} />
  • Show a warning mentioning that there is no way to write with that external storage.

As I write these lines, I wonder why forcing the read-only flag here -- shouldn't that be down to the user's permissions? I assume some users can write.


return (
<form className="form-rk-green" data-cy="cloud-storage-edit-mount">
<h5>Final details</h5>
Expand Down Expand Up @@ -1141,21 +1167,31 @@ function AddStorageMount({ setStorage, storage }: AddStorageStepProps) {
}}
value=""
checked={storage.readOnly ?? false}
readOnly={(selectedSchema && selectedSchema.readOnly) ?? false}
/>
)}
rules={{ required: true }}
/>
{!storage.readOnly && (
{(selectedSchema && selectedSchema.readOnly && (
<div className="mt-1">
<WarnAlert dismissible={false}>
<InfoAlert dismissible={false}>
<p className="mb-0">
You are mounting this storage in read-write mode. If you have
read-only access, please check the box to prevent errors with
some storage types.
This cloud storage only supports read-only access.
</p>
</WarnAlert>
</InfoAlert>
</div>
)}
)) ||
(!storage.readOnly && (
<div className="mt-1">
<WarnAlert dismissible={false}>
<p className="mb-0">
You are mounting this storage in read-write mode. If you have
read-only access, please check the box to prevent errors with
some storage types.
</p>
</WarnAlert>
</div>
))}
<div className={cx("form-text", "text-muted")}>
Check this box to mount the storage in read-only mode. You should
always check this if you do not have credentials to write. You can use
Expand Down
15 changes: 12 additions & 3 deletions client/src/features/project/components/cloudStorage/CloudStorageModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,9 +380,17 @@ export default function CloudStorageModal({

// Visual elements
const disableContinueButton =
state.step === 1 &&
(!storageDetails.schema ||
(schemaRequiresProvider && !storageDetails.provider));
(state.step === 1 &&
(!storageDetails.schema ||
(schemaRequiresProvider && !storageDetails.provider))) ||
(state.step === 2 &&
storageDetails.convenientMode &&
!(
storageDetails.sourcePath &&
storageDetails.options &&
Object.values(storageDetails.options).every((v) => v)
)) ||
false;

const isAddResultLoading = addResult.isLoading || addResultV2.isLoading;
const isModifyResultLoading =
Expand Down Expand Up @@ -448,6 +456,7 @@ export default function CloudStorageModal({
storageDetails={storageDetails}
storageId={storageId}
success={success}
disableContinueButton={disableContinueButton}
/>
</ModalBody>

Expand Down
6 changes: 5 additions & 1 deletion client/src/features/project/components/cloudStorage/cloudStorageModalComponents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ interface AddCloudStorageBodyContentProps
state: AddCloudStorageState;
storageDetails: CloudStorageDetails;
success: boolean;
disableContinueButton: boolean;
}
export function AddCloudStorageBodyContent({
addResultStorageName,
Expand Down Expand Up @@ -255,6 +256,7 @@ export function AddCloudStorageContinueButton({
testIsFailure={validationResult.isError}
testIsOngoing={validationResult.isLoading}
testIsSuccess={validationResult.isSuccess}
disableContinueButton={disableContinueButton}
/>
{disableContinueButton && (
<UncontrolledTooltip
Expand Down Expand Up @@ -335,6 +337,7 @@ interface TestConnectionAndContinueButtonsProps {
testIsFailure: boolean;
testIsOngoing: boolean;
testIsSuccess: boolean;
disableContinueButton: boolean;
}
function TestConnectionAndContinueButtons({
actionState,
Expand All @@ -346,6 +349,7 @@ function TestConnectionAndContinueButtons({
testIsFailure,
testIsOngoing,
testIsSuccess,
disableContinueButton,
}: TestConnectionAndContinueButtonsProps) {
const buttonTestId = `${testId}-button`;
const divTestId = `${testId}-div`;
Expand Down Expand Up @@ -375,7 +379,7 @@ function TestConnectionAndContinueButtons({
id={buttonTestId}
data-cy={buttonTestId}
className={cx(testConnectionColorClass)}
disabled={testIsOngoing}
disabled={disableContinueButton || testIsOngoing}
onClick={() => actionTest()}
>
{testConnectionContent}
Expand Down
12 changes: 11 additions & 1 deletion client/src/features/project/components/cloudStorage/projectCloudStorage.constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ export const CLOUD_STORAGE_OVERRIDE = {
"WebDAV compatible services, including PolyBox and SwitchDrive",
position: 2,
},
openbis: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is easy to address: you can add openbis to the array "skipWords" in the lining configuration in "client/.eslintrc.json"

position: 3,
convenientMode: true,
readOnly: true,
},
} as Record<string, Partial<CloudStorageOverride>>,
};

Expand Down Expand Up @@ -122,7 +127,12 @@ export const CLOUD_STORAGE_MOUNT_PATH_HELP = {
},
} as Record<string, Record<"help" | "placeholder", string>>;

export const CLOUD_STORAGE_SCHEMA_SHORTLIST = ["s3", "webdav", "azureblob"];
export const CLOUD_STORAGE_SCHEMA_SHORTLIST = [
"azureblob",
"openbis",
"s3",
"webdav",
];

export const CLOUD_STORAGE_PROVIDERS_SHORTLIST = {
s3: ["AWS", "GCS", "Switch"],
Expand Down
3 changes: 3 additions & 0 deletions client/src/features/project/components/cloudStorage/projectCloudStorage.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ export interface CloudStorageSchema {
hide?: boolean;
prefix: string; // ? weird naming; it's the machine readable name
position?: number;
convenientMode?: boolean; // ? Disables the Rclone full options list, ...
readOnly?: boolean; // ? Forces read-only access e.g. for storage that do not support write access
options: CloudStorageSchemaOptions[];
}

Expand Down Expand Up @@ -155,6 +157,7 @@ export type CloudStorageDetails = {
sourcePath?: string;
mountPoint?: string;
readOnly?: boolean;
convenientMode?: boolean;
};

export interface TestCloudStorageConnectionParams {
Expand Down
2 changes: 2 additions & 0 deletions client/src/features/project/utils/projectCloudStorage.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ export function getSchemaStorage(
name: override.name ?? element.name,
description: override.description ?? element.description,
position: override.position ?? element.position,
convenientMode: override.convenientMode ?? element.convenientMode,
readOnly: override.readOnly ?? element.readOnly,
});
}
} else {
Expand Down
45 changes: 45 additions & 0 deletions tests/cypress/fixtures/cloudStorage/storage-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,5 +230,50 @@
"aliases": null,
"hide": false,
"metadata_info": null
},
{
"name": "openbis",
"description": "openBIS",
"prefix": "openbis",
"options": [
{
"name": "host",
"help": "openBIS host to connect to.\n\nE.g. \"openbis-eln-lims.ethz.ch\".",
"provider": "",
"default": "",
"short_opt": "",
"hide": 0,
"required": true,
"is_password": false,
"no_prefix": false,
"advanced": false,
"exclusive": false,
"sensitive": false,
"default_str": "",
"value_str": "",
"type": "string"
},
{
"name": "session_token",
"help": "openBIS session token",
"provider": "",
"default": "",
"short_opt": "",
"hide": 0,
"required": false,
"is_password": true,
"no_prefix": false,
"advanced": false,
"exclusive": false,
"sensitive": true,
"default_str": "",
"value_str": "",
"type": "string"
}
],
"command_help": null,
"aliases": null,
"hide": false,
"metadata_info": null
}
]
Loading