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

Feat/tr/pagination #123

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Feat/tr/pagination #123

wants to merge 31 commits into from

Conversation

theodorehreuter
Copy link

@theodorehreuter theodorehreuter commented Jan 3, 2025

Related Issue(s):

Proposed Changes:

Adding pagination for 4 endpoints:

  1. GET /orders
  2. GET /products
  3. GET /orders/{order_id}/statuses
  4. POST /products/{product_id}/opportunities

We are using token based approach to pagination.

BREAKING CHANGES:

  • Moving OrderCollection construction to root_router.py instead of root_backend.py
  • Pagination support on the backend methods has changed returned types in root_backend.py and product_backend.py to tuples, to include returning a new pagination token.

PR Checklist:

  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.

…agination tests: creating incomplete test to start testing pagination
… feat: replacing token in url if new token is provided tests: continue to updae test to validate additional pieces of the limit/token expected functionality, still breaking on limit check feat: removing use or Orders type and going with tuple instead
… object if we are returned a token. tests: created simple test implementation to abstract out token handling and generation
…r bad token tests: reworking pagination tests tests: adding handle to max limit in test implemented backend
tests/application.py Outdated Show resolved Hide resolved
tests/application.py Outdated Show resolved Hide resolved
tests/application.py Outdated Show resolved Hide resolved
tests/test_order.py Outdated Show resolved Hide resolved
tests/test_order.py Outdated Show resolved Hide resolved
tests/test_order.py Outdated Show resolved Hide resolved
tests/test_order.py Outdated Show resolved Hide resolved
tests/test_order.py Outdated Show resolved Hide resolved
…getting records until no more are available ftests: added separate test to get 404 back with no orders and using a token
…y features after token lookup tests: separating empty orders check to separate test
… issue with in memory DB by adding missing __init__ to class. tests: added multiple orders to create_order fixture and passed this fixture to other tests using previously existing create_order_allowed_paylaods tests: added setup_pagination fixture that is now necessary again after fixing the in memory db init issue
…ing pagination token return Maybe[str] instead of usng empty strings feat: tweaking logic path in endpoint functions to make a single return point tests: updating tests as necessary to accomodating changes
… where next token and search params are not their own separat key value pairs but in the same body
products=[pr.get_product(request) for pr in self.product_routers.values()],
links=[
def get_products(
self, request: Request, next: str | None = None, limit: int = 10
Copy link
Contributor

@philvarner philvarner Jan 27, 2025

Choose a reason for hiding this comment

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

next would be better as Maybe[str] instead of Optional[str]

Copy link
Author

Choose a reason for hiding this comment

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

per async convo will leave as is for now, pydantic + fastapi dont like Maybe[str] as an arg type.

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@philvarner
Copy link
Contributor

Done leaving comments.

…ethods to clean up endpoint business logic. tests: small tweaks to test based on PR feedback
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