Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1424: Set value for boolean field without default value #2525

Closed
wants to merge 4 commits into from
Closed

Fix #1424: Set value for boolean field without default value #2525

wants to merge 4 commits into from

Conversation

chrfritsch
Copy link
Contributor

Summary
This fixes a validation issue for boolean fields without any default value for new posts.

Test plan

  1. Define a field like this in config.yml:
-
  label: Awesome
  name: awesome
  widget: boolean
  1. Create a new item in the collection
  2. Try to publish the item

You'll get an "AWESOME IS REQUIRED" error.

For the record, specifying default: false for the field makes the problem disappear.

A picture of a cute animal (not mandatory but encouraged)
WhatsApp Image 2019-06-26 at 16 05 13

@barthc
Copy link
Contributor

barthc commented Aug 12, 2019

@chrfritsch could you add a simple test to confirm this behavior in the entries.spec file

@chrfritsch
Copy link
Contributor Author

Maybe something like this? It's my first jest test I ever wrote :D I don't understand why it returns an object with title: false. Maybe there is something else broken? But without the fix the object was empty.

{
name: 'post',
widget: 'boolean',
fields: [{ name: 'title', widget: 'boolean' }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean widgets don't have fields attrs, test for { name: 'boolean', widget: 'boolean' } for example, which should output { boolean: false }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched the parent back to object, like all the other tests. That was a leftover from some back and forth testing...

// A boolean field without a default value defined will initially render
// render as false, but the field will fail the required check. So we are
// assinging false.
const widgetDefaultValue = item.get('widget') == 'boolean' ? false : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really want to avoid hardcoding widget names. The underlying issue here is bigger than just the boolean widget.

@erquhart
Copy link
Contributor

Thanks for this @chrfritsch! I'm not sure it's the right approach, though. What we really need is for widgets to set their own default values. There's some discussion around this in #1407, but it isn't fully fleshed out yet.

Going to close this, but please comment if you have any thoughts on it or questions!

@erquhart erquhart closed this Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants