[FIX] Make required respect dependsOn on Relationship fields too #4685
Conversation
Corrects a bug regarding the dependsOn/require options interaction in Relationship Fields (follows the enhancement of keystonejs/keystone#2200)
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| DependsOn.model.find({}).remove(function (error) { | ||
| if (error) { | ||
| done(error); | ||
| } | ||
|
|
||
| var newPost = new DependsOn.model({ | ||
| var newDependsOn = new DependsOn.model({ |
There was a problem hiding this comment.
This seems to be an arbitrary rename without changing any functionality for relationships.
There was a problem hiding this comment.
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.
Description of changes
Fixes a bug: work done by keystonejs/keystone#2200 did not apply to Relationship fields.
Testing
npm run test-allran successfully.