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

Update docs with recent changes to membership APIs #9210

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Dec 12, 2024

Fixes #9134.

You can preview the new version of the docs built from this PR on readthedocs:

This PR depends on several other open PRs to actually make some of the API changes that're documented here: #9188, #9190, #9197, #9200, #9207.

@seanh seanh requested a review from robertknight December 12, 2024 17:17
@@ -102,7 +102,7 @@ info:
| 500 | Server Error | An error occurred with our API |

servers:
- url: https://api.hypothes.is/api
- url: https://hypothes.is/api
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought I'd fix this while I was here: all the example URLs in our docs have been wrong, presumably since we first moved to redoc.

@@ -111,6 +111,7 @@ tags:
- name: general
- name: annotations
- name: groups
- name: memberships
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The membership APIs used to be listed under groups in the docs. I'm moving them out into their own separate section since they do form a separate set of endpoints (list-memberships, add-membership, get-membership, edit-membership and delete-membership). Like annotation, user and group, "membership" is now a first-class noun in the h model and API.

Comment on lines +126 to +144
PageNumber:
name: "page[number]"
in: query
required: false
description: Which page of results to get
schema:
type: int
default: 1
minimum: 1
PageSize:
name: "page[size]"
in: query
required: false
description: How many items to get per page
schema:
type: int
default: 20
minimum: 1
maximum: 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting the pagination query params in a reusable place. They're currently only used by one endpoint but if we ever add another endpoint with page-based pagination we'll want to re-use the same query params. That said I don't know if/how you can customize the default and maximum values when reusing a parameter in OpenAPI. 🤷‍♂️ A problem for another day

Copy link
Contributor

Choose a reason for hiding this comment

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

That said I don't know if/how you can customize the default and maximum values when reusing a parameter in OpenAPI. 🤷‍♂️ A problem for another day

You can use allOf where one of the "sides" is a $ref to the common parts of the schema, and the rest are the parts that are different.

Comment on lines +288 to +293
Membership:
$ref: './schemas/membership.yaml#/Membership'
MembershipCreate:
$ref: './schemas/membership-create.yaml#/Membership'
PaginationMeta:
$ref: './schemas/pagination-meta.yaml#/PaginationMeta'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reusable schemas for various parts of membership API request and response bodies.

@@ -100,7 +100,7 @@ info:
| 500 | Server Error | An error occurred with our API |

servers:
- url: https://api.hypothes.is/api
- url: https://hypothes.is/api
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every change made to hypothesis-v1.yaml has to be duplicated in hypothesis-v2.yaml as well.

@seanh seanh requested a review from acelaya December 12, 2024 17:30
@seanh seanh marked this pull request as ready for review December 12, 2024 17:30
@seanh seanh requested review from Copilot and removed request for robertknight December 12, 2024 17:34

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 5 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • docs/_extra/api-reference/hypothesis-v1.yaml: Language not supported
  • docs/_extra/api-reference/hypothesis-v2.yaml: Language not supported
Comments skipped due to low confidence (1)

docs/_extra/api-reference/schemas/membership.yaml:27

  • The example for the 'actions' property includes 'updates.roles.member', which is inconsistent with the defined roles ('member', 'moderator', 'admin', 'owner'). Update the example to match the defined roles.
example: ["delete", "updates.roles.member"]
Comment on lines +893 to +898
If the `page[number]` query param is included in the request then the response will be paginated.
If there's no `page[number]` query param in the request then a legacy,
un-paginated response format will be used. In the future this legacy
un-paginated response format will be removed and the paginated response
format will be used even when the `page[number]` query param is not
present.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a PR ready to remove the un-paginated API when we're ready to do that, and I've included removing this stuff from the docs: #9183

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

LGTM

type: array
items:
$ref: '#/components/schemas/Membership'
- title: "Unpaginated (deprecated)"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is deprecated, I reckon you can also add deprecated: true, but I would need to check the docs to confirm this in fact possible in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did try this but it didn't seem to do anything

@seanh seanh merged commit 3081a7a into main Jan 7, 2025
10 checks passed
@seanh seanh deleted the update-api-docs branch January 7, 2025 17:35
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.

Update h's API docs with recent group membership API changes
2 participants