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] Make required respect dependsOn on Relationship fields too #4685

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicolas-guichard
Copy link

Description of changes

Fixes a bug: work done by keystonejs/keystone#2200 did not apply to Relationship fields.

Testing

  • Please confirm you've added test coverage for this change.
  • Please confirm npm run test-all ran successfully.

Nico264 added 2 commits June 26, 2018 18:25
Corrects a bug regarding the dependsOn/require options interaction in Relationship Fields (follows the enhancement of keystonejs/keystone#2200)
Copy link
Contributor

@stennie stennie left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in contributing to Keystone! Are there code changes missing from this PR? Unless I'm missing some context, this does not appear to include any substantial fixes that match the current summary.

Can you add more details on what problem this solves?

Thanks,
Stennie

index: (this.options.index ? true : false),
required: (this.options.required ? true : false),
unique: (this.options.unique ? true : false),
index: (this.options.index ? this.options.index : false),
Copy link
Contributor

Choose a reason for hiding this comment

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

The values for these 3 options (index, required, and unique) are either true or false and the conditional (ternary) should already be setting these correctly.

Can you clarify the motivation for changing these?

Copy link
Author

@nicolas-guichard nicolas-guichard Jun 27, 2018

Choose a reason for hiding this comment

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

The desired values are not necessarily true or false, in the case described by keystonejs/keystone#2200, if dependsOn is set, this.option.required may actually be a function (see below), which is then used by Mongoose. I changed the three of them for consistency.

https://github.com/keystonejs/keystone/blob/d34f45662eb359e2cb18b397f2ffea21f9883141/fields/types/Type.js#L74

DependsOn.model.find({}).remove(function (error) {
if (error) {
done(error);
}

var newPost = new DependsOn.model({
var newDependsOn = new DependsOn.model({
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be an arbitrary rename without changing any functionality for relationships.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, I should have done a separate commit for the renaming, which is there to clarify that this test does not use the Post model. The new test case is lines 58-84: it fails without commit 4e27b7b and passes with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants