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

Absences: offset parameter does not match documentation #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sun
Copy link

@sun sun commented Jan 10, 2022

Problem

  • The GET /company/time-offs parameter offset is documented as zero based and the wording "telling an offset from the first record that would be returned" suggests that it accepts an offset of records instead of pages.

- name: offset
in: query
required: false
description: Pagination attribute to identify which page you are requesting, by the form of telling an offset from the first record that would be returned.
schema:
type: integer
minimum: 0
default: 0

Actual behavior

  • The offset parameter is based on 1. Passing 0 is automatically converted into 1 in the API. This results in duplicate response data when the client is counting up $offset++, as the client is first passing 0 then 1.

  • The offset parameter indicates the "page" to retrieve, chunked by limit.

- The `offset` parameter is based on `1`. Passing `0` is automatically converted into `1` in the API. This results in duplicate response data when the client is counting up `$offset++`, as the client is first passing `0` then `1`.

- The `offset` parameter indicates the "page" to retrieve, chunked by `limit`.
default: 200
- name: offset
in: query
required: false
description: Pagination attribute to identify which page you are requesting, by the form of telling an offset from the first record that would be returned.
description: Pagination attribute to identify which page to request from the chunks indicated by `limit`. The value `0` is interpreted as page `1`. You cannot pass a value larger than the available amount of pages, as returned by the `total_pages` meta data in the response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Page 1 is also interpreted as page 1 right? Maybe we need to add that to make it more clear and explicit?

@mdbenito
Copy link
Contributor

mdbenito commented Mar 5, 2022

I think documenting this is important. If it were not for this PR I would have wasted quite some time wondering why the pagination didn't work as expected from other endpoints.

@sun
Copy link
Author

sun commented Mar 5, 2022

Thanks for your feedback. I'll incorporate it into the PR next week.

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.

3 participants