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

[WIP] Filtering and batching for @vocabularies #550

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@davisagli
Member

davisagli commented Jun 23, 2018

This implements the first and second requirements for #535

@lukasgraf

Nice! 🎉 Looks good to me, apart from the open question regarding terms vs items 👍

u'last': u'http://localhost:55001/plone/@vocabularies/plone.restapi.tests.test_vocabulary?b_start=1&b_size=1', # noqa
u'next': u'http://localhost:55001/plone/@vocabularies/plone.restapi.tests.test_vocabulary?b_start=1&b_size=1', # noqa
},
u'terms': [

This comment has been minimized.

@lukasgraf

lukasgraf Jun 23, 2018

Member

Keeping the name terms instead of items deviates from what other batched resultsets look like. When initially implementing batching (for search results, folder listings and collections) we took care have the response data follow a common shape, as described in the batching docs.

This should allow API consumers (frontend, but server-to-server clients as well) to implement batching support once in their client, and have it work for all batched resources in plone.restapi.

I'm not sure whether that goal is worth enforcing a generic property name like items for resources where a more descriptive name (like terms) would be available. We could possibly introduce a property like 'batched_property': 'terms' in the batching header to point to which property contains the batch results, and still retain the possibility to use custom names.

Or, since it's a breaking change to the @vocabularies endpoint anyway, we just bite the bullet and rename terms to items (and terms_total to items_total) to conform with the "batching interface".

@tisto any thoughts?

This comment has been minimized.

@davisagli

davisagli Jun 23, 2018

Member

@lukasgraf I can rename it to items. I was thinking that it should be named something different since the entries in the list are not content items, but your argument about being able to reuse client-side batching support convinced me.

This comment has been minimized.

@lukasgraf

lukasgraf Jun 23, 2018

Member

@davisagli there's some history behind items: Back when Hydra was still on the table, it started as member, but __parent__ and __children__ properties were also being discussed :trollface: So, as a general term for "sequence of things" we could do much worse than items 😉

This comment has been minimized.

@davisagli

davisagli Jun 23, 2018

Member

@lukasgraf it also matches the terminology of JSON Schema

@davisagli

This comment has been minimized.

Member

davisagli commented Jun 23, 2018

@lukasgraf when you get a chance, could you review my latest commit here? I'll probably wait to merge until I've tried making the corresponding frontend changes, in case that reveals anything that should be tweaked.

@lukasgraf

This comment has been minimized.

Member

lukasgraf commented Jun 23, 2018

All looks good to me, I say 🚢 as soon as you feel like it 👍

As discussed, the vocabulary: <endpoint_url> reference probably doesn't have any well defined meaning in JSON-Schema anymore, but I don't see this as an issue.

@coveralls

This comment has been minimized.

coveralls commented Jun 23, 2018

Coverage Status

Coverage decreased (-0.3%) to 95.819% when pulling fe44013 on filtered-batched-vocabs into 1b88d93 on master.

@tisto tisto changed the title from Filtering and batching for @vocabularies to [WIP] Filtering and batching for @vocabularies Jun 24, 2018

@tisto tisto added this to the 3.x.x milestone Jun 24, 2018

@sneridagh

LGTM, +1 to ship it too!

@sneridagh

This comment has been minimized.

Member

sneridagh commented Jul 6, 2018

@davisagli Did you do any progress on the frontend side? I can't find the PR or the branch.

@davisagli

This comment has been minimized.

Member

davisagli commented Jul 6, 2018

@sneridagh I made some progress but have some questions for Rob. I'll try to clean things up and ask those questions this weekend.

@davisagli

This comment has been minimized.

Member

davisagli commented Jul 9, 2018

Here is as far as I got on the plone-react side: plone/volto#214

@tisto

This comment has been minimized.

Member

tisto commented Jul 21, 2018

@davisagli do you think this PR is mergeable now?

@davisagli

This comment has been minimized.

Member

davisagli commented Jul 21, 2018

@tisto not unless someone else worked on the frontend changes. If this is merged without updating the widgets, they will only load the first batch of vocabulary terms.

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