-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
add missing page_info / cursors for Association Proxy logic #232
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThe pull request addresses a bug in the Sequence diagram for association proxy resolver with relay paginationsequenceDiagram
participant Client
participant Resolver
participant AssociationProxy
participant Connection
Client->>Resolver: Request data
Resolver->>AssociationProxy: resolve(info)
alt in_between_objects is None
AssociationProxy-->>Connection: Create with empty edges + PageInfo
else has objects
AssociationProxy->>AssociationProxy: Process objects
AssociationProxy-->>Connection: Create with edges + PageInfo + cursors
end
Connection-->>Client: Return paginated data
Class diagram for connection types with relay paginationclassDiagram
class PageInfo {
+boolean has_next_page
+boolean has_previous_page
+string start_cursor
+string end_cursor
}
class ConnectionType {
+List~Edge~ edges
+PageInfo page_info
}
class Edge {
+Object node
+string cursor
}
ConnectionType *-- PageInfo
ConnectionType *-- Edge
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tylernisonoff - I've reviewed your changes - here's some feedback:
Overall Comments:
- The hardcoded
has_next_page
andhas_previous_page
values should be calculated based on the actual data and pagination parameters rather than always being set toFalse
- Consider adding tests for the pagination behavior to prevent future regressions, even though the original code was untested
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description
association_proxy_resolver_for
has a few code blocks that were not updated for the relay changes. Association proxy mapping would fail due do constructors missing eitherpage_info
orcursor
args.Types of Changes
Issues Fixed or Closed by This PR
Checklist
I haven't added tests as it looks like this code is originally untested.
Summary by Sourcery
Bug Fixes:
page_info
orcursor
arguments in constructors.