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

Add simple subfield support #297

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

Conversation

fostermh
Copy link

@fostermh fostermh commented Aug 26, 2021

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.

@wardi
Copy link
Contributor

wardi commented Aug 26, 2021

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?

@fostermh
Copy link
Author

sure, I will give that a go. thanks.

@fostermh fostermh mentioned this pull request May 9, 2022
@fostermh
Copy link
Author

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.

@wardi
Copy link
Contributor

wardi commented May 10, 2022

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?

@fostermh
Copy link
Author

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:

{
    "help": "http://localdev1.local/api/3/action/help_show?name=package_create",
    "error": {
        "temporal-extent": [
            {
                "temporal-extent": [
                    "unexpected list length 2. List length of simple subfields should be 1."
                ]
            },
            {
                "temporal-extent": [
                    "unexpected list length 2. List length of simple subfields should be 1."
                ]
            }
        ],
        "__type": "Validation Error"
    },
    "success": false
}

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:

{
    "help": "http://localdev1.local/api/3/action/help_show?name=package_create",
    "error": {
        "temporal-extent": [
            {},
            {
                "": [
                    "Too many items in simple subfield. Found 2 items, expected 1"
                ]
            }
        ],
        "__type": "Validation Error"
    },
    "success": false
}

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?

@wardi
Copy link
Contributor

wardi commented May 12, 2022

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.

@fostermh
Copy link
Author

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.

@maxclac
Copy link

maxclac commented Jan 4, 2023

I would love to this is implemented. Is there any hope to have that in master soon?

@spwoodcock
Copy link

That would be very helpful for a schema I am using.
Is the PR ready @fostermh?

@fostermh
Copy link
Author

fostermh commented Aug 4, 2023

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.

@spwoodcock
Copy link

Thank you! No rush 😄

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.

simple subfields
4 participants