feat: commit media with post #2851

erezrokah · 2019-11-07T12:08:07Z

Opening a new one since I can't re-open #2823 due to a forced commit (rebased branch).

This is a new (breaking) behaviour for the editorial workflow (not sure if we should make it the default).

When creating a new draft entry while in editorial workflow all media files will be stored on the client side until the first save (instead of being committed directly to the default branch). On first save the media files and entry will be pushed together as a single commit.

Any further changes will be synched between the browser and the corresponding branch.

Main changes:

Update GitHub backend to support multiple file persistence in a single commit. This includes adding/modifying/deleting multiple files in a single commit and required changing the way we create and update git trees.
Sync all the different locations media files are stored - redux store, asset store, local backup and GitHub repo.
Update test backend to support it since it is the only other backend that supports editorial workflow.
Re-record the fixtures data for the cypress tests due to the API changes.
UI changes to mark the media files as draft when in edit mode.
Added unit tests for almost every part of the code I touched.

Items left:

Regression tests for all other backends and non editorial flow mode for GitHub and test backend
Bug fixes
Add some tests to the media library UI components.
Decide if this should be the default behaviour
Code cleanup
Make sure I didn't break any media library integrations

Known issue:

Currently the editor components are not synced with the media library. For example, one can add an image component in the editor, choose an image from the media library, then delete that image from the media library and the image component will still show up in the editor. I think this is unrelated to this pull request so I won't fix it at the moment (ideally we would like to warn before deleting an image that is linked to an entry).

Stuff I found and fixed during my tests:

fix(external-media-libraries): support for external media libraries is broken #2857 : support for external media libraries is broken
fix(git-gateway): unpublished entries not loaded for git-gateway(GitHub) #2856 : unpublished entries not loaded for git-gateway(GitHub)

erezrokah · 2019-11-07T12:18:04Z

packages/netlify-cms-backend-github/src/API.js

