Editorial workflow dependency tracking #243

Benaiah · 2017-02-23T02:34:31Z

- Summary

Dependency tracking for unpublished entries: closes #192.

- Test plan

Existing test suite passes.
State actions manually tested by dispatching appropriate actions.
Dependency resolution and publishing of multiple entries manually tested on benaiah/netlify-cms-test-repo.
pre-commit hooks skipped since they complain about code already in master.

- Description for the changelog

Add dependency tracking and multiple-entry publishing.

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

Benaiah · 2017-02-23T20:25:09Z

src/reducers/editorialWorkflow.js

@@ -76,6 +77,15 @@ const unpublishedEntries = (state = null, action) => {
      // Update Optimistically
      return state.deleteIn(['entities', `${ action.payload.collection }.${ action.payload.slug }`]);

+    case UNPUBLISHED_ENTRY_REGISTER_DEPENDENCY:
+      return state.setIn([


This is currently storing the dependencies in entry.data.dependencies, where it is then grabbed in persistUnpublishedEntry, added to options, and stored in metadata in editorialWorkflowGit. Should this be saving directly to entry.metaData instead?

calavera · 2017-02-24T23:04:18Z

example/config.yml

@@ -1,58 +1,29 @@
 backend:
-  name: test-repo


please, do not push changes to this file

This is temporary, it'll be rebased out before this leaves WIP.

calavera · 2017-02-24T23:06:35Z

src/actions/editorialWorkflow.js

+  };
+}
+
+function unpublishedEntryDependenciesRequest (collection, slug) {


please, remove space between method name and parameters.

Will do (ditto to those below).

calavera · 2017-02-24T23:06:45Z

src/actions/editorialWorkflow.js

+  };
+}
+
+function unpublishedEntryDependenciesSuccess (collection, slug, dependencies) {


please, remove space between method name and parameters.

calavera · 2017-02-24T23:06:51Z

src/actions/editorialWorkflow.js

+  };
+}
+
+function unpublishedEntryDependenciesError (collection, slug, error) {


please, remove space between method name and parameters.

calavera · 2017-02-24T23:07:02Z

src/actions/editorialWorkflow.js

+
+export const registerUnpublishedEntryDependency = unpublishedEntryRegisterDependency;
+
+export function getUnpublishedEntryDependencies (collection, slug) {


please, remove space between method name and parameters.

calavera · 2017-02-24T23:09:17Z

src/actions/editorialWorkflow.js

+      "dependencies",
+    ];
+
+    const getEventualDependencies = (paths, loadedDeps, state) =>


why is this function defined inside the parent function? please, extract.

This function is only useful within getUnpublishedEntryDependencies - I didn't want it exposed to the outer scope since it's very specific to the functionality of traverse. I'm fine with extracting it, that was just the reasoning behind nesting it with traverse.

calavera · 2017-02-24T23:12:26Z

src/actions/editorialWorkflow.js

+           .then(() => path);
+       });
+
+    const traverse = (collectedDeps, path) => {


why is this function defined inside the parent function? please, extract.

This function closes over getState, which is why it was defined here. I can hoist it and make getState an argument, though.

calavera · 2017-02-24T23:14:29Z

src/actions/editorialWorkflow.js

+    };
+
+    // Begin traversal
+    return traverse(new Immutable.List(), `${ collection }.${ slug }`)


If you use a Map rather than a List, you'll avoid the O(N) lookup per iteration in the traverse call.

Good point, thanks!

Benaiah · 2017-02-25T00:21:58Z

@calavera I've addressed your comments on editorialWorkflow.js - thanks for the review!

biilmann

Finally got some time to go through this. Still want to see if I can find some time to dig a little more into the traverse function - it feels like it could be cleaned up a bit, but I would have to spend a bit more understanding it to get a closer look.

Apart from some minor things, the main issue I see is keeping the dependency state in in entry.data.dependencies, when entry.data should be the actual data that will be stored in git when persisting the entry.

Apart from that a widget would also need to be able to remove or change a dependency - and I'm wondering if there's any good way to handle cases with a list of relation widgets (what happens when a list item is removed).

biilmann · 2017-02-26T03:08:12Z

src/actions/editorialWorkflow.js

+  const state = getState();
+
+  if (collectedDeps.get(path) === true) {
+    return new Promise(resolve => resolve(collectedDeps));


return Promise.resolve(collectedDeps) is cleaner

biilmann · 2017-02-26T03:09:17Z

src/actions/editorialWorkflow.js

+  if (!state.editorialWorkflow.hasIn(depsPath) ||
+    state.editorialWorkflow.getIn(depsPath).size === 0) {
+    // This path has no dependencies
+    return new Promise(resolve => resolve(newDeps));


return Promise.resolve(newDeps)

biilmann · 2017-02-26T03:33:22Z

src/reducers/entryDraft.js

@@ -73,6 +74,10 @@ const entryDraftReducer = (state = Map(), action) => {
    case REMOVE_ASSET:
      return state.update('mediaFiles', list => list.filterNot(path => path === action.payload));

+    case UNPUBLISHED_ENTRY_REGISTER_DEPENDENCY:
+      return state.setIn(['entry', 'data', 'dependencies', action.payload.field],


The entry data is what gets persisted in the repository - we don't want to add "dependencies" to the frontmatter of the entry and the name could easily class with a field.

Either we should store them in something like entry.dependencies or do like the assets and have dependencies in the state for entryDraft.

Ah, thanks for clarifying that. We definitely don't want this clashing with frontmatter. I'll also change the code in src/backends/backend.js/persistEntry to pass the dependencies somewhere other than parsedData, for the same reason (perhaps in the options object).

Touching on that last point, I was originally passing the deps into the backend in persistUnpublishedEntry, but it seemed that it wasn't getting called when creating a new unpublished entry, so dependencies that were created when the entry was first created weren't being saved. Is this behavior of persistUnpublishedEntry intentional or a bug?

biilmann · 2017-02-26T03:35:02Z

src/components/Widgets/ControlHOC.js

    return React.createElement(controlComponent, {
      field,
      value,
      metadata,
      onChange,
      onAddAsset,
      onRemoveAsset,
+      registerDependency,


seems like onAddDependency and onRemoveDependency would be more in line with the other callback functions in terms of naming (and it should be possible to remove a dependency as well as adding one...)

Yeah, that's a good point re: naming. See my comment below for more on the removal and updating.

Benaiah · 2017-02-27T20:31:20Z

@biilmann updating a dependency already works (in the case of a 1-to-1 field->dependency relationship; there isn't any functionality to handle a 1-to-many field->dependencies relationship). Removing a dependency is unimplemented - perhaps this should just be triggered whenever a relation field changes (other than selecting a suggestion)?

Benaiah · 2017-02-28T01:04:39Z

@biilmann I've pushed changes that address your comments. I've moved the dependency storage to entry.dependencies, renamed the registerDependency prop to onAddDependency, added an onRemoveDependency prop (this is currently a noop), and changed the immediately-resolving promises to use Promise.resolve.

I'm working on hooking this into the publishUnpublishedEntry action, and I noticed something - if I do the simplest implementation, the traversal doesn't work as the current entry is optimistically removed from the editorialWorkflow store on the publish request. I could either grab the dependencies from the entry object itself, which would be a bit messier as traverse would be implemented two different ways (once for the root entry, once for the rest); or I could only fire the publish request after the dependency check. Thoughts?

calavera · 2017-03-01T02:00:48Z

or I could only fire the publish request after the dependency check. Thoughts?

I think firing the publish request when the we're sure everything is correct might be more expected. @biilmann wdyt?

Benaiah · 2017-03-07T03:12:39Z

@calavera @biilmann I've pushed an updated version with all the functionality, including dependency removal. It currently uses window.confirm for UI - I think building out a custom modal interface should wait until the UI stuff has settled down a bit, and the search issue I've mentioned previously (unpublished dependencies don't show up in relation widget suggestions) means you can't register dependencies yet from the UI anyway.

Benaiah · 2017-03-09T22:14:08Z

@calavera @biilmann I've clarified traverse a bit more by pulling the promise array reduction into its own function, reducePromises. The function seems pretty out-of-place in src/actions/editorialWorkflow.js though - what do you think about moving it somewhere, perhaps lib/promiseHelper.js?

I've also rebased this onto the latest master.

Benaiah · 2017-03-11T01:01:33Z

@calavera @biilmann I've removed the temporary config.yml commit that I was using for testing.

calavera · 2017-03-11T18:52:07Z

LGTM, but unfortunately, there are some conflicts.

Benaiah · 2017-03-11T22:47:30Z

@calavera I'll get it rebased again.

Benaiah · 2017-03-11T22:58:25Z

Blech, tried to git in the morning and ended up accidentally squashing two commits. It looks like all the changes are still there, but the changes ended up in two commits instead of the four I had before. It is rebased onto the current master now, though.

- New actions, `UNPUBLISHED_ENTRY_REGISTER_DEPENDENCY` and `UNPUBLISHED_ENTRY_UNREGISTER_DEPENDENCY`, in `src/actions/editorialWorkflow.js`, with exported action creators. - New reducers case in `src/reducers/entryDraft.js` for (un)registering dependencies to the current `entryDraft.entry`. - `persistEntry` in `src/backends/backend.js` pulls dependency data out of `entryDraft` and saves it to metadata. - New widget control props, `onAddDependency` and `onRemoveDependency` passed down to widget controls from `EntryPageHOC`. These call `(un)registerUnpublishedEntryDependency`. - `RelationControl` calls `onAddDependency` when selecting a suggestion.

- New action constants in `src/actions/editorialWorkflow.js` - `UNPUBLISHED_ENTRY_DEPENDENCIES_REQUEST`, `UNPUBLISHED_ENTRY_DEPENDENCIES_SUCCESS`, and `UNPUBLISHED_ENTRY_DEPENDENCIES_FAILURE`, with exported thunk action creators. - `loadUnpublishedEntry` returns its promise so it can be used to load dependencies. - Add new thunk action `getUnpublishedEntryDependencies` to resolve the dependencies of a given entry.

- New action constants in `src/editorialWorkflow.js` - `UNPUBLISHED_ENTRIES_PUBLISH_REQUEST`, `UNPUBLISHED_ENTRIES_PUBLISH_SUCCESS`, and `UNPUBLISHED_ENTRIES_PUBLISH_FAILURE`, with exported thunk action creators. - New backend and GitHub API function `publishUnpublishedEntries`. - New reducer case in `src/reducers/editorialWorkflow.js` which updates editorialWorkflow state when publishing multiple entries.

Benaiah · 2017-03-11T23:36:10Z

@calavera, @biilmann OK, got that fixed. It's now rebased onto current master with the correct history (thanks, git reflog).

Benaiah · 2017-08-11T19:25:18Z

I'm closing this PR for now, since it's quite old and not ready for release. I'll reopen this once work starts on it again, but I don't want anyone mistakenly reviewing what I have here since it needs to be rebased and I'm not currently working on it.

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from 10747bc to 071e956 Compare February 23, 2017 20:22

Benaiah commented Feb 23, 2017

View reviewed changes

Benaiah force-pushed the editorial-workflow-dependency-tracking branch 6 times, most recently from 8735aaa to 24d5dfc Compare February 24, 2017 21:45

calavera reviewed Feb 24, 2017

View reviewed changes

calavera suggested changes Feb 24, 2017

View reviewed changes

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from 24d5dfc to 9faf2ed Compare February 25, 2017 00:21

biilmann suggested changes Feb 26, 2017

View reviewed changes

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from 9faf2ed to abb067c Compare February 28, 2017 00:57

Benaiah force-pushed the editorial-workflow-dependency-tracking branch 4 times, most recently from 1dc91cc to eca9c97 Compare March 6, 2017 18:54

Benaiah force-pushed the editorial-workflow-dependency-tracking branch 2 times, most recently from ea55a23 to 291eb13 Compare March 9, 2017 22:11

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from 291eb13 to 65595a6 Compare March 11, 2017 01:00

calavera approved these changes Mar 11, 2017

View reviewed changes

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from 65595a6 to a1a8096 Compare March 11, 2017 22:53

Benaiah added 3 commits March 11, 2017 15:23

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from a1a8096 to d73a80d Compare March 11, 2017 23:35

Benaiah changed the title ~~[WIP] Editorial workflow dependency tracking~~ Editorial workflow dependency tracking Mar 13, 2017

Require dependency publishing to publish unpublished entry

f74b719

Benaiah force-pushed the editorial-workflow-dependency-tracking branch from d73a80d to f74b719 Compare March 14, 2017 18:44

Benaiah closed this Aug 11, 2017

verythorough mentioned this pull request Aug 12, 2017

Dependency system for relation fields & editorial workflow. #192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial workflow dependency tracking #243

Editorial workflow dependency tracking #243

Benaiah commented Feb 23, 2017 •

edited

Benaiah Feb 23, 2017

calavera Feb 24, 2017

Benaiah Feb 24, 2017

calavera Feb 24, 2017

Benaiah Feb 24, 2017

calavera Feb 24, 2017

calavera Feb 24, 2017

calavera Feb 24, 2017

calavera Feb 24, 2017

Benaiah Feb 24, 2017 •

edited

calavera Feb 24, 2017

Benaiah Feb 24, 2017

calavera Feb 24, 2017 •

edited

Benaiah Feb 24, 2017

Benaiah commented Feb 25, 2017

biilmann left a comment

biilmann Feb 26, 2017

biilmann Feb 26, 2017

biilmann Feb 26, 2017

Benaiah Feb 27, 2017 •

edited

biilmann Feb 26, 2017

Benaiah Feb 27, 2017

Benaiah commented Feb 27, 2017 •

edited

Benaiah commented Feb 28, 2017

calavera commented Mar 1, 2017

Benaiah commented Mar 7, 2017

Benaiah commented Mar 9, 2017 •

edited

Benaiah commented Mar 11, 2017

calavera commented Mar 11, 2017

Benaiah commented Mar 11, 2017

Benaiah commented Mar 11, 2017 •

edited

Benaiah commented Mar 11, 2017

Benaiah commented Aug 11, 2017


		export const registerUnpublishedEntryDependency = unpublishedEntryRegisterDependency;

		export function getUnpublishedEntryDependencies (collection, slug) {

Editorial workflow dependency tracking #243

Editorial workflow dependency tracking #243

Conversation

Benaiah commented Feb 23, 2017 • edited

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

Benaiah Feb 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calavera Feb 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah commented Feb 25, 2017

biilmann 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

Benaiah Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah commented Feb 27, 2017 • edited

Benaiah commented Feb 28, 2017

calavera commented Mar 1, 2017

Benaiah commented Mar 7, 2017

Benaiah commented Mar 9, 2017 • edited

Benaiah commented Mar 11, 2017

calavera commented Mar 11, 2017

Benaiah commented Mar 11, 2017

Benaiah commented Mar 11, 2017 • edited

Benaiah commented Mar 11, 2017

Benaiah commented Aug 11, 2017

Benaiah commented Feb 23, 2017 •

edited

Benaiah Feb 24, 2017 •

edited

calavera Feb 24, 2017 •

edited

Benaiah Feb 27, 2017 •

edited

Benaiah commented Feb 27, 2017 •

edited

Benaiah commented Mar 9, 2017 •

edited

Benaiah commented Mar 11, 2017 •

edited