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

barthc · 2019-11-13T21:12:19Z

Summary
Closes #2620, closes #1933, closes #1396

erquhart · 2019-11-14T03:10:22Z

Thanks @barthc! Since Erez has a PR open that touches the workflow a lot I'm going to defer to him for review. I do remember a PR like this getting merged in the past (from someone else, not you), and I believe it caused some issues and had to be reverted. I'll see if I can find it.

erezrokah · 2019-11-14T10:38:36Z

I like the new approach, but instead of baking in backwards compatibility into several different API methods, how about doing one single migrate operation?

A possible place to do it is in listUnpublishedBranches - we already get all the cms/ branches, we can get their metadata (or even use labels to mark the ones that have been migrated like suggested here: #1669).

Then we can rename the existing branches and update any relevant metadata.
The plus side it will fix those issues for existing branches as well.

Update: I think it will also be a bit easier to test - we just need to make sure the migration process works instead of testing different API methods, each one for the old/new way of creating branches.

Another update: might be a good opportunity to introduce some versioning scheme for metadata to make it easier to migrate in the future.

barthc · 2019-11-14T15:26:30Z

I was thinking with this approach(backwards compatible), we can drop searching for the old key sometime in the future.

Regarding migration, I believe we discussed something like so in another PR. One of the issues is that we can't delete or rename(maybe not sure) any metadata file inside the orphaned branch. So the best we can do is duplicate the metadata file with the new key name.

I also don't think we can rename branch via the api.

erezrokah · 2019-11-14T15:59:46Z

Good points @barthc, I'll have time to take a closer look once I'm done with #2851

erquhart · 2019-11-17T15:24:13Z

Creating a new branch and deleting the old should do the trick for renaming.

_{Sent with GitHawk}

erezrokah · 2019-11-18T11:08:31Z

How about something like this: b37faa0

I cleaned up some of the code for getting unpublished branches, but the relevant migration code is here: b37faa0#diff-b4ba42c688a23ed9cec5139daec2c157R452

It needs some more testing (manual and automatic) and some code cleanup though.

barthc · 2019-11-19T12:34:40Z

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

-    const onlyBranchesWithOpenPRs = filterPromisesWith(({ ref }) =>
-      this.branchHasPR({ branchName: this.branchNameFromRef(ref), state: 'open' }),
-    );
+    const onlyBranchesWithOpenPRs = flowAsync([


Used a new flowAsync utility function, which is cleaner.

Please see here:
b37faa0#diff-b4ba42c688a23ed9cec5139daec2c157R518

Instead of sending a pulls API request per branch and since that API uses substring match, we can send a single request with cms as the matching pattern and filter the branches according to the result.

erezrokah · 2019-11-19T13:09:59Z

Just noticed you ported the migration code. Nice one :)
I do have a bug in my branch where I don't set the metadata current version when persisting metadata.
I'll try to go over your recent changes soon

erezrokah · 2019-11-19T13:13:06Z

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

@@ -225,6 +224,29 @@ export default class API {
    );
  }

+  deleteMetadata(key) {


erezrokah · 2019-11-19T13:14:56Z

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

@@ -387,8 +387,8 @@ export default class GitHub {
        const promises = [];
        branches.map(({ ref }) => {
          promises.push(
-            new Promise(resolve => {
-              const contentKey = ref.split('refs/heads/cms/').pop();
+            new Promise(async resolve => {


Is the async required?

Sorry I forgot to remove that.

barthc · 2019-11-19T13:16:04Z

I do have a bug in my branch where I don't set the metadata current version when persisting metadata.

Yes I added that in.

erezrokah · 2019-11-20T10:51:09Z

Hey @barthc. I added some unit tests, re-recorded the e2e tests data and made some code changes, the most notable one is to hard code the migration code.
We still want the migration code to work, even if the implementation of the key/branch generation changes in the future.

I'm good with merging if you are.

barthc · 2019-11-20T11:08:31Z

@erezrokah thanks for the changes. I think we forgot to handle one edge case. I will push up shortly.

erezrokah · 2019-11-20T11:30:19Z

Cool, I'm pushing a fix for the GraphQL createBranch too (it should return an object with a ref property).

barthc · 2019-11-20T12:45:23Z

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

+    const entriesLoaded = get(state.editorialWorkflow.toJS(), 'pages.ids', false);
+    //run possible unpublishedEntries migration
+    if (!entriesLoaded) {
+      const response = await backend.unpublishedEntries(state.collections).catch(() => false);


For published entry that have a draft entry, let's run the migration code first before loading the draft.

This can happen when opening an entry in edit mode directly without going through the workflow tab first? Am I correct?

Yes, that's correct.

barthc · 2019-11-20T12:46:16Z

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

    const backend = currentBackend(state.config);
+    const entriesLoaded = get(state.editorialWorkflow.toJS(), 'pages.ids', false);


The unpublished entries(workflow page) should now load only once.

barthc · 2019-11-20T13:17:17Z

@erezrokah did you find the PR @erquhart referenced above?

erezrokah · 2019-11-20T13:24:02Z

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

+        head: pr.head.sha,
+      },
+      branch: newBranchName,
+      version: CURRENT_METADATA_VERSION,


This should be hard coded to '1' since we don't want it to change when someone updates CURRENT_METADATA_VERSION (see comment at the beginning of the method).

e.g. let's say we have at some point CURRENT_METADATA_VERSION = 2, we would have two scenarios:

Migrating from no version to version 1, then to version 2.

Migrating from version 1 to version 2.

The migrateBranch method will run migrations serially based on the current version and metadata version.

ok, I will remove it.

erezrokah · 2019-11-20T16:16:46Z

Again, great job @barthc! Going to let @erquhart have a look at this before merging just to be sure.

erezrokah · 2019-11-25T15:54:32Z

@erezrokah did you find the PR @erquhart referenced above?

Just saw this comment. Didn't find the PR, but we've discussed it and we're getting confident about merging this. Just letting @erquhart express his opinion the migration code first then it will be good to go.

erezrokah · 2019-11-26T08:40:02Z

Going to push this through as we have some more changes coming to the GitHub API

* fix(backend-github): prepend collection name * chore: prefer migrating entries * chore: cleanup * chore: move migration to listUnpublishedBranches * chore: prefer flowAsync * chore: feedback updates * refactor: extract current metadata version to a const * refactor: don't send pulls request on open authoring * test: update recorded data * fix: hardcode migration key/branch logic * test(backend-github): add unit tests for migration code * fix(github-graphql): add ref property to result of createBranch * test(cypress): update recorded data * fix: load unpublished entries once * fix: run migration for published draft entry * fix: failing test * chore: use hardcoded version number * fix: use hardcoded version number * test(cypress): update recorded data

erquhart requested a review from erezrokah November 14, 2019 03:10

fix(backend-github): prepend collection name

b1e8a58

barthc added 2 commits November 18, 2019 11:38

chore: prefer migrating entries

0c04316

chore: cleanup

232de41

barthc force-pushed the fix/github-backend-prepend-collection-name-to-branch-name branch from e07f610 to 232de41 Compare November 18, 2019 10:50

barthc added 2 commits November 18, 2019 21:02

chore: move migration to listUnpublishedBranches

93b9b75

chore: prefer flowAsync

73e7ef5

barthc commented Nov 19, 2019

View reviewed changes

erezrokah reviewed Nov 19, 2019

View reviewed changes

barthc and others added 6 commits November 19, 2019 15:19

chore: feedback updates

19a650f

refactor: extract current metadata version to a const

7f32f8e

refactor: don't send pulls request on open authoring

6bc1071

test: update recorded data

32c5e99

fix: hardcode migration key/branch logic

8bbd5d6

test(backend-github): add unit tests for migration code

c448609

erezrokah and others added 3 commits November 20, 2019 13:55

fix(github-graphql): add ref property to result of createBranch

24cd1d5

test(cypress): update recorded data

6b534e4

fix: load unpublished entries once

eca36df

fix: run migration for published draft entry

7ae7e52

barthc commented Nov 20, 2019

View reviewed changes

fix: failing test

2a9a3bd

erezrokah reviewed Nov 20, 2019

View reviewed changes

barthc and others added 3 commits November 20, 2019 14:30

chore: use hardcoded version number

24ccfd9

fix: use hardcoded version number

3910cea

test(cypress): update recorded data

f7ce7b7

erezrokah approved these changes Nov 26, 2019

View reviewed changes

erezrokah merged commit 465f463 into master Nov 26, 2019

erezrokah deleted the fix/github-backend-prepend-collection-name-to-branch-name branch November 28, 2019 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

barthc commented Nov 13, 2019

erquhart commented Nov 14, 2019 •

edited

Loading

erezrokah commented Nov 14, 2019 •

edited

Loading

barthc commented Nov 14, 2019

erezrokah commented Nov 14, 2019

erquhart commented Nov 17, 2019 •

edited

Loading

erezrokah commented Nov 18, 2019

barthc Nov 19, 2019

erezrokah Nov 19, 2019 •

edited

Loading

erezrokah commented Nov 19, 2019

erezrokah Nov 19, 2019

erezrokah Nov 19, 2019

barthc Nov 19, 2019

barthc commented Nov 19, 2019

erezrokah commented Nov 20, 2019

barthc commented Nov 20, 2019

erezrokah commented Nov 20, 2019 •

edited

Loading

barthc Nov 20, 2019

erezrokah Nov 20, 2019

barthc Nov 20, 2019

barthc Nov 20, 2019

barthc commented Nov 20, 2019

erezrokah Nov 20, 2019 •

edited

Loading

barthc Nov 20, 2019

erezrokah commented Nov 20, 2019

erezrokah commented Nov 25, 2019

erezrokah commented Nov 26, 2019

		const backend = currentBackend(state.config);
		const entriesLoaded = get(state.editorialWorkflow.toJS(), 'pages.ids', false);

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

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

Conversation

barthc commented Nov 13, 2019

erquhart commented Nov 14, 2019 • edited Loading

erezrokah commented Nov 14, 2019 • edited Loading

barthc commented Nov 14, 2019

erezrokah commented Nov 14, 2019

erquhart commented Nov 17, 2019 • edited Loading

erezrokah commented Nov 18, 2019

Choose a reason for hiding this comment

erezrokah Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

erezrokah commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthc commented Nov 19, 2019

erezrokah commented Nov 20, 2019

barthc commented Nov 20, 2019

erezrokah commented Nov 20, 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

barthc commented Nov 20, 2019

erezrokah Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezrokah commented Nov 20, 2019

erezrokah commented Nov 25, 2019

erezrokah commented Nov 26, 2019

erquhart commented Nov 14, 2019 •

edited

Loading

erezrokah commented Nov 14, 2019 •

edited

Loading

erquhart commented Nov 17, 2019 •

edited

Loading

erezrokah Nov 19, 2019 •

edited

Loading

erezrokah commented Nov 20, 2019 •

edited

Loading

erezrokah Nov 20, 2019 •

edited

Loading