-          parentSha: sha,
-        }));
-    });
+  async updateTree(sha, files) {


Used nested file paths to update the tree instead of recursion (@erquhart figured it out).
This is also the only way we got file deletion to work.

Should have some performance boost.

erezrokah · 2019-11-07T12:19:55Z

packages/netlify-cms-backend-github/src/implementation.js

@@ -322,7 +323,9 @@ export default class GitHub {

  async persistMedia(mediaFile, options = {}) {
    try {
-      await this.api.persistFiles(null, [mediaFile], options);
+      if (!options.draft) {


Skip the update and just return the expected object structure

erezrokah · 2019-11-07T12:21:14Z

packages/netlify-cms-backend-github/src/API.js

@@ -656,7 +630,7 @@ export default class API {
      }

      if (pr) {
-        return this.rebasePullRequest(pr.number, branchName, contentKey, metadata, commit);
+        return this.rebasePullRequest(pr.number, branchName, contentKey, updatedMetadata, commit);


Had to fix this to make sure metadata.object.files are synced

erezrokah · 2019-11-07T12:22:02Z

packages/netlify-cms-backend-github/src/API.js

@@ -681,7 +655,7 @@ export default class API {
       */
      const [baseBranch, commits] = await Promise.all([
        this.getBranch(),
-        this.getPullRequestCommits(prNumber, head),
+        this.getPullRequestCommits(prNumber),


getPullRequestCommits accepts only one argument

erezrokah · 2019-11-07T12:24:55Z

packages/netlify-cms-backend-github/src/implementation.js

@@ -343,6 +347,29 @@ export default class GitHub {
    return this.api.deleteFile(path, commitMessage, options);
  }

+  async getMediaFiles(data) {


Loads the draft entry media files

erezrokah · 2019-11-07T12:25:21Z

packages/netlify-cms-backend-github/src/implementation.js

                      resolve({
                        slug,
-                        file: { path },
+                        file: { path: data.metaData.objects.entry.path },


Just to be consistent with unpublishedEntry

erezrokah · 2019-11-07T12:27:49Z

packages/netlify-cms-core/src/actions/editorialWorkflow.js

+      const assetProxies = await Promise.all(
+        mediaFiles.map(({ file }) => createAssetProxy(file.name, file)),
+      );
+      dispatch(addAssets(assetProxies));


sync assets, draft media files and media library on entry load

Doing this all in one place is really good for visibility, love it

erezrokah · 2019-11-07T12:31:53Z

packages/netlify-cms-core/src/actions/entries.js

+      const assetProxies = await Promise.all(
+        assets.map(asset => createAssetProxy(asset.value, asset.fileObj)),
+      );
+      dispatch(addAssets(assetProxies));


sync assets - createAssetProxy creates an AssetProxy class (can't used the serialized object directly since we need the class methods).

erezrokah · 2019-11-07T12:33:32Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

+
+      const entry = state.entryDraft.get('entry');
+      const useWorkflow = state.config.getIn(['publish_mode']) === EDITORIAL_WORKFLOW;
+      const draft = entry && !entry.isEmpty() && useWorkflow;


I'm not sure this is the best way to check that we are editing an entry

I would expect the entry in state.entryDraft to always be an open entry, but maybe there are gaps - did you experience this not being the case?

I'm note sure what "open entry" means.
What I have experienced is that when you exit the editor the draft is discarded here: https://github.com/netlify/netlify-cms/blob/7554134f999970e8f8198f38200c47ff5880c0a4/packages/netlify-cms-core/src/reducers/entryDraft.js#L72

erezrokah · 2019-11-07T12:38:04Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

      if (!integration) {
-        const asset = await backend.persistMedia(state.config, assetProxy);
+        const asset = await backend.persistMedia(state.config, assetProxy, draft);


We still need the backend to generate the asset object so we call persistMedia

erezrokah · 2019-11-07T12:38:35Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

+      dispatch(removeDraftEntryMediaFile({ id: file.id }));
+
+      if (!file.draft) {
+        await backend.deleteMedia(state.config, file.path);


We don't need the result so we can skip the backend operation completely

erezrokah · 2019-11-07T12:48:13Z

packages/netlify-cms-core/src/reducers/mediaLibrary.js

-        const updatedFiles = map.get('files').filter(file => file.key !== key);
+        const updatedFiles = map
+          .get('files')
+          .filter(file => (key ? file.key !== key : file.id !== id));


support remove by id

erezrokah · 2019-11-07T15:45:37Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

+      payload: { mediaFiles },
+    };
+    // add media files to library only after the library finished loading
+    if (state.mediaLibrary.get('isLoading') === false) {


I can't believe I had to do this, but when loading a backup for a new unsaved entry directly from the new entry url (e.g. /collections/posts/new) the backup is loaded before the media library is mounted (hence loaded). Once the library did finish loading the entry media files were overwritten in the media library reducer.
Another solution was not to overwrite at all on media library load success, but it does makes sense to wait for the library to finish initial load before using it.

Yeah this is fine for now at least. It's a code smell, but we have stuff like isLoading and isFetching for a reason.

When you start syncing actions order inside componentDidUpdate based on props you know something is not right.

A better way to wait for actions is to use redux-saga https://redux-saga.js.org/docs/advanced/FutureActions.html as it uses a pull approach, but it might be an overkill for now.

barthc · 2019-11-07T15:47:47Z

packages/netlify-cms-core/src/reducers/mediaLibrary.js

+      let updatedFiles = differenceBy(state.get('files'), mediaFiles, 'path');
+      mediaFiles.forEach(file => {
+        const fileWithKey = { ...file, key: uuid() };
+        updatedFiles = [fileWithKey, ...updatedFiles];


Won't something like so:

[...mediaFiles.map(file => ({ ...file, key: uuid() })), ...updatedFiles]

be sufficient here ?

Good catch, I'll fix it.

barthc · 2019-11-07T15:50:07Z

packages/netlify-cms-core/src/actions/__tests__/editorialWorkflow.spec.js

+});
+jest.mock('ValueObjects/AssetProxy');
+jest.mock('netlify-cms-lib-util');
+jest.mock('ValueObjects/AssetProxy');


Yes, I'll remove it

erquhart · 2019-11-11T15:17:20Z

This does break expectations, so from a semver standpoint we should wait for 3.0 to make it default behavior. That said, the current default is just not great at all, and I doubt anyone will have an issue with this change. We could just introduce it as default and have a backup plan to move it under a config flag if folks cry foul.

_{Sent with GitHawk}

erquhart

Blown away by all of these tests, fantastic work Erez!! This is a big PR but I'd be comfortable merging and releasing it into the beta channel. Regarding default behavior, I'm inclined to just do it. If people are thrown off we can put it behind a flag, but I don't think that will happen.

erquhart · 2019-11-14T02:17:35Z

packages/netlify-cms-backend-github/src/API.js

@@ -1,6 +1,7 @@
 import { Base64 } from 'js-base64';
 import semaphore from 'semaphore';
-import { find, flow, get, hasIn, initial, last, partial, result, uniq } from 'lodash';
+import { find, flow, get, hasIn, initial, last, partial, result, differenceBy } from 'lodash';
+import trimStart from 'lodash/trimStart';


Should we include this in the line above and let treeshaking resolve the rest? Might need the lodash webpack plugin for that to actually happen.

You're right. I probably copy pasted it from somewhere and didn't notice the existing import. Going forward we can use https://github.com/wix/eslint-plugin-lodash/blob/master/docs/rules/import-scope.md to enforce a way to import lodash methods (probably Destructured Members) and use a Webpack/babel plugin to get tree shaking like you said.

Update: there is lodash-es, but we'll also need to check what happens if one of our dependencies imports lodash the "wrong way".

erquhart · 2019-11-14T02:23:36Z

packages/netlify-cms-backend-github/src/API.js

+        sha,
+      }));
+      return this.editorialWorkflowGit(files, entry, mediaFilesList, options);
+    }


Love all of this code removal 🔥🔥🔥

erquhart · 2019-11-14T02:32:02Z

packages/netlify-cms-backend-github/src/implementation.js

@@ -343,6 +347,29 @@ export default class GitHub {
    return this.api.deleteFile(path, commitMessage, options);
  }

+  async getMediaFiles(data) {


erquhart · 2019-11-14T02:38:09Z

packages/netlify-cms-core/src/actions/editorialWorkflow.js

+      const assetProxies = await Promise.all(
+        mediaFiles.map(({ file }) => createAssetProxy(file.name, file)),
+      );
+      dispatch(addAssets(assetProxies));


Doing this all in one place is really good for visibility, love it

erquhart · 2019-11-14T02:42:37Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

+
+      const entry = state.entryDraft.get('entry');
+      const useWorkflow = state.config.getIn(['publish_mode']) === EDITORIAL_WORKFLOW;
+      const draft = entry && !entry.isEmpty() && useWorkflow;


I would expect the entry in state.entryDraft to always be an open entry, but maybe there are gaps - did you experience this not being the case?

erquhart · 2019-11-14T02:44:20Z

packages/netlify-cms-core/src/actions/mediaLibrary.js

+      payload: { mediaFiles },
+    };
+    // add media files to library only after the library finished loading
+    if (state.mediaLibrary.get('isLoading') === false) {


Yeah this is fine for now at least. It's a code smell, but we have stuff like isLoading and isFetching for a reason.

* feat: commit media with post - initial commit * feat: add draft media indication * feat: sync UI media files with GitHub on entry load * feat: bug fixes * feat: delete media files from github when removed from library * test: add GitHub backend tests * test: add unit tests * fix: meta data object files are not updated * feat: used nested paths when update a tree instead of recursion * feat(test-backend): update test backend to persist media file with entry * test(e2e): re-record fixtures data * chore: code cleanup * chore: code cleanup * fix: wait for library to load before adding entry media files * chore: code cleanup * fix: don't add media files on entry when not a draft * fix: sync media library after draft entry was published * feat: update media library card draft style, add tests * test: add Editor unit tests * chore: test code cleanup * fix: publishing an entry from workflow tab throws an error * fix: duplicate media files when using test backend * refactor: fix lodash import * chore: update translations and yarn file after rebase * test(cypress): update recorded data * fix(test-backend): fix mapping of media files on publish

erezrokah commented Nov 7, 2019

View reviewed changes

barthc reviewed Nov 7, 2019

View reviewed changes

erezrokah force-pushed the feat/commit_media_with_post branch from 04f231e to 62dc650 Compare November 10, 2019 09:04

erezrokah changed the title ~~[WIP] feat: commit media with post~~ feat: commit media with post Nov 11, 2019

erezrokah force-pushed the feat/commit_media_with_post branch from 19a63dc to af2cecd Compare November 11, 2019 18:21

erezrokah requested a review from erquhart November 11, 2019 19:20

erquhart approved these changes Nov 14, 2019

View reviewed changes

erezrokah mentioned this pull request Nov 14, 2019

fix(backend-github): prepend collection name #2878

Merged

erezrokah added 8 commits November 17, 2019 09:21

feat: commit media with post - initial commit

c66dae2

feat: add draft media indication

959c64d

feat: sync UI media files with GitHub on entry load

b42501b

feat: bug fixes

f95ec8b

feat: delete media files from github when removed from library

62754fa

test: add GitHub backend tests

949ac67

test: add unit tests

626da26

fix: meta data object files are not updated

628fb9a

erezrokah added 18 commits November 17, 2019 09:24

feat: used nested paths when update a tree instead of recursion

47e1f9d

feat(test-backend): update test backend to persist media file with entry

028ecd6

test(e2e): re-record fixtures data

b0efa8a

chore: code cleanup

c95b6e6

chore: code cleanup

6c57192

fix: wait for library to load before adding entry media files

1a034b1

chore: code cleanup

d40f4a8

fix: don't add media files on entry when not a draft

30a0c46

fix: sync media library after draft entry was published

f7f9b57

feat: update media library card draft style, add tests

d16fb0c

test: add Editor unit tests

f30c2b7

chore: test code cleanup

44a973f

fix: publishing an entry from workflow tab throws an error

de33b29

fix: duplicate media files when using test backend

ff7c3c9

refactor: fix lodash import

0f4a975

chore: update translations and yarn file after rebase

0a49cd8

test(cypress): update recorded data

f85bc4a

fix(test-backend): fix mapping of media files on publish

4dfd87f

erezrokah force-pushed the feat/commit_media_with_post branch from 7554134 to 4dfd87f Compare November 17, 2019 08:56

erezrokah merged commit 6515dee into master Nov 17, 2019

erezrokah deleted the feat/commit_media_with_post branch November 26, 2019 13:34

erezrokah mentioned this pull request Dec 15, 2019

Feat: assets with content #2958

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: commit media with post #2851

feat: commit media with post #2851

erezrokah commented Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019

erquhart Nov 14, 2019

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019

erquhart Nov 14, 2019

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019

erquhart Nov 14, 2019

erezrokah Nov 14, 2019

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019

erezrokah Nov 7, 2019

erquhart Nov 14, 2019

erezrokah Nov 14, 2019

barthc Nov 7, 2019

erezrokah Nov 7, 2019

barthc Nov 7, 2019

erezrokah Nov 7, 2019

erquhart commented Nov 11, 2019

erquhart left a comment

erquhart Nov 14, 2019

erezrokah Nov 14, 2019 •

edited

Loading

erquhart Nov 14, 2019

erquhart Nov 14, 2019

erquhart Nov 14, 2019

erquhart Nov 14, 2019

erquhart Nov 14, 2019

feat: commit media with post #2851

feat: commit media with post #2851

Conversation

erezrokah commented Nov 7, 2019 • edited Loading

erezrokah Nov 7, 2019 • edited Loading

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

erezrokah Nov 7, 2019 • edited Loading

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

erezrokah Nov 7, 2019 • edited Loading

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 commented Nov 11, 2019

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezrokah Nov 14, 2019 • edited Loading

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

erezrokah commented Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 7, 2019 •

edited

Loading

erezrokah Nov 14, 2019 •

edited

Loading