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

NEW LinkFieldController to handle FormSchema #108

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 1 deletion _config.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@

// Avoid creating global variables
call_user_func(function () {

});
19 changes: 1 addition & 18 deletions _config/config.yml
Original file line number Diff line number Diff line change
@@ -1,23 +1,6 @@
---
Name: linkfield
---

SilverStripe\Admin\LeftAndMain:
extensions:
- SilverStripe\LinkField\Extensions\LeftAndMain

SilverStripe\Admin\ModalController:
extensions:
- SilverStripe\LinkField\Extensions\ModalController

SilverStripe\Forms\TreeDropdownField:
extensions:
- SilverStripe\LinkField\Extensions\AjaxField

SilverStripe\CMS\Forms\AnchorSelectorField:
extensions:
- SilverStripe\LinkField\Extensions\AjaxField
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved

SilverStripe\LinkField\Form\FormFactory:
extensions:
- SilverStripe\LinkField\Extensions\FormFactoryExtension
- SilverStripe\LinkField\Extensions\LeftAndMainExtension
1 change: 0 additions & 1 deletion _config/types.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
---
Name: linkfield-types
---

SilverStripe\LinkField\Type\Registry:
types:
cms:
Expand Down
3 changes: 0 additions & 3 deletions _graphql/queries.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
'readLinkDescription(dataStr: String!)':
type: LinkDescription
resolver: ['SilverStripe\LinkField\GraphQL\LinkDescriptionResolver', 'resolve']
'readLinkTypes(keys: [ID])':
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
type: '[LinkType]'
resolver: ['SilverStripe\LinkField\GraphQL\LinkTypeResolver', 'resolve']
6 changes: 0 additions & 6 deletions _graphql/types.yml
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
LinkDescription:
description: Given some Link data, computes the matching description
fields:
description: String

LinkType:
description: Describe a Type of Link that can be managed by a LinkField
fields:
key: ID
handlerName: String!
title: String!
2 changes: 1 addition & 1 deletion client/dist/js/bundle.js

Large diffs are not rendered by default.

5 changes: 0 additions & 5 deletions client/src/boot/index.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
/* global document */
/* eslint-disable */
import Config from 'lib/Config';
import registerReducers from './registerReducers';
import registerComponents from './registerComponents';
import registerQueries from './registerQueries';

document.addEventListener('DOMContentLoaded', () => {
registerComponents();

registerQueries();

registerReducers();
});
2 changes: 1 addition & 1 deletion client/src/boot/registerComponents.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@

/* eslint-disable */
import Injector from 'lib/Injector';
import LinkPicker from 'components/LinkPicker/LinkPicker';
import LinkField from 'components/LinkField/LinkField';
import LinkModal from 'components/LinkModal/LinkModal';
import FileLinkModal from 'components/LinkModal/FileLinkModal';


const registerComponents = () => {
Injector.component.registerMany({
LinkPicker,
Expand Down
2 changes: 0 additions & 2 deletions client/src/boot/registerQueries.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
/* eslint-disable */
import Injector from 'lib/Injector';
import readLinkTypes from 'state/linkTypes/readLinkTypes';
import readLinkDescription from 'state/linkDescription/readLinkDescription';

const registerQueries = () => {
Injector.query.register('readLinkTypes', readLinkTypes);
Injector.query.register('readLinkDescription', readLinkDescription);
};
export default registerQueries;
26 changes: 0 additions & 26 deletions client/src/boot/registerReducers.js

This file was deleted.

171 changes: 115 additions & 56 deletions client/src/components/LinkField/LinkField.js
Original file line number Diff line number Diff line change
@@ -1,90 +1,149 @@
import React, { Fragment, useState } from 'react';
import { compose } from 'redux';
import { inject, injectGraphql, loadComponent } from 'lib/Injector';
import React, { useState, useEffect } from 'react';
import { bindActionCreators, compose } from 'redux';
import { connect } from 'react-redux';
import { injectGraphql, loadComponent } from 'lib/Injector';
import fieldHolder from 'components/FieldHolder/FieldHolder';
import LinkPicker from 'components/LinkPicker/LinkPicker';
import * as toastsActions from 'state/toasts/ToastsActions';
import backend from 'lib/Backend';
import Config from 'lib/Config';
import PropTypes from 'prop-types';

// section used in window.ss config
const section = 'SilverStripe\\LinkField\\Controllers\\LinkFieldController';

/**
* value - ID of the Link passed from JsonField
* onChange - callback function passed from JsonField - used to update the underlying <input> form field
* types - injected by the GraphQL query
* actions - object of redux actions
*/
const LinkField = ({ value, onChange, types, actions }) => {
const linkID = value;
const [typeKey, setTypeKey] = useState('');
const [data, setData] = useState({});
const [editing, setEditing] = useState(false);

const LinkField = ({ id, loading, Loading, data, LinkPicker, onChange, types, linkDescription, ...props }) => {
if (loading) {
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
return <Loading />;
}
/**
* Call back used by LinkModal after the form has been submitted and the response has been received
*/
const onModalSubmit = async (modalData, action, submitFn) => {

Choose a reason for hiding this comment

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

Need to handle other errors beyond validation. (e.g. Session time-out, not enough permission, random 500)

Choose a reason for hiding this comment

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

Can we look at moving all the methods that do HTTP request to their own file?

It's somewhat manageable now, but once we throw in Link lists in there I think it's going to became a bit more difficult.

Having them as individual method will also make it easier to write JEST test for them.

Copy link
Member Author

@emteknetnz emteknetnz Nov 2, 2023

Choose a reason for hiding this comment

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

Not sure what quite sure why we'd move HTTP request stuff to a different file. HTTP request are:

  • GraphQL query to get link types
  • fetch() (lib/Backend) query to fetch data
  • fetch() (lib/Backend) query to delete link

The queries are simple and don't take up much in the file. Splitting things across files seems like it's needlessly increasing complexity.

For the 2x fetch() requests we can mock in jest them pretty easily - e.g. https://github.com/silverstripe/silverstripe-session-manager/blob/2/client/src/components/LoginSession/tests/LoginSessionContainer-test.js#L10

For GraphQL, I don't know how to mock the responses in jest

Copy link
Member Author

Choose a reason for hiding this comment

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

Can implement in #110

Choose a reason for hiding this comment

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

Having played with the thing a bit more, the const match = formSchema.id.match(/\/linkForm\/([0-9]+)/) logic doesn't seem robust.

I'm thinking this could be substantially better.

  • If you're form validation that should return a 400 along with a form schema.
  • If you're form submission is successful, then it shouldn't return a form schema at all. It should return the ID of the successfully created record. That would mimic tho behaviour of a regular Form object where we return a view with a success message.

Copy link
Member Author

@emteknetnz emteknetnz Nov 6, 2023

Choose a reason for hiding this comment

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

If you're form validation [failed] that should return a 400 along with a form schema.

I can't find the exact commit (it's been forced pushed over so many times), though I do remember there was a technical reason why I chose to not return a 400 on validation failure, which I'd definitely prefer to do.

From memory it's because in lib/Backend (Silverstripe wrapper for fetch()) we have some custom code to throw Errors on 400+ status codes (i.e. !response.ok) so that we can then use fetch's .catch() to handle these. However there's this code in FormBuilder that will then just throw these Errors again and won't get the FormSchema returned, meaning that now validation messages don't show in the FormBuilder form

You can test this by updating the last bit of $form->setValidationResponseCallback in LinkFieldController::createLinkForm()

            $response = $this->getSchemaResponse($schemaId, $form, $errors);
            $response->setStatusCode(400);
            return $response;

You'll get a 400 returned on validation failure, however you'll also get javascript errors in FormBuilderModal.js and Backend.js in browser developer tools

Copy link
Member Author

@emteknetnz emteknetnz Nov 6, 2023

Choose a reason for hiding this comment

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

If you're form submission is successful, then it shouldn't return a form schema at all. It should return the ID of the successfully created record.

I don't agree. To the best of my knowledge the existing FormBuilder/FormSchema "standard" is designed to expect a FormSchema with no errors returned on success. Presumably under the hood it just re-renders the form at that point using the returned FormSchema? Having a quick look at campaign admin it returns a FormSchema on success.

We absolutely should NOT be tinkering with FormBuilder/FormSchema at this stage IMO. While it's a kind of weird standard it does work pretty great and we should just implement it as is

Choose a reason for hiding this comment

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

Looking at Campaign Admin, on success it it's stick a message attribute on success that then gets displayed on the front end.

I don't like that behaviour. It should be displaying a toast instead.

I've added a few ACs to silverstripe/silverstripe-admin#1589:

  • There's a standardised and documented way of creating new FormSchema endpoints in the CMS.
    • This accounts for form validation
    • This accounts for communicating success to the front end (e.g. Displaying success message)
    • Extra context about the success of the form submission can be communicated to the parent JS process

Created a follow up card.

image
image

const formSchema = await submitFn();

// slightly annoyingly, on validation error formSchema at this point will not have an errors node
// instead it will have the original formSchema id used for the GET request to get the formSchema i.e.
// admin/linkfield/schema/linkfield/<ItemID>
// instead of the one used by the POST submission i.e.
// admin/linkfield/linkForm/<LinkID>
const hasValidationErrors = formSchema.id.match(/\/schema\/linkfield\/([0-9]+)/);
if (!hasValidationErrors) {
// get link id from formSchema response
const match = formSchema.id.match(/\/linkForm\/([0-9]+)/);
const valueFromSchemaResponse = parseInt(match[1], 10);

// update component state
setEditing(false);

const [editing, setEditing] = useState(false);
const [newTypeKey, setNewTypeKey] = useState('');
// update parent JsonField data id - this is required to update the underlying <input> form field
// so that the Page (or other parent DataObject) gets the Link relation ID set
onChange(valueFromSchemaResponse);

const onClear = (event) => {
if (typeof onChange !== 'function') {
return;
// success toast
actions.toasts.success('Saved link');
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
}

onChange(event, { id, value: {} });
return Promise.resolve();
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
};

const { typeKey } = data;
const type = types[typeKey];
const modalType = newTypeKey ? types[newTypeKey] : type;

let title = data ? data.Title : '';
/**
* Call back used by LinkPicker when the 'Clear' button is clicked
*/
const onClear = () => {
const endpoint = `${Config.getSection(section).form.linkForm.deleteUrl}/${linkID}`;
// CSRF token 'X-SecurityID' headers needs to be present for destructive requests
backend.delete(endpoint, {}, { 'X-SecurityID': Config.get('SecurityID') })
.then(() => {
actions.toasts.success('Deleted link');
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
})
.catch(() => {
actions.toasts.error('Failed to delete link');
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
});

// update component state
setTypeKey('');
setData({});

// update parent JsonField data ID used to update the underlying <input> form field
onChange(0);
};

if (!title) {
title = data ? data.TitleRelField : '';
}
const title = data.Title || '';
const type = types.hasOwnProperty(typeKey) ? types[typeKey] : {};
const modalType = typeKey ? types[typeKey] : type;
const handlerName = modalType && modalType.hasOwnProperty('handlerName')
? modalType.handlerName
: 'FormBuilderModal';
const LinkModal = loadComponent(`LinkModal.${handlerName}`);

const linkProps = {
const pickerProps = {
title,
link: type ? { type, title, description: linkDescription } : undefined,
onEdit: () => { setEditing(true); },
description: data.description,
typeTitle: type.title || '',
onEdit: () => {
setEditing(true);
},
onClear,
onSelect: (key) => {
setNewTypeKey(key);
setTypeKey(key);
setEditing(true);
},
types: Object.values(types)
};

const onModalSubmit = (modalData, action, submitFn) => {
const { SecurityID, action_insert: actionInsert, ...value } = modalData;

if (typeof onChange === 'function') {
onChange(event, { id, value });
}

setEditing(false);
setNewTypeKey('');

return Promise.resolve();
};

const modalProps = {
type: modalType,
typeTitle: type.title || '',
typeKey,
editing,
onSubmit: onModalSubmit,
onClosed: () => {
setEditing(false);
},
data
linkID
};

const handlerName = modalType ? modalType.handlerName : 'FormBuilderModal';
const LinkModal = loadComponent(`LinkModal.${handlerName}`);
// read data from endpoint and update component state
useEffect(() => {
if (!editing && linkID) {
const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}/${linkID}`;
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
backend.get(endpoint)
.then(response => response.json())
.then(responseJson => {
setData(responseJson);
setTypeKey(responseJson.typeKey);
});
}
}, [editing, linkID]);

return <>
<LinkPicker {...pickerProps} />
<LinkModal {...modalProps} />
</>;
};

return <Fragment>
<LinkPicker {...linkProps} />
<LinkModal {...modalProps} />
</Fragment>;
LinkField.propTypes = {
value: PropTypes.number.isRequired,
onChange: PropTypes.func.isRequired,
};

const stringifyData = (Component) => (({ data, value, ...props }) => {
let dataValue = value || data;
if (typeof dataValue === 'string') {
dataValue = JSON.parse(dataValue);
}
return <Component dataStr={JSON.stringify(dataValue)} {...props} data={dataValue} />;
// redux actions loaded into props - used to get toast notifications
const mapDispatchToProps = (dispatch) => ({
actions: {
toasts: bindActionCreators(toastsActions, dispatch),
},
});

export default compose(
inject(['LinkPicker', 'Loading']),
injectGraphql('readLinkTypes'),
stringifyData,
injectGraphql('readLinkDescription'),
fieldHolder
fieldHolder,
connect(null, mapDispatchToProps)
)(LinkField);
10 changes: 10 additions & 0 deletions client/src/components/LinkModal/FileLinkModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import i18n from 'i18n';
import React, {useEffect} from 'react';
import InsertMediaModal from 'containers/InsertMediaModal/InsertMediaModal';
import {connect} from "react-redux";
import LinkType from 'types/LinkType';
import PropTypes from 'prop-types';

const FileLinkModal = ({type, editing, data, actions, onSubmit, ...props}) => {

Expand Down Expand Up @@ -46,6 +48,14 @@ const FileLinkModal = ({type, editing, data, actions, onSubmit, ...props}) => {
/>;
}

FileLinkModal.propTypes = {
type: LinkType.isRequired,
editing: PropTypes.bool.isRequired,
data: PropTypes.object.isRequired,
actions: PropTypes.object.isRequired,
onClick: PropTypes.func.isRequired,
};

function mapStateToProps() {
return {};
}
Expand Down
Loading