-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add simple subfield support #297
base: master
Are you sure you want to change the base?
Conversation
this looks interesting. Would you include some tests that demonstrate how simple subfields would be represented as JSON in the API and how simple subfields support server-side validation? |
sure, I will give that a go. thanks. |
…nd after_search as it breaks down stream harvesters
I removed the conversion of simple subfields from lists into dictionaries in the after-show and after-search hooks as this broke downstream harvesters which would assume the field was a list if using the same customer schema as the parent catalogue. Instead, the conversion is handled in the jinja templates. This, once again, appears ready for review. |
Simple subfields in the API look a lot like repeating subfields. Is there anything that prevents users from inserting more than one record into a simple subfield and returns a clear error message? |
good point @wardi there is not currently a validator for this. I am remembering now that I looked into this before but did not make much headway with it. After some more digging I think I have found at least two ways this could work. one is attaching a validator to the field which results in output like:
That doesn't seem quite right to me. The other option I explored was attaching a 'before' validator to look for any field with simple_subfields. This resulted in the following output:
I would like to attach the error directly to the parent field but don't seem to be able to. it causes odd string errors downstream in the validation process. any advice on the best way to approach this validation problem? |
Much earlier I removed another simple subfields-like feature because I was thinking it would be cleaner to implement this kind of feature on the front-end only by visually grouping field elements together in the form but keeping them technically as normal form fields (siblings of the top-level fields), possibly with a common field name prefix. Validation is simpler and API use is simpler. Your feature here though has the ability to have 0 or 1 of the sub-forms right? That combined with required fields can't be implemented by only visual grouping on the form. I'm not sure the best approach for validation of the number of fields provided or how best to combine the error with others returned. Maybe return a pseudo-field for the "wrong number of sub-forms" error like "error": {
"temporal-extent-count": [
"Too many items in simple subfield. Found 2 items, expected 1"
] But now it looks like this simple subfields implementation could be treated as a special case of repeating subfields by adding a minumum and maximum number of sub-forms option to the repeating subfields definition. That approach could reduce the code changes required and be a more generic feature. |
interesting point @wardi re the repeating subfield with max entries of 1. It would look very much like a simple subfield. Based on my exploration of validators I think the validation bit would be the same. The form templates would also still need adjustment. I can see however how a limited list of repeating fields could have other use cases. I think I will finish up the before hook for validation with this branch using a 'pseudo-field' as you suggested and then explore what a min/max option for repeating subfields would look like. |
I would love to this is implemented. Is there any hope to have that in master soon? |
That would be very helpful for a schema I am using. |
Honestly, it has been so long since I looked at it I have forgotten. I should have some time next week to dig into this again. I seem to remember there being a second step that I had not completed. |
Thank you! No rush 😄 |
fix #296
depends on #295
This pull request adds support for simple subfields. code changes allow fields to be saved, retrieved, and displayed in ckan.