-
Notifications
You must be signed in to change notification settings - Fork 18
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
Restrict/Unrestrict File Feature in File Page #617
base: develop
Are you sure you want to change the base?
Restrict/Unrestrict File Feature in File Page #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...edit-file-menu/restrict-file-button/confirm-restrict-file-modal/ConfirmRestrictFileModal.tsx
Outdated
Show resolved
Hide resolved
...edit-file-menu/restrict-file-button/confirm-restrict-file-modal/ConfirmRestrictFileModal.tsx
Show resolved
Hide resolved
src/sections/file/file-action-buttons/edit-file-menu/restrict-file-button/useRestrictFile.tsx
Outdated
Show resolved
Hide resolved
tests/component/sections/file/file-action-buttons/edit-file-menu/EditFileMenu.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
About the redirection/refresh of dataset after restricting/unrestricting a file we can have a call if you want.
I have some suggestions about the EditFilesOptions
component that is being reused for the files header and file actions cell.
It is confusing that in both cases you send an array of files even knowing that for file row you just need the file only and then mapping the array of files knowing that for IsHeader= false it will be only one.
You can do two different things:
- One component for the header and another different one for the file actions cell. This could simplify lots of things but also think if using the same component for header and file table row will bring some benefit (if you keep about using same file for header and table row see option 2)
- Use a union type props in EditFilesOptions like this:
type EditFileOptionsProps =
| {
files: FilePreview[]
file?: never
fileSelection: FileSelection
fileRepository: FileRepository
datasetInfo?: never
isHeader: true
}
| {
files?: never
file: FilePreview
fileSelection?: never
fileRepository: FileRepository
datasetInfo: EditFilesMenuDatasetInfo
isHeader: false
}
This interface will only require a files
and fileSelection
prop when isHeader
is true.
And only require file
and datasetInfo
prop when isHeader
is false.
Then the usage of EditFileOptions will be:
In the header:
<EditFilesOptions
files={files}
fileSelection={fileSelection}
fileRepository={fileRepository}
isHeader={true}
/>
In the File Actions Cell:
<EditFilesOptions
file={file}
fileRepository={fileRepository}
datasetInfo={datasetInfo}
isHeader={false}
/>
EditFilesOptions.tsx could look like:
export function EditFilesOptions({
file,
files,
fileSelection,
fileRepository,
datasetInfo,
isHeader
}: EditFileOptionsProps) {
const { t } = useTranslation('files')
const [showNoFilesSelectedModal, setShowNoFilesSelectedModal] = useState(false)
const settingsEmbargoAllowed = false // TODO - Ask Guillermo if this is included in the settings endpoint
const provenanceEnabledByConfig = false // TODO - Ask Guillermo if this is included in the MVP and from which endpoint is coming from
const { showModal } = useNotImplementedModal()
if (!isHeader) {
return (
<>
<RestrictFileButton
key={file.id}
fileId={file.id}
isRestricted={file.access.restricted}
fileRepository={fileRepository}
datasetInfo={datasetInfo}
/>
<DeleteFileButton
key={file.id}
fileId={file.id}
fileRepository={fileRepository}
datasetInfo={datasetInfo}
/>
</>
)
}
const onClick = () => {
if (Object.keys(fileSelection).length === SELECTED_FILES_EMPTY) {
setShowNoFilesSelectedModal(true)
} else {
// TODO - Implement edit files
showModal()
}
}
return (
<>
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.metadata')}
</DropdownButtonItem>
{files.some((file) => file.access.restricted) && (
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.unrestrict')}
</DropdownButtonItem>
)}
{files.some((file) => !file.access.restricted) && (
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.restrict')}
</DropdownButtonItem>
)}
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.replace')}
</DropdownButtonItem>
{settingsEmbargoAllowed && (
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.embargo')}
</DropdownButtonItem>
)}
{provenanceEnabledByConfig && (
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.provenance')}
</DropdownButtonItem>
)}
<DropdownButtonItem onClick={onClick}>
{t('actions.editFilesMenu.options.delete')}
</DropdownButtonItem>
<NoSelectedFilesModal
show={showNoFilesSelectedModal}
handleClose={() => setShowNoFilesSelectedModal(false)}
/>
</>
)
}
What this PR does / why we need it:
Implement the Restrict/Unrestrict Feature for a single file
Which issue(s) this PR closes:
Special notes for your reviewer:
Some changes need to know:
isHeader
toEditFileOptions
.isHeader == true
means it's the EditFileOptions button existing in the header of filesTable, dealing with multiple files.isHeader == false
means it is one of the file in files, dealing with a single file.Suggestions on how to test this:
create a dataset > upload file
click into the file page, Edit File > Restrict
A modal should be shown

(input box and checkbox are both disabled, waiting for api )
Please notice that
Restrict Access
andTerms of Access
are hard-coded, not connect to api yetSave Changes, a toast should be shown. then check if the file is restricted by checking if a Unlock Icon is shown on dataset page > File tab

click into the file page, Edit File > Unrestrict, a modal shown
lOG
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: