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

Metadata forms: List of Attached Forms #1652

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

Conversation

SofiaSazonova
Copy link
Contributor

Feature or Bugfix

  • Feature

Detail

  • New Tab in MF View
  • NO ACCESS RESTRICTION: if entity has an attached form, it's visible in this tab

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)?
    • Is the input sanitized?
    • What precautions are you taking before deserializing the data you consume?
    • Is injection prevented by parametrizing queries?
    • Have you ensured no eval or similar functions are used?
  • Does this PR introduce any functionality or component that requires authorization?
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
    • Are you logging failed auth attempts?
  • Are you using or adding any cryptographic features?
    • Do you use a standard proven implementations?
    • Are the used keys controlled by the customer? Where are they stored?
  • Are you introducing any new policies/roles/users?
    • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@SofiaSazonova SofiaSazonova marked this pull request as ready for review October 29, 2024 12:45
@noah-paige
Copy link
Contributor

One bug I see - Env and Org Forms not showing to Attach to S3 Dataset Entity Anymore
- This is from earlier PR when changing entityType to S3-Dataset: At createAttachedMetadataForm API in RenderedMetadataForm UI View we use entityType Dataset not S3-Dataset
- The source is from DatasetView - Line: <MetadataAttachment entityType="Dataset" entityUri={params.uri} />

Just need to change to S3-Dataset

Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

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

some minor comments

Is access restrictions to be reconsidered later - just curious to hear thoughts here, I think outside scope of this PR for now

@@ -38,6 +39,7 @@ const MetadataFormView = () => {
const tabs = [
{ label: 'Form Info', value: 'info' },
{ label: 'Fields', value: 'fields' },
{ label: 'Attached', value: 'attached' },
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would suggest change Attached to Attached Entities for clarity

maxLines: 1
}}
>
{version.attached_forms}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Change to {'Count: ' + version.attached_forms} for clarity

{' version ' + version.version}
</Typography>
</Grid>
<Grid item lg={3} xl={3}>
Copy link
Contributor

Choose a reason for hiding this comment

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

would suggest to even out spacing, the below seemed more evenly spaced on Versions Table (with including the Count: suggestion below):

L 194-195: lg={5} xl={5}

L214: <Grid item lg={5} xl={5}>

L228: <Grid item lg={1} xl={1}>

or however best to include all information

<CardContent sx={{ display: 'flex', justifyContent: 'center' }}>
<DoNotDisturbAltOutlinedIcon sx={{ mr: 1 }} />
<Typography variant="subtitle2" color="textPrimary">
No Metadata Forms Attached
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should be No Metadata Form Versions in this case right?

<CardHeader title="Versions" />

<Divider />
{versions.length > 0 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

can we wrap this section similarly around a block that if loading is false we show <CircularProgress /> and if true we show the version options

or use a new loadingVersion state variable if better UX

++ can we set loading state initially to true so it begins in this loading state until MF forms are populated

@SofiaSazonova
Copy link
Contributor Author

All done

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.

2 participants