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.

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

Feature/upload config and blob from UI #803

Merged
merged 72 commits into from
Nov 3, 2023

Conversation

postrowinski
Copy link
Contributor

@postrowinski postrowinski commented Apr 24, 2023

Your checklist for this pull request

  • I've read the contributing guideline.
  • I've tested my changes by building and running the project, and testing changed functionality (if applicable)
  • I've added automated tests for my change (if applicable, optional)
  • I've updated documentation to reflect my change (if applicable)

What is the current behaviour?

From UI, you can only add file/sample.

What is the new behaviour?

I change upload to dropdown with options to file/config/blob
image

Config form layout:
image

Blob form layout:
image

Test plan

Closing issues

closes #677

@postrowinski postrowinski requested a review from psrok1 April 24, 2023 11:37
@postrowinski postrowinski marked this pull request as ready for review May 8, 2023 09:43
# Conflicts:
#	mwdb/web/src/commons/ui/index.jsx
# Conflicts:
#	mwdb/web/package-lock.json
#	mwdb/web/package.json
#	mwdb/web/src/commons/hooks/index.js
#	mwdb/web/src/commons/ui/index.jsx
…o-common-ui

# Conflicts:
#	mwdb/web/package-lock.json
#	mwdb/web/src/commons/auth/provider.tsx
#	mwdb/web/src/commons/ui/index.jsx
#	mwdb/web/src/components/Profile/Views/ProfileCapabilities.jsx
#	mwdb/web/src/components/Settings/Views/AccessControl.jsx
# Conflicts:
#	mwdb/web/src/commons/api/index.jsx
# Conflicts:
#	mwdb/web/src/commons/api/index.jsx
postrowinski and others added 22 commits May 23, 2023 09:55
…ature/add-types-to-components-settings

# Conflicts:
#	mwdb/web/src/commons/api/index.tsx
#	mwdb/web/src/components/Profile/ProfileView.tsx
#	mwdb/web/src/components/Profile/common/CapabilitiesSelect.tsx
#	mwdb/web/src/components/Profile/common/CapabilitiesTable.tsx
#	mwdb/web/src/types/context.ts
…to feature/upload-config-and-blob-from-ui

# Conflicts:
#	mwdb/web/src/App.jsx
#	mwdb/web/src/commons/api/index.jsx
#	mwdb/web/src/components/Navigation.tsx
#	mwdb/web/src/components/Views/UploadView.tsx
# Conflicts:
#	mwdb/web/src/commons/api/index.tsx
#	mwdb/web/src/commons/navigation/AppRoutes.tsx
#	mwdb/web/src/commons/plugins/Extendable.tsx
#	mwdb/web/src/commons/plugins/Extension.tsx
#	mwdb/web/src/components/AttributesAddModal.tsx
#	mwdb/web/src/components/Navigation.tsx
#	mwdb/web/src/components/RelationsNode.tsx
#	mwdb/web/src/components/ShowObject/common/RelationsTab.tsx
#	mwdb/web/src/components/UploadButton.tsx
#	mwdb/web/src/components/Views/RelationsPlotView.tsx
#	mwdb/web/src/components/Views/UploadView.tsx
#	mwdb/web/src/components/Views/UserLoginView.tsx
Copy link
Member

@psrok1 psrok1 left a comment

Choose a reason for hiding this comment

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

Looks good, although I would change it a bit as described in comments.

Comment on lines 81 to 83
path="upload"
path="file_upload"
Copy link
Member

Choose a reason for hiding this comment

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

I woyld leave that path for compatibility

@@ -24,6 +25,7 @@ export function Label(props: Props) {
return (
<label className={setClassName()} htmlFor={htmlFor}>
{label}
{children ?? children}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't ?? children no-op here?

import { UploadButton } from "../../UploadButton";

export function UploadDropdown() {
const uploadElement = [
Copy link
Member

Choose a reason for hiding this comment

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

I would hide additional dropdown elements when user doesn't have capability to upload configs or blobs. If no upload is possible, button should be hidden. If only one type is possible, I would make it single button described as Upload (for files), Upload blob (for blobs), Upload config (for configs)

@psrok1
Copy link
Member

psrok1 commented Oct 10, 2023

I see that my merge was incomplete, we should ensure that all new features made in between are included. Right now I see that 3rd party sharing is missing.

Copy link
Member

@psrok1 psrok1 left a comment

Choose a reason for hiding this comment

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

I've made some hands-on on this code and found more issues. Another part of the review

UploadConfigRequest,
UploadConfigResponse,
UploadBlobRequest,
UploadBlobReponse,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UploadBlobReponse,
UploadBlobResponse,

export type UploadFileRequest = {
file: File;
parent?: string;
upload_as?: string;
Copy link
Member

Choose a reason for hiding this comment

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

This field is no longer used, as we have shareWith for that.

Suggested change
upload_as?: string;

family: string;
parent?: string;
config_type?: string;
attributes?: Attribute[];
Copy link
Member

Choose a reason for hiding this comment

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

missing shareWith and share3rdParty. I think it would be nice to split the common fields from specialized fields in that scheme (if possible)

parent?: string;
shareWith: string;
group: string;
attributes: Attribute[];
Copy link
Member

Choose a reason for hiding this comment

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

missing share3rdParty

</Autocomplete>
</div>
)}
<Attributes
Copy link
Member

Choose a reason for hiding this comment

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

We need to add sharing with 3rd party button. I would add prettified version

Suggested change
<Attributes
<ShowIf condition={
!config.config[
"is_3rd_party_sharing_consent_enabled"
]
}
>
<div className="input-group mb-3">
<div className="input-group-prepend">
<label
className="input-group-text"
>
Share with 3rd party services
</label>
</div>
<div className="input-group-append form-control" style={{
alignItems: "center",
height: "32pt",
}}>
<div className="material-switch make-horizontal">
<input
{...register("share3rdParty" as keyof FormValues)}
type="checkbox"
id="share_3rd_party"
onChange={updateSharing3rdParty}
/>
<label
htmlFor={"share_3rd_party"}
className="bg-primary"
/>
</div>
</div>
<div className="form-hint">
Object can be shared automatically with 3rd party services
</div>
</div>
</ShowIf>
<Attributes

@@ -317,3 +327,25 @@ export type OauthRemoveSingleProviderResponse = Response<null>;
export type OauthGetLogoutLinkResponse = Response<{
url: string;
}>;

export type UploadConfigRequest = {
Copy link
Member

Choose a reason for hiding this comment

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

I would place these declarations after UploadFileRequest and Response for better readability

@psrok1 psrok1 merged commit a450b83 into master Nov 3, 2023
12 checks passed
@psrok1 psrok1 deleted the feature/upload-config-and-blob-from-ui branch November 3, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload config from the UI
2 participants