Check for title/slug field on config load. #1203

tech4him1 · 2018-03-27T19:45:58Z

This PR fixes #690 by verifying that folder-type collections have a valid title ("slug"-able) field at CMS load time, instead of when the user tries to save an entry. It also explains how to fix the error.

verythorough · 2018-03-27T19:48:13Z

Deploy preview for netlify-cms-www ready!

Built with commit 4044766

https://deploy-preview-1203--netlify-cms-www.netlify.com

verythorough · 2018-03-27T19:48:59Z

Deploy preview for cms-demo ready!

Built with commit 4044766

https://deploy-preview-1203--cms-demo.netlify.com

erquhart · 2018-03-28T14:17:14Z

Sweet! Is the WIP label still accurate?

tech4him1 · 2018-03-28T14:19:48Z

@erquhart I haven't tested it much yet. Also, is this, in your opinion, too big of a change to the backend, or is it good to merge?

erquhart · 2018-03-28T18:00:11Z

It's mostly a refactor, just porting the identifier logic out to a selector, which makes sense, and improving the error message. I haven't given it a thorough review, but it seems merge ready to me.

tech4him1 · 2018-03-28T19:23:44Z

This is going to need some more work before merge.

tech4him1 · 2018-03-28T19:36:47Z

@erquhart I think I've fixed all the issues, but it probably deserves a pretty thorough review before merge -- the logic gets changed up quite a bit, it's not just a copy/paste.

tech4him1 · 2018-04-13T19:33:42Z

Ready for review reminder 😆

erquhart · 2018-04-13T21:51:53Z

Alright I've got it queued.

erquhart

The logic looks solid, just a few notes to simplify the implementation.

erquhart · 2018-04-19T19:36:31Z

src/reducers/collections.js

 }

+const validIdentifierFields = ["title", "path"];
+export const selectIdentifier = (entryData) => {


fieldNames would be more self documenting than entryData, which I would expect to be a map of the actual entry data.

erquhart · 2018-04-19T19:38:12Z

src/reducers/collections.js

@@ -34,7 +34,8 @@ function validateCollection(configCollection) {
    files,


This validateCollection function is just validating configuration - I'd say move it to Actions/config with the rest of the config validation (all of which may be worth porting elsewhere in the future), which allows you to put the new selector in with the rest of the selectors under the [FOLDER] key.

erquhart · 2018-05-15T21:23:36Z

src/actions/config.js

@@ -40,6 +43,38 @@ export function applyDefaults(config) {
    });
 }

+function validateCollection(collection) {


Not a new function, relocated from the collections reducer (because it validates configuration), with the exception of the last conditional.

erquhart · 2018-05-15T21:24:00Z

src/actions/config.js

+  if (!!folder && !selectIdentifier(collection)) {
+    // Verify that folder-type collections have an identifier field for slug creation.
+    throw new Error(`Collection "${name}" must have a field that is a valid entry identifier. Supported fields are ${IDENTIFIER_FIELDS.join(', ')}.`);
+  }


This last conditional was added for this PR.

erquhart · 2018-05-15T21:25:51Z

@tech4him1 mind reviewing with this latest commit? I think it's good to merge if it looks good to you.

tech4him1 · 2018-05-17T16:38:52Z

@erquhart We'll throw every time here, instead of just when the {{slug}} placeholder is used. For example, we had people using only the date to save the file, without a title at all. Even though it is intended to always check, like this PR is doing, do you think it is a breaking change and we should merge into 2.0 instead?

erquhart · 2018-05-17T17:10:55Z

Yeah that makes sense - let's not backport this one if we can get away with it.

Benaiah

LGTM - I have a couple nitpicks, but nothing important. Tested the functionality manually and it works as designed.

Benaiah · 2018-05-17T21:19:05Z

src/actions/config.js

+    // Cannot set custom delimiter without explicit and proper frontmatter format declaration
+    throw new Error(`Please set a proper frontmatter format for collection "${name}" to use a custom delimiter. Supported frontmatter formats are yaml-frontmatter, toml-frontmatter, and json-frontmatter.`);
+  }
+  if (!!folder && !selectIdentifier(collection)) {


I don't think the !! bool coercion here is necessary. Any value which would turn into false due to the coercion would also be ignored due to the && which follows immediately after.

Benaiah · 2018-05-17T21:27:48Z

src/backends/backend.js

-    return identifier;
-  };
+  const identifier = entryData.get(selectIdentifier(collection));
+  if (identifier === undefined) {


if (identifier === undefined) seems overly specific when if (identifier) seems to express the same semantics (none of false, null, and 0 are valid identifier field names either). If we'd rather avoid falsiness semantics, a better solution would be to use the second arguments to Immutable's .get and .find to explicitly return false when nothing is found.

This was just copying existing code, but if we're going for 2.0 this should be fine.

tech4him1 changed the title ~~Clarify "Invalid entry identifier" message~~ WIP: Clarify "Invalid entry identifier" message Mar 27, 2018

tech4him1 force-pushed the clarify-title-necessary branch 2 times, most recently from d0c907d to f054994 Compare March 27, 2018 20:49

tech4him1 changed the title ~~WIP: Clarify "Invalid entry identifier" message~~ Clarify "Invalid entry identifier" message Mar 28, 2018

tech4him1 force-pushed the clarify-title-necessary branch 2 times, most recently from fec276b to 09f4d73 Compare March 28, 2018 18:31

tech4him1 changed the title ~~Clarify "Invalid entry identifier" message~~ WIP: Clarify "Invalid entry identifier" message Mar 28, 2018

tech4him1 changed the title ~~WIP: Clarify "Invalid entry identifier" message~~ Clarify "Invalid entry identifier" message Mar 28, 2018

tech4him1 requested review from erquhart, Benaiah and talves March 28, 2018 19:36

tech4him1 changed the title ~~Clarify "Invalid entry identifier" message~~ Check for title/slug field on config load. Mar 29, 2018

tech4him1 added the status: 2-code-review label Apr 1, 2018

tech4him1 removed the status: 2-code-review label Apr 18, 2018

erquhart suggested changes Apr 19, 2018

View reviewed changes

tech4him1 added 4 commits May 15, 2018 14:46

Update backend.js

7608ed1

Verify slug field exists on config load.

6ea068f

Only require title field for folder-type collections.

ce48fdd

Fix broken slugs.

4138a23

erquhart force-pushed the clarify-title-necessary branch from 1eab638 to 1d08984 Compare May 15, 2018 21:21

split config and value validation, move selector to reducer

bbae4b5

erquhart force-pushed the clarify-title-necessary branch from 1d08984 to bbae4b5 Compare May 15, 2018 21:22

erquhart reviewed May 15, 2018

View reviewed changes

erquhart approved these changes May 15, 2018

View reviewed changes

fix test

78f0f50

tech4him1 added the release: 2.0 label May 17, 2018

Benaiah approved these changes May 17, 2018

View reviewed changes

improve conditionals

4044766

erquhart merged commit 0022df5 into master May 25, 2018

erquhart deleted the clarify-title-necessary branch May 25, 2018 16:15

brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 29, 2018

Check for title/slug field on config load. (decaporg#1203)

7ea455d

brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 29, 2018

Check for title/slug field on config load. (decaporg#1203)

a3dc56a

honeyspoon mentioned this pull request Oct 9, 2018

Remove requirement for 'title' field in folder collections #1802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for title/slug field on config load. #1203

Check for title/slug field on config load. #1203

tech4him1 commented Mar 27, 2018 •

edited

Loading

verythorough commented Mar 27, 2018 •

edited

Loading

verythorough commented Mar 27, 2018 •

edited

Loading

erquhart commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

erquhart commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

tech4him1 commented Apr 13, 2018

erquhart commented Apr 13, 2018

erquhart left a comment •

edited

Loading

erquhart Apr 19, 2018

erquhart Apr 19, 2018

erquhart May 15, 2018 •

edited

Loading

erquhart May 15, 2018

erquhart commented May 15, 2018 •

edited

Loading

tech4him1 commented May 17, 2018

erquhart commented May 17, 2018

Benaiah left a comment

Benaiah May 17, 2018

Benaiah May 17, 2018

tech4him1 May 17, 2018

		@@ -34,7 +34,8 @@ function validateCollection(configCollection) {
		files,

Check for title/slug field on config load. #1203

Check for title/slug field on config load. #1203

Conversation

tech4him1 commented Mar 27, 2018 • edited Loading

verythorough commented Mar 27, 2018 • edited Loading

verythorough commented Mar 27, 2018 • edited Loading

erquhart commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

erquhart commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

tech4him1 commented Mar 28, 2018

tech4him1 commented Apr 13, 2018

erquhart commented Apr 13, 2018

erquhart left a comment • edited Loading

Choose a reason for hiding this comment

erquhart Apr 19, 2018

Choose a reason for hiding this comment

erquhart Apr 19, 2018

Choose a reason for hiding this comment

erquhart May 15, 2018 • edited Loading

Choose a reason for hiding this comment

erquhart May 15, 2018

Choose a reason for hiding this comment

erquhart commented May 15, 2018 • edited Loading

tech4him1 commented May 17, 2018

erquhart commented May 17, 2018

Benaiah left a comment

Choose a reason for hiding this comment

Benaiah May 17, 2018

Choose a reason for hiding this comment

Benaiah May 17, 2018

Choose a reason for hiding this comment

tech4him1 May 17, 2018

Choose a reason for hiding this comment

tech4him1 commented Mar 27, 2018 •

edited

Loading

verythorough commented Mar 27, 2018 •

edited

Loading

verythorough commented Mar 27, 2018 •

edited

Loading

erquhart left a comment •

edited

Loading

erquhart May 15, 2018 •

edited

Loading

erquhart commented May 15, 2018 •

edited

Loading