From dac57c60a0b245603f4ceccab4a467ee0b2b3411 Mon Sep 17 00:00:00 2001 From: Benaiah Mischenko Date: Fri, 21 Jul 2017 23:40:33 -0700 Subject: [PATCH] Entry deletion for the simple workflow (#485) --- src/actions/entries.js | 66 +++++++++++++++++-- src/backends/backend.js | 6 ++ src/backends/github/API.js | 36 ++++++++-- src/backends/github/implementation.js | 4 ++ src/backends/test-repo/implementation.js | 6 ++ src/components/EntryEditor/EntryEditor.js | 6 ++ .../EntryEditor/EntryEditorToolbar.js | 10 +++ .../__tests__/EntryEditorToolbar.spec.js | 2 + src/containers/EntryPage.js | 23 ++++++- .../editorialWorkflow/EntryPageHOC.js | 15 +++-- src/reducers/entries.js | 8 +++ src/reducers/entryDraft.js | 2 + 12 files changed, 167 insertions(+), 17 deletions(-) diff --git a/src/actions/entries.js b/src/actions/entries.js index d18e1208..ad54d99a 100644 --- a/src/actions/entries.js +++ b/src/actions/entries.js @@ -30,6 +30,10 @@ export const ENTRY_PERSIST_REQUEST = 'ENTRY_PERSIST_REQUEST'; export const ENTRY_PERSIST_SUCCESS = 'ENTRY_PERSIST_SUCCESS'; export const ENTRY_PERSIST_FAILURE = 'ENTRY_PERSIST_FAILURE'; +export const ENTRY_DELETE_REQUEST = 'ENTRY_DELETE_REQUEST'; +export const ENTRY_DELETE_SUCCESS = 'ENTRY_DELETE_SUCCESS'; +export const ENTRY_DELETE_FAILURE = 'ENTRY_DELETE_FAILURE'; + /* * Simple Action Creators (Internal) * We still need to export them for tests @@ -126,6 +130,37 @@ export function entryPersistFail(collection, entry, error) { }; } +export function entryDeleting(collection, slug) { + return { + type: ENTRY_DELETE_REQUEST, + payload: { + collectionName: collection.get('name'), + entrySlug: slug, + }, + }; +} + +export function entryDeleted(collection, slug) { + return { + type: ENTRY_DELETE_SUCCESS, + payload: { + collectionName: collection.get('name'), + entrySlug: slug, + }, + }; +} + +export function entryDeleteFail(collection, slug, error) { + return { + type: ENTRY_DELETE_FAILURE, + payload: { + collectionName: collection.get('name'), + entrySlug: slug, + error: error.toString(), + }, + }; +} + export function emptyDraftCreated(entry) { return { type: DRAFT_CREATE_EMPTY, @@ -229,13 +264,13 @@ export function persistEntry(collection) { const entryDraft = state.entryDraft; // Early return if draft contains validation errors - if (!entryDraft.get('fieldsErrors').isEmpty()) return; + if (!entryDraft.get('fieldsErrors').isEmpty()) return Promise.reject(); const backend = currentBackend(state.config); const assetProxies = entryDraft.get('mediaFiles').map(path => getAsset(state, path)); const entry = entryDraft.get('entry'); dispatch(entryPersisting(collection, entry)); - backend + return backend .persistEntry(state.config, collection, entryDraft, assetProxies.toJS()) .then(() => { dispatch(notifSend({ @@ -243,8 +278,7 @@ export function persistEntry(collection) { kind: 'success', dismissAfter: 4000, })); - dispatch(entryPersisted(collection, entry)); - dispatch(closeEntry(collection)); + return dispatch(entryPersisted(collection, entry)); }) .catch((error) => { dispatch(notifSend({ @@ -252,7 +286,29 @@ export function persistEntry(collection) { kind: 'danger', dismissAfter: 8000, })); - dispatch(entryPersistFail(collection, entry, error)); + return dispatch(entryPersistFail(collection, entry, error)); }); }; } + +export function deleteEntry(collection, slug) { + return (dispatch, getState) => { + const state = getState(); + const backend = currentBackend(state.config); + + dispatch(entryDeleting(collection, slug)); + return backend.deleteEntry(state.config, collection, slug) + .then(() => { + return dispatch(entryDeleted(collection, slug)); + }) + .catch((error) => { + dispatch(notifSend({ + message: `Failed to delete entry: ${ error }`, + kind: 'danger', + dismissAfter: 8000, + })); + console.error(error); + return dispatch(entryDeleteFail(collection, slug, error)); + }); + }; +} diff --git a/src/backends/backend.js b/src/backends/backend.js index 13010ead..1f027628 100644 --- a/src/backends/backend.js +++ b/src/backends/backend.js @@ -230,6 +230,12 @@ class Backend { }); } + deleteEntry(config, collection, slug) { + const path = selectEntryPath(collection, slug); + const commitMessage = `Delete ${ collection.get('label') } “${ slug }”`; + return this.implementation.deleteFile(path, commitMessage); + } + persistUnpublishedEntry(config, collection, entryDraft, MediaFiles) { return this.persistEntry(config, collection, entryDraft, MediaFiles, { unpublished: true }); } diff --git a/src/backends/github/API.js b/src/backends/github/API.js index 51ee3a26..19571f94 100644 --- a/src/backends/github/API.js +++ b/src/backends/github/API.js @@ -201,7 +201,12 @@ export default class API { // Get PRs with a `head` of `branchName`. Note that this is a // substring match, so we need to check that the `head.ref` of // at least one of the returned objects matches `branchName`. - return this.request(`${ this.repoURL }/pulls?head=${ branchName }&state=open`) + return this.request(`${ this.repoURL }/pulls`, { + params: { + head: branchName, + state: 'open', + }, + }) .then(prs => prs.some(pr => pr.head.ref === branchName)); })) .catch((error) => { @@ -258,6 +263,21 @@ export default class API { }); } + deleteFile(path, message, options={}) { + const branch = options.branch || this.branch; + const fileURL = `${ this.repoURL }/contents/${ path }`; + // We need to request the file first to get the SHA + return this.request(fileURL) + .then(({ sha }) => this.request(fileURL, { + method: "DELETE", + params: { + sha, + message, + branch, + }, + })); + } + editorialWorkflowGit(fileTree, entry, filesList, options) { const contentKey = entry.slug; const branchName = `cms/${ contentKey }`; @@ -343,10 +363,18 @@ export default class API { deleteUnpublishedEntry(collection, slug) { const contentKey = slug; - let prNumber; return this.retrieveMetadata(contentKey) .then(metadata => this.closePR(metadata.pr, metadata.objects)) - .then(() => this.deleteBranch(`cms/${ contentKey }`)); + .then(() => this.deleteBranch(`cms/${ contentKey }`)) + // If the PR doesn't exist, then this has already been deleted - + // deletion should be idempotent, so we can consider this a + // success. + .catch((err) => { + if (err.message === "Reference does not exist") { + return Promise.resolve(); + } + return Promise.reject(err); + }); } publishUnpublishedEntry(collection, slug) { @@ -374,7 +402,7 @@ export default class API { deleteRef(type, name, sha) { return this.request(`${ this.repoURL }/git/refs/${ type }/${ encodeURIComponent(name) }`, { - method: "DELETE", + method: 'DELETE', }); } diff --git a/src/backends/github/implementation.js b/src/backends/github/implementation.js index 600d7f93..c0473587 100644 --- a/src/backends/github/implementation.js +++ b/src/backends/github/implementation.js @@ -83,6 +83,10 @@ export default class GitHub { return this.api.persistFiles(entry, mediaFiles, options); } + deleteFile(path, commitMessage, options) { + return this.api.deleteFile(path, commitMessage, options); + } + unpublishedEntries() { return this.api.listUnpublishedBranches().then((branches) => { const sem = semaphore(MAX_CONCURRENT_DOWNLOADS); diff --git a/src/backends/test-repo/implementation.js b/src/backends/test-repo/implementation.js index dfea59a2..61f0bebf 100644 --- a/src/backends/test-repo/implementation.js +++ b/src/backends/test-repo/implementation.js @@ -93,4 +93,10 @@ export default class TestRepo { return Promise.resolve(); } + deleteEntry(path, commitMessage) { + const folder = path.substring(0, path.lastIndexOf('/')); + const fileName = path.substring(path.lastIndexOf('/') + 1); + window.repoFiles[folder][fileName] = undefined; + return Promise.resolve(); + } } diff --git a/src/components/EntryEditor/EntryEditor.js b/src/components/EntryEditor/EntryEditor.js index 47b3c035..5413d12d 100644 --- a/src/components/EntryEditor/EntryEditor.js +++ b/src/components/EntryEditor/EntryEditor.js @@ -47,6 +47,8 @@ class EntryEditor extends Component { fieldsErrors, getAsset, onChange, + showDelete, + onDelete, onValidate, onAddAsset, onRemoveAsset, @@ -127,6 +129,8 @@ class EntryEditor extends Component { isPersisting={entry.get('isPersisting')} onPersist={this.handleOnPersist} onCancelEdit={onCancelEdit} + onDelete={onDelete} + showDelete={showDelete} /> @@ -145,6 +149,8 @@ EntryEditor.propTypes = { onChange: PropTypes.func.isRequired, onValidate: PropTypes.func.isRequired, onPersist: PropTypes.func.isRequired, + showDelete: PropTypes.bool.isRequired, + onDelete: PropTypes.func.isRequired, onRemoveAsset: PropTypes.func.isRequired, onCancelEdit: PropTypes.func.isRequired, }; diff --git a/src/components/EntryEditor/EntryEditorToolbar.js b/src/components/EntryEditor/EntryEditorToolbar.js index a5e29ba1..da5847e9 100644 --- a/src/components/EntryEditor/EntryEditorToolbar.js +++ b/src/components/EntryEditor/EntryEditorToolbar.js @@ -5,6 +5,8 @@ const EntryEditorToolbar = ( { isPersisting, onPersist, + showDelete, + onDelete, onCancelEdit, }) => { const disabled = isPersisting; @@ -19,6 +21,12 @@ const EntryEditorToolbar = ( { isPersisting ? 'Saving...' : 'Save' } {' '} + { showDelete + ? () + : '' } + { showDelete ? ' ' : '' } @@ -29,6 +37,8 @@ const EntryEditorToolbar = ( EntryEditorToolbar.propTypes = { isPersisting: PropTypes.bool, onPersist: PropTypes.func.isRequired, + showDelete: PropTypes.bool.isRequired, + onDelete: PropTypes.func.isRequired, onCancelEdit: PropTypes.func.isRequired, }; diff --git a/src/components/EntryEditor/__tests__/EntryEditorToolbar.spec.js b/src/components/EntryEditor/__tests__/EntryEditorToolbar.spec.js index e907c195..5da4eb64 100644 --- a/src/components/EntryEditor/__tests__/EntryEditorToolbar.spec.js +++ b/src/components/EntryEditor/__tests__/EntryEditorToolbar.spec.js @@ -8,6 +8,7 @@ describe('EntryEditorToolbar', () => { {}} onCancelEdit={() => {}} + onDelete={() => {}} /> ); const tree = component.html(); @@ -20,6 +21,7 @@ describe('EntryEditorToolbar', () => { isPersisting onPersist={() => {}} onCancelEdit={() => {}} + onDelete={() => {}} /> ); const tree = component.html(); diff --git a/src/containers/EntryPage.js b/src/containers/EntryPage.js index b377ffb8..059fa947 100644 --- a/src/containers/EntryPage.js +++ b/src/containers/EntryPage.js @@ -10,6 +10,7 @@ import { changeDraftField, changeDraftFieldValidation, persistEntry, + deleteEntry, } from '../actions/entries'; import { closeEntry } from '../actions/editor'; import { addAsset, removeAsset } from '../actions/media'; @@ -34,6 +35,8 @@ class EntryPage extends React.Component { entryDraft: ImmutablePropTypes.map.isRequired, loadEntry: PropTypes.func.isRequired, persistEntry: PropTypes.func.isRequired, + deleteEntry: PropTypes.func.isRequired, + showDelete: PropTypes.bool.isRequired, removeAsset: PropTypes.func.isRequired, closeEntry: PropTypes.func.isRequired, openSidebar: PropTypes.func.isRequired, @@ -79,16 +82,29 @@ class EntryPage extends React.Component { }; handleCloseEntry = () => { - this.props.closeEntry(); + return this.props.closeEntry(); }; handlePersistEntry = () => { const { persistEntry, collection } = this.props; setTimeout(() => { - persistEntry(collection); + persistEntry(collection).then(() => this.handleCloseEntry()); }, 0); }; + handleDeleteEntry = () => { + if (!window.confirm('Are you sure you want to delete this entry?')) { return; } + if (this.props.newEntry) { + return this.handleCloseEntry(); + } + + const { deleteEntry, entry, collection } = this.props; + const slug = entry.get('slug'); + setTimeout(() => { + deleteEntry(collection, slug).then(() => this.handleCloseEntry()); + }, 0); + } + render() { const { entry, @@ -124,6 +140,8 @@ class EntryPage extends React.Component { onAddAsset={addAsset} onRemoveAsset={removeAsset} onPersist={this.handlePersistEntry} + onDelete={this.handleDeleteEntry} + showDelete={this.props.showDelete} onCancelEdit={this.handleCloseEntry} /> ); @@ -162,6 +180,7 @@ export default connect( createEmptyDraft, discardDraft, persistEntry, + deleteEntry, closeEntry, openSidebar, } diff --git a/src/containers/editorialWorkflow/EntryPageHOC.js b/src/containers/editorialWorkflow/EntryPageHOC.js index c13a7b92..280cb67f 100644 --- a/src/containers/editorialWorkflow/EntryPageHOC.js +++ b/src/containers/editorialWorkflow/EntryPageHOC.js @@ -15,8 +15,9 @@ export default function EntryPageHOC(EntryPage) { function mapStateToProps(state, ownProps) { const { collections } = state; const isEditorialWorkflow = (state.config.get('publish_mode') === EDITORIAL_WORKFLOW); - const returnObj = { isEditorialWorkflow }; + const returnObj = { isEditorialWorkflow, showDelete: !ownProps.newEntry }; if (isEditorialWorkflow) { + returnObj.showDelete = false; const slug = ownProps.params.slug; const collection = collections.get(ownProps.params.name); const unpublishedEntry = selectUnpublishedEntry(state, collection.get('name'), slug); @@ -35,14 +36,16 @@ export default function EntryPageHOC(EntryPage) { if (isEditorialWorkflow) { // Overwrite loadEntry to loadUnpublishedEntry - returnObj.loadEntry = (collection, slug) => { + returnObj.loadEntry = (collection, slug) => dispatch(loadUnpublishedEntry(collection, slug)); - }; - + // Overwrite persistEntry to persistUnpublishedEntry - returnObj.persistEntry = (collection) => { + returnObj.persistEntry = collection => dispatch(persistUnpublishedEntry(collection, unpublishedEntry)); - }; + + // Overwrite deleteEntry to a noop (deletion is currently + // disabled in the editorial workflow) + returnObj.deleteEntry = () => undefined; } return { diff --git a/src/reducers/entries.js b/src/reducers/entries.js index fcc7c441..acd891bf 100644 --- a/src/reducers/entries.js +++ b/src/reducers/entries.js @@ -6,6 +6,7 @@ import { ENTRIES_REQUEST, ENTRIES_SUCCESS, ENTRIES_FAILURE, + ENTRY_DELETE_SUCCESS, } from '../actions/entries'; import { SEARCH_ENTRIES_SUCCESS } from '../actions/search'; @@ -61,6 +62,13 @@ const entries = (state = Map({ entities: Map(), pages: Map() }), action) => { )); }); + case ENTRY_DELETE_SUCCESS: + return state.withMutations((map) => { + map.deleteIn(['entities', `${ action.payload.collectionName }.${ action.payload.entrySlug }`]); + map.updateIn(['pages', action.payload.collectionName, 'ids'], + ids => ids.filter(id => id !== action.payload.entrySlug)); + }); + default: return state; } diff --git a/src/reducers/entryDraft.js b/src/reducers/entryDraft.js index f8c8fc6f..02ac895b 100644 --- a/src/reducers/entryDraft.js +++ b/src/reducers/entryDraft.js @@ -8,6 +8,7 @@ import { ENTRY_PERSIST_REQUEST, ENTRY_PERSIST_SUCCESS, ENTRY_PERSIST_FAILURE, + ENTRY_DELETE_SUCCESS, } from '../actions/entries'; import { UNPUBLISHED_ENTRY_PERSIST_REQUEST, @@ -76,6 +77,7 @@ const entryDraftReducer = (state = Map(), action) => { } case ENTRY_PERSIST_SUCCESS: + case ENTRY_DELETE_SUCCESS: case UNPUBLISHED_ENTRY_PERSIST_SUCCESS: return state.withMutations((state) => { state.deleteIn(['entry', 'isPersisting']);