Problem: When a user goes to save a new episode, there is a potential to double submit an episode/show/ancillary if the user double clicks fast enough
Notes :
- Functionally Complex
The current update function for
writeEpisode(found ingulp/assets/javascripts/stores/actions/updateActions.js) is hopelessly too complex and not very legiable, and handles both contexts for updating and saving new episodes.
export const writeEpisode = () => {
return(dispatch, getState) => {
let { deliveryLineItemState, showState, presentationalState } = getState();
let { full_episode, episode_copy } = showState;
let { id, attributes } = full_episode;
const { deliveryLineItems, deliveryLineItemIds } = deliveryLineItemState;
const lineItemsAreDirty = deliveryLineItemState.dirty;
const showId = attributes.show_id;
const url = id ? `/shows/${showId}/episodes/${id}` : `/shows/${showId}/episodes`;
const requestAction = id ? 'PATCH' : 'POST';
const platform = attributes.platform.toLowerCase();
const requiredFields = (platform === 'youtube') ? ['title', 'producers'] : ['title']
const validForm = checkRequiredFields(requiredFields, attributes, dispatch);
const strippedLineItems = deliveryLineItemIds.map(id => removeIdsFromIdObject('fieldValidation', deliveryLineItems[id]));
if (validForm) {
let epData = (requestAction === 'POST') ? attributes : getChangedAttributes(episode_copy.attributes, attributes);
if (Object.keys(epData).length > 0 || lineItemsAreDirty) {
// Only send delivery line items if something has changed
const lineItemParams = lineItemsAreDirty ? strippedLineItems : {}
return request(requestAction, url)
.set('Content-Type', 'application/json')
.withCredentials()
.send({
episode: {
...epData,
delivery_line_items: lineItemParams
},
})
.query({ type: full_episode.type })
.then(
success => {
if (epData.project_code) {
mixpanelInstance.track(`Edited A Project Code On Episode`, { episode_id: success.body.episode.id })
}
return dispatch(episodeSuccess(success, requestAction, presentationalState.cancelModalIsOpen))
},
error => {
const { errors, errored_object, record_id, text } = error.response.body;
if (errored_object === "DeliveryLineItem") {
if (presentationalState.cancelModalIsOpen) {
dispatch({ type: 'TOGGLE_CANCEL_MODAL' });
}
dispatch({
type: 'ADD_FIELD_ERROR',
value: {
fieldName: `deliveryLineItem-${record_id}`,
fieldMessage: 'Invalid Delivery Line Item.'
}
});
return dispatch({
type: 'deliveryLineItem/SET_INVALID_FIELD',
value: {
fieldName: 'delivery_destination_id',
deliveryLineItemId: record_id,
fieldMessage: errors.destination[0]
}
})
} else {
return dispatch({ type: 'POST_ERROR', value: error.response.text })
}
},
)
} else {
if (presentationalState.cancelModalIsOpen) {
dispatch({ type: 'TOGGLE_CANCEL_MODAL' });
}
return dispatch({ type: 'TOGGLE_EDIT' })
}
}
}
}
const checkRequiredFields = (fields, attributes, dispatch) => {
let validForm = true;
fields.map((field) => {
let dispatcherStatus = dispatch(checkValidity(field, attributes[field]));
if (dispatcherStatus.type === 'ADD_FIELD_ERROR') {
validForm = false;
}
});
return validForm;
};
I've written an attempt to break up some of the logic in this into smaller sub functions for easier readability
export const selectDeliveryLineItemEpisode = ({ deliveryLineItems, deliveryLineItemIds }) => {
return deliveryLineItemIds.map(
id => removeIdsFromIdObject('fieldValidation', deliveryLineItems[id])
)
}
export const checkEpisodeValidity = () => (dispatch, getState) => {
let { attributes } = getState().showState.full_episode;
let mustValidateFields = ['title'];
if (attributes.platform.toLowerCase() == 'youtube') {
mustValidateFields.push('producers');
}
mustValidateFields.forEach(
(field) => dispatch(
checkValidity(field, attributes[field])
)
)
return;
}
const changedEpisodeAttributes = () => ({ full_episode, episode_copy }) => {
if (!full_episode.id) {
return full_episode.attributes;
} else {
let nextAttr = Object.keys(full_episode.attributes).reduce((key, reducedAttributes) => {
let newerEditedAttributeValue = full_episode.attributes[key]
let valueExisting = episode_copy.attributes[key]
if (valueExisting !== newerEditedAttributeValue) {
reducedAttributes[key] = newerEditedAttributeValue
}
}, {})
return (Object.keys(nextAttr).length > 0) ? nextAttr : false
}
}
export const writeEpisode = () => (dispatch, getState) => {
let {deliveryLineItemState, showState} = getState();
let cleanedDeliveryLineItemObjs = selectDeliveryLineItemEpisode(deliveryLineItemState)
const makeEpisodeRequest = epData => {
if (epData) {
const epId = showState.full_episode.id;
const showId = showState.full_episode.attributes.show_id
const url = epId ? `/shows/${showId}/episodes/${epId}` : `/shows/${showId}/episodes`;
const requestAction = epId ? 'PATCH' : 'POST';
return request(requestAction, url)
.set('Content-Type', 'application/json')
.withCredentials()
.send({
episode: Object.assign(epData, {
delivery_line_items: cleanedDeliveryLineItemObjs
})
})
.query({type: showState.full_episode.type})
} else {
return Promise.reject({noChange: true})
}
}
Promise.resolve(
dispatch(checkEpisodeValidity())
).then(
() => (
!getState().deliveryLineItemState.dirty &&
getState().formValidationState.isFormValid
)
).then(
ableToSave => (ableToSave ? changedEpisodeAttributes(showState) : false)
).then(
makeEpisodeRequest
).then(
success => {
if (epData.project_code) {
mixpanelInstance.track(`Edited A Project Code On Episode`, { episode_id: success.body.episode.id })
}
return dispatch(episodeSuccess(success, requestAction, presentationalState.cancelModalIsOpen))
},
error => {
if (error.noChange) {
if (presentationalState.cancelModalIsOpen) {
dispatch({ type: 'TOGGLE_CANCEL_MODAL' });
}
return dispatch({ type: 'TOGGLE_EDIT' })
}
const { errors, errored_object, record_id, text } = error.response.body;
}
)
}
- Tightly Woven Conditional Gating but more looming of a concern is how we currently determine whether or not a save button should be enabled or not.
The conditional that is evaluated for saving (seen in DetailsNavigationHeader in gulp/assets/javascripts/components/navigation)
const disableSave = (!enableEditing || !isFormValid || this._titleMatchFound() || createContext === 'film')
involves checking across a very vague set of rules that is brittle and inconsistent,
we could simplify this by making only a single disableSave property passed into the DetailsNavigationHeader.
that we could administer validation to externally.
// in presentationalState
case `TOGGLE_ABLE_TO_SAVE`:
return {
...currentState,
ableToSave: value
}
//in some hypothetical actions file
const validateSave = () => (dispatch, getState) => {
let { isCreating, isEditing } = getState().showState;
//validate title and/or producers on save of
let { attributes } = getState().showState.full_episode;
let mustValidateFields = ['title'];
if (attributes.platform.toLowerCase() == 'youtube') {
mustValidateFields.push('producers');
}
mustValidateFields.forEach(
(field) => dispatch(
checkValidity(field, attributes[field])
)
)
let { dirty, fieldValidation } = getState().formValidationState;
// check to see if there are any errors from the validations
if (dirty) {
//update the button to be disabled
return dispatch({type: "TOGGLE_ABLE_TO_SAVE", value: false})
}
if ( getState().userState.userPrivileges.includes('write_metadata') ) {
return dispatch({type: "TOGGLE_ABLE_TO_SAVE", value: false})
}
//other validations to follow
}
// Futhermore within the DetailsNavigationHeader the disable button becomes a single prop passed in, agnostic to the other properties around it.
(ableToSave && createContext !== 'film') ? (
<a className="btn btn-primary btn-lg btn-sq hidden-xs hidden-sm"
data-uat={uatSavingTarget}
disabled={ableToSave} onClick={this._handleActionBtn}>
{this._saveButtonMarkup()}
</a>
) : null}
We would need to apply this across Films, Shows and Misc Detail pages