Add optional sort_on parameter to @vocabularies#1990
Conversation
|
@hasansyed107 thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
84fe109 to
2f89974
Compare
davisagli
left a comment
There was a problem hiding this comment.
I'm okay with this feature, but:
- It's really frustrating that I have to spend time telling you to re-add comments and tests that were removed for no good reason. If you continue doing this, I will not review your PRs.
- Sorting on token makes no sense, it's an internal identifier not visible to the user
- The new option needs to be included in the documentation
|
@davisagli I'd close this PR, and even consider a temporary ban from the Plone GitHub organization per https://6.docs.plone.org/contributing/first-time.html#requirements to give the author the time they need to review that and reconsider their actions. We're volunteers, and such persistent, careless, and disrespectful behavior shouldn't be tolerated. I've done exactly that in the Volto and Documentation repos a few times for similar behavior. |
277a958 to
5f306f4
Compare
|
Hi @davisagli, thank you for the feedback. You’re absolutely right — removing those comments and tests during iteration was a mistake on my part. I understand that review time is limited, and I apologize for the extra work this caused. I’ve now:
All tests pass locally and CI checks are running. Thanks again for the thorough review. |
|
@jenkins-plone-org please run jobs |
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
|
I’ve resolved the merge conflict and rebased the branch to align with the latest changes. The vocabulary sorting logic is now consistent with the serialized output, and the documentation includes the generated HTTP examples. All tests are passing locally — happy to address any feedback. |
stevepiercy
left a comment
There was a problem hiding this comment.
Docs look good, with one suggested change.
Co-authored-by: Steve Piercy <web@stevepiercy.com>
|
@jenkins-plone-org please run jobs |
stevepiercy
left a comment
There was a problem hiding this comment.
Docs look good. Needs a final technical review.
davisagli
left a comment
There was a problem hiding this comment.
Thanks for updating based on my earlier feedback. This looks quite good now, my new comments are just final cleanup.
Co-authored-by: David Glick <david@glicksoftware.com>
Co-authored-by: David Glick <david@glicksoftware.com>
|
@jenkins-plone-org please run jobs |
|
Hi @davisagli, thanks for the suggestions. I’ve applied the requested cleanup changes |
Co-authored-by: David Glick <david@glicksoftware.com>
Hey team! I've added a sort_on query parameter to the @vocabularies endpoint to allow for server-side sorting by either title or token.
Why this matters:
Previously, if you had a large vocabulary, you'd have to sort it on the frontend after batching, which led to inconsistent results. Now, we sort the terms before the batching happens.
How I built it:
I made sure the sorting logic exactly matches how terms are serialized. If a term doesn't have a title, it falls back to the token so the alphabetical list stays consistent.
Handled i18n translations and case-insensitivity so "Title" and "title" (or "Tötle") play nice together.
Added functional tests to cover these new cases without touching the existing 1,300+ tests.
Closes #1989. Thanks for taking a look!
📚 Documentation preview 📚: https://plonerestapi--1990.org.readthedocs.build/