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

Validate provided displayFields against the json schema. #123

Closed
wants to merge 2 commits into from

Conversation

n1k0
Copy link
Contributor

@n1k0 n1k0 commented Jun 2, 2016

This adds validation to the Collection creation/edition form to ensure that entered displayFields actually match the provided json schema.

r=? @Natim @leplatrem

@n1k0
Copy link
Contributor Author

n1k0 commented Jun 2, 2016

Deployed to gh-pages.

}
return Object.keys(schema.properties).reduce((paths, key) => {
const path = (prefix ? prefix + "." : prefix) + key;
return paths.concat(path, extractSchemaPaths(schema.properties[key], path));
Copy link
Member

Choose a reason for hiding this comment

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

Genius

@Natim
Copy link
Member

Natim commented Jun 2, 2016

The code LGTM, I will have a look at the usability on gh-pages.

@Natim
Copy link
Member

Natim commented Jun 2, 2016

Ah you need to allow to display id and last_modified fields too.

They are present even if they are not in the schema.

@Natim
Copy link
Member

Natim commented Jun 2, 2016

I am wondering how you plan to handle cases where you have a displayField that are not in the schema but can be in the record?

@n1k0
Copy link
Contributor Author

n1k0 commented Jun 2, 2016

Ah you need to allow to display id and last_modified fields too.

last_modified is displayed by default in a dedicated column in the list. Remains id which should be addressed by #66 (we actually always want the id rendered in the list, but we need a convenient way to do that, as uuids are rather long strings).

I am wondering how you plan to handle cases where you have a displayField that are not in the schema but can be in the record?

Hmm, that's a strange use case but that may happen indeed. I can't infer these, so we should probably discard this patch as I can't think of any technical solution to address this.

@n1k0
Copy link
Contributor Author

n1k0 commented Jun 2, 2016

Now I think a bit more about it, we could split the current displayField field into two different ones:

  • schemaFields would select properties part of the schema;
  • recordFields would allow defining other extraneous properties.

The schemaFields field could be a list of checkboxes listing all the available field paths (see the patch), and users would simply cherry-pick the ones they want to render in the table view.

Bunch of work, but may be worth it. Thoughts?

@Natim
Copy link
Member

Natim commented Jun 2, 2016

Or you can add a warning (this field is not in the schema, are you sure you didn't made a typo?)

@n1k0
Copy link
Contributor Author

n1k0 commented Jun 2, 2016

Yeah that may work too, and it's much simpler.

@Natim
Copy link
Member

Natim commented Aug 29, 2016

What is the status of this? Should we close it?

@n1k0
Copy link
Contributor Author

n1k0 commented Aug 29, 2016

Yes, probably.

@n1k0
Copy link
Contributor Author

n1k0 commented Mar 17, 2017

#412 is probably a good hint that we should work on this again.

@n1k0 n1k0 reopened this Mar 17, 2017
return paths;
}
return Object.keys(schema.properties).reduce((paths, key) => {
const path = (prefix ? prefix + "." : prefix) + key;

Choose a reason for hiding this comment

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

Suggested change
const path = (prefix ? prefix + "." : prefix) + key;
const path = `${prefix}${prefix ? "." : ""}${key}`;

@leplatrem
Copy link
Contributor

Pull Request TTL timeout.

@leplatrem leplatrem closed this Oct 7, 2020
@grahamalama grahamalama deleted the validate-displayFields branch February 1, 2024 19:25
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.

4 participants