Entry deletion for the simple workflow #485

Benaiah · 2017-07-13T22:38:17Z

- Summary

This PR adds a "Delete" button to the entry page in the simple workflow which deletes the current entry from the backend. ~~This does not close #291 completely~~, but it is most of the way there (the remaining work to close that ticket is to implement deletion in the editorial workflow, which is in progress in the delete-entries-in-editorial-workflow branch.

- Test plan

Existing tests pass, relevant spec tests were updated, and the PR was manually tested with editorial workflow both enabled and disabled.

- Description for the changelog

Entry deletion for the simple workflow

- A picture of a cute animal (not mandatory but encouraged)

erquhart

Awesome work, just a couple of clean up requests.

erquhart · 2017-07-17T19:37:23Z

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: true };


showDelete should also be false if the entry is new.

Currently it deletes your unsaved changes - that seemed like the most obvious approach to me. I'll switch this out.

Is this change still on the way?

erquhart · 2017-07-17T19:43:42Z

src/containers/EntryPage.js

  };

  handlePersistEntry = () => {
    const { persistEntry, collection } = this.props;
    setTimeout(() => {
-      persistEntry(collection);  
+      persistEntry(collection).then(() => this.handleCloseEntry());


erquhart · 2017-07-17T19:53:35Z

src/backends/github/implementation.js

@@ -1,4 +1,5 @@
 import semaphore from "semaphore";
+import { resolvePromiseProperties } from "../../lib/promiseHelper";


Unused import.

Thanks for the catch!

erquhart · 2017-07-17T19:55:44Z

src/backends/github/API.js

+    .then(({ sha }) => this.request(
+      `${ this.repoURL }/contents/${ path }`,
+      {
+        method: "DELETE",


Go with single quotes for this and other quotes in the PR (except React component attributes).

erquhart · 2017-07-17T19:58:16Z

src/backends/github/API.js

+  deleteFile(path, message, options={}) {
+    const branch = options.branch || this.branch;
+    // We need to request the file first to get the SHA
+    return this.request(`${ this.repoURL }/contents/${ path }`)


Nitpick: we can store the assembled url above the return and reuse.

erquhart · 2017-07-17T20:00:29Z

src/backends/github/API.js

+    // deletion should be idempotent, so we can consider this a
+    // success.
+    .catch((err) => {
+      if (err.message === "Reference does not exist") {


Nothing less ephemeral to check against, such as an error code?

The error code here is a 422, and there's no other metadata available there other than the error message. 422 is a very general error code in the GitHub API, so resolving this promise for all 422s will other errors, so I checked the more specific error message.

https://developer.github.com/v3/#client-errors says clients should differentiate 422 errors based on the body of the response (the APIError wrapper we use isn't including this information, which should probably be changed in another PR). I was hoping there would be more metadata in the request, but when inspecting a relevant 422 response manually, only the message and documentation_url were present, leaving us with just the message and status to switch between. Given GitHub's API documentation, it seems that the message is intended to be used to differentiate errors, so I used that.

If you'd rather catch all 422 errors, I can change this pretty trivially (just need to change the condition to err.status === 422).

Okay, this is fine as is. Thanks for the explanation.

erquhart

LGTM

erquhart · 2017-07-22T02:38:53Z

@carlflor would you be up for reviewing this PR? You mentioned contributing, and code review is one area that needs much more community activity; we prefer to have two maintainers/collaborators okay a PR before merging. Let me know!

carlflor · 2017-07-22T02:41:13Z

@erquhart okay! cool! I can review this :)

carlflor

Looking good! :)

Benaiah · 2017-07-22T06:40:50Z

@carlflor thanks for the review!

Deletion was added in #485, but the function for the `test-repo` backend was `deleteEntry` instead of `deleteFile` like it was supposed to be. Also, setting the key for a deleted file to `undefined` did not really remove that file from the object, so there were then errors stating `file.content` is not defined. `delete`ing the "file" from the object fixes this bug.

mbergeronupgrade · 2018-05-04T17:45:10Z

@Benaiah I think in editorial mode, the delete entry button does not respect the delete: false option from config.yml

tech4him1 · 2018-05-04T19:23:22Z

@mbergeronupgrade Please open a new issue for that.

Entry deletion for the simple workflow

e3ea497

erquhart suggested changes Jul 17, 2017

View reviewed changes

Benaiah force-pushed the delete-entries-in-simple-workflow branch from 49a3e32 to 82cd253 Compare July 21, 2017 02:07

Address code review comments

ef2f819

Benaiah force-pushed the delete-entries-in-simple-workflow branch from 82cd253 to ef2f819 Compare July 21, 2017 02:09

Hide delete button for new entries

9214c20

erquhart approved these changes Jul 22, 2017

View reviewed changes

carlflor approved these changes Jul 22, 2017

View reviewed changes

Benaiah merged commit dac57c6 into master Jul 22, 2017

tech4him1 mentioned this pull request Aug 4, 2017

Ability to delete cards outside of editorial workflow #291

Closed

tech4him1 mentioned this pull request Aug 17, 2017

Cannot delete test post(s) in example seed data #537

Closed

tech4him1 mentioned this pull request Aug 17, 2017

Fix deletion for the test-repo backend. #538

Merged

tech4him1 deleted the delete-entries-in-simple-workflow branch August 26, 2017 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entry deletion for the simple workflow #485

Entry deletion for the simple workflow #485

Benaiah commented Jul 13, 2017 •

edited

Loading

erquhart left a comment

erquhart Jul 17, 2017

Benaiah Jul 21, 2017

erquhart Jul 21, 2017

erquhart Jul 17, 2017

erquhart Jul 17, 2017

Benaiah Jul 21, 2017

erquhart Jul 17, 2017

Benaiah Jul 21, 2017

erquhart Jul 17, 2017

erquhart Jul 17, 2017

Benaiah Jul 20, 2017

erquhart Jul 21, 2017

erquhart left a comment

erquhart commented Jul 22, 2017 •

edited

Loading

carlflor commented Jul 22, 2017

carlflor left a comment

Benaiah commented Jul 22, 2017

mbergeronupgrade commented May 4, 2018

tech4him1 commented May 4, 2018

		@@ -1,4 +1,5 @@
		import semaphore from "semaphore";
		import { resolvePromiseProperties } from "../../lib/promiseHelper";

Entry deletion for the simple workflow #485

Entry deletion for the simple workflow #485

Conversation

Benaiah commented Jul 13, 2017 • edited Loading

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart left a comment

Choose a reason for hiding this comment

erquhart commented Jul 22, 2017 • edited Loading

carlflor commented Jul 22, 2017

carlflor left a comment

Choose a reason for hiding this comment

Benaiah commented Jul 22, 2017

mbergeronupgrade commented May 4, 2018

tech4him1 commented May 4, 2018

Benaiah commented Jul 13, 2017 •

edited

Loading

erquhart commented Jul 22, 2017 •

edited

Loading