Skip to content

Implement create/clone/edit functionality for collection form #3644

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

Merged
dvail merged 1 commit into from
Nov 4, 2022
Merged

Implement create/clone/edit functionality for collection form #3644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 67 additions & 10 deletions ui/apps/platform/src/Containers/Collections/CollectionForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,18 @@ import ConfirmationModal from 'Components/PatternFly/ConfirmationModal';
import useSelectToggle from 'hooks/patternfly/useSelectToggle';
import useToasts from 'hooks/patternfly/useToasts';
import { collectionsBasePath } from 'routePaths';
import { CollectionResponse, deleteCollection } from 'services/CollectionsService';
import {
CollectionResponse,
createCollection,
deleteCollection,
updateCollection,
} from 'services/CollectionsService';
import { CollectionPageAction } from './collections.utils';
import RuleSelector from './RuleSelector';
import CollectionAttacher from './CollectionAttacher';
import CollectionResults from './CollectionResults';
import { Collection, ScopedResourceSelector, SelectorEntityType } from './types';
import { generateRequest } from './converter';

function AttachedCollectionTable({ collections }: { collections: CollectionResponse[] }) {
return collections.length > 0 ? (
Expand Down Expand Up @@ -147,9 +153,19 @@ function CollectionForm({
const [isDeleting, setIsDeleting] = useState(false);
const { toasts, addToast, removeToast } = useToasts();

const { values, errors, handleChange, handleBlur, setFieldValue } = useFormik({
const {
values,
isValid,
errors,
handleChange,
handleBlur,
setFieldValue,
submitForm,
isSubmitting,
setSubmitting,
} = useFormik({
initialValues: initialData,
onSubmit: () => {},
onSubmit: onSaveCollection,
validationSchema: yup.object({
name: yup.string().trim().required(),
description: yup.string(),
Expand Down Expand Up @@ -191,11 +207,7 @@ function CollectionForm({
deleteCollection(deleteId)
.request.then(history.goBack)
.catch((err) => {
addToast(
`Could not delete collection ${initialData.name ?? ''}`,
'danger',
err.message
);
addToast(`Could not delete collection ${initialData.name}`, 'danger', err.message);
})
.finally(() => {
setDeleteId(null);
Expand All @@ -207,6 +219,38 @@ function CollectionForm({
setDeleteId(null);
}

function onSaveCollection(collection: Collection) {
if (action.type === 'view') {
// Logically should not happen, but just in case
return;
}

const saveServiceCall =
action.type === 'edit'
? (payload) => updateCollection(action.collectionId, payload)
: (payload) => createCollection(payload);

const requestPayload = generateRequest(collection);
const { request } = saveServiceCall(requestPayload);

request
.then(() => {
history.push({ pathname: `${collectionsBasePath}` });
Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

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

As a follow up, you might compare to policies which goBack after save. For a specific example, there was some consensus among the team that when people have the task to create several collections, it might be convenient and seem intuitive to go back to table page which has Create collection button.

We can brainstorm pro and con on a call, if you like.

Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

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

Sequence of titles in browser history is another way to compare pro and con of push versus pop.

Copy link
Copy Markdown
Contributor Author

@dvail dvail Nov 4, 2022

Choose a reason for hiding this comment

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

Sure we can discuss this next week. I compared the behavior of policies and integrations when deciding on this vs goBack and went with push since the way the form works seems a little more similar to integrations. In both collections and integrations, the individual view pages are more of a read-only version of the form, while policies has its own dedicated UI for viewing a single policy. I felt like the goBack functionality was a little unclear when going from an editable collection form to a read-only collection form with the changes applied.

})
.catch((err) => {
addToast(
`There was an error saving collection '${values.name}'`,
'danger',
err.message
);
setSubmitting(false);
});
}

function onCancelSave() {
history.push({ pathname: `${collectionsBasePath}` });
}

const onResourceSelectorChange = (
entityType: SelectorEntityType,
scopedResourceSelector: ScopedResourceSelector
Expand Down Expand Up @@ -450,8 +494,21 @@ function CollectionForm({
</Flex>
{action.type !== 'view' && (
<div className="pf-u-background-color-100 pf-u-p-lg pf-u-py-md">
<Button className="pf-u-mr-md">Save</Button>
<Button variant="secondary">Cancel</Button>
<Button
className="pf-u-mr-md"
onClick={submitForm}
isDisabled={isSubmitting || !isValid}
isLoading={isSubmitting}
>
Save
</Button>
<Button
variant="secondary"
isDisabled={isSubmitting}
onClick={onCancelSave}
>
Cancel
</Button>
</div>
)}
</Form>
Expand Down
3 changes: 3 additions & 0 deletions ui/apps/platform/src/Containers/Collections/CollectionsFormPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ function CollectionsFormPage({
} else if (loading) {
content = <>{/* TODO - Handle UI for loading state */}</>;
} else if (initialData) {
if (pageAction.type === 'clone') {
initialData.name += ' (COPY)';
}
Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

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

is that although control flow prevents initialData from referring to defaultCollectionData for clone action, therefore cannot mutate it,

A question for thought (not blocker to merge) is whether there is an alternative control flow which would allow TypeScript to report such a mutation as a problem? Maybe a prior question is does TypeScript report a problem with current code if defaultCollectionData has as const annotation?

Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

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

Commenting here because I noticed while reading code, but it is hard to comment on the line above:

{getAxiosErrorMessage(error)}

Less important now that you implemented permission checks, but allows any other generic logic.

Copy link
Copy Markdown
Contributor Author

@dvail dvail Nov 4, 2022

Choose a reason for hiding this comment

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

I thought about making the defaultCollectionData as const too, but didn't since it would require the Collection type to use readonly for the embeddedCollectionIds and resourceSelector properties. It is something I thought about when working on the dashboards too, but didn't consider it too strongly since it didn't seem to be a pattern that was used extensively elsewhere.

I think I generally just assume that the data structures I use are immutable even if it isn't strictly enforced. (And sometimes take advantage over it not being so strict like in this case.)

I wonder how Formik would handle things if we passed it immutable initial data? A quick look at their source shows they are running state through a reducer so chances are it would work out fine. I can experiment with this in a follow up.

content = (
<CollectionForm
hasWriteAccessForCollections={hasWriteAccessForCollections}
Expand Down