-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add GraphQL schema for results service #37
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: Add GraphQL schema for results service #37
Conversation
Add new GraphQL types and operations: - RankingEntry type for league group rankings - CalendarDate type for upcoming matchdays - getLeagueGroupRanking query - getUpcomingMatchdays query - getAllMatchdaysInSeason query - importMatchdayFromRBT mutation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
WalkthroughThe GraphQL schema adds two public types (RankingEntry, CalendarDate), three new read queries (getLeagueGroupRanking, getUpcomingMatchdays, getAllMatchdaysInSeason) and one mutation (importMatchdayFromRBT); docs and generated HTML pages were added/updated and auth directives ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLAPI as "GraphQL API"
participant Service as "Matchday/Ranking Resolver"
Note over Client,GraphQLAPI `#D6EAF8`: Read queries
Client->>GraphQLAPI: getLeagueGroupRanking(groupId, includeNonCompetitive)
GraphQLAPI->>Service: resolve getLeagueGroupRanking
Service-->>GraphQLAPI: RankingEntry[]
GraphQLAPI-->>Client: RankingEntry[]
Note over Client,GraphQLAPI `#F9E79F`: Upcoming matchdays
Client->>GraphQLAPI: getUpcomingMatchdays(associationId, seasonId)
GraphQLAPI->>Service: resolve getUpcomingMatchdays
Service-->>GraphQLAPI: CalendarDate[]
GraphQLAPI-->>Client: CalendarDate[]
Note over Client,GraphQLAPI `#F5B7B1`: Mutation (auth required)
Client->>GraphQLAPI: importMatchdayFromRBT(groupId, data)
GraphQLAPI->>Service: resolve importMatchdayFromRBT
alt success
Service-->>GraphQLAPI: MatchDay
GraphQLAPI-->>Client: MatchDay
else failure
Service-->>GraphQLAPI: Error
GraphQLAPI-->>Client: Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
schema.graphql (1)
654-655: Consider authorization scope and structured input forimportMatchdayFromRBT.The new mutation accepts a raw
data: String!parameter instead of a structured input type. While this may be intentional for flexible RBT format handling, it reduces API documentation and schema clarity compared to other mutations.Additionally, the mutation only requires
@aws_cognito_user_poolswithout group-based restrictions (unlike admin operations on lines 574–575). Consider whether importing matchday data should be restricted to admins or group leaders with explicit authorization checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
schema.graphql(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
schema.graphql (2)
225-239: LGTM!The new
RankingEntryandCalendarDatetypes are well-designed with clear field semantics and proper references to existing types. Both include appropriate authentication directives.
587-594: Clarify intent ofgetAllMatchdaysInSeasonvs existinggetListOfMatchdaysInSeason.Line 594 introduces
getAllMatchdaysInSeason(seasonId: ID!): [MatchDay!]!, but line 557 already hasgetListOfMatchdaysInSeason(seasonId: ID!): [MatchDay]that appears to serve the same purpose. Both accept the same parameter and return matchday collections for a season.Differences:
- Naming:
getAll*vsgetList*- Return type: non-nullable items/list vs nullable (less strict)
Are both intended, or should one be deprecated to avoid API confusion?
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/assets/navigation.js (1)
1-1: Consider excluding generated documentation from the repository.All documentation files in this PR (navigation.js, TypeDoc HTML pages) appear to be build artifacts. Committing generated files can lead to repository bloat, merge conflicts, and documentation drift from the source schema.
Recommended approach:
- Add
docs/to.gitignore- Generate documentation as part of your CI/CD pipeline or publish workflow
- Host the generated docs separately (e.g., GitHub Pages, documentation site)
If you must commit generated docs:
- Ensure they're regenerated on every schema change
- Consider a separate docs branch to keep main branch history clean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/generated/graphql.model.generated.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
docs/assets/navigation.js(1 hunks)docs/assets/search.js(1 hunks)docs/types/CalendarDate.html(1 hunks)docs/types/MutationImportMatchdayFromRbtArgs.html(1 hunks)docs/types/QueryGetAllMatchdaysInSeasonArgs.html(1 hunks)docs/types/QueryGetLeagueGroupRankingArgs.html(1 hunks)docs/types/QueryGetUpcomingMatchdaysArgs.html(1 hunks)docs/types/RankingEntry.html(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/types/QueryGetAllMatchdaysInSeasonArgs.html
- docs/assets/search.js
- docs/types/CalendarDate.html
🔇 Additional comments (4)
docs/types/RankingEntry.html (1)
1-9: Documentation structure looks good.The RankingEntry type documentation is well-formed with appropriate fields for representing league standings. All numeric statistics use the Int scalar type as expected.
docs/types/MutationImportMatchdayFromRbtArgs.html (1)
1-3: Mutation arguments documented correctly.The importMatchdayFromRBT mutation arguments are properly documented with required groupId and data parameters.
docs/types/QueryGetLeagueGroupRankingArgs.html (1)
1-3: Query arguments well-designed.The getLeagueGroupRanking query has a good parameter structure with required groupId and an optional includeNonCompetitive filter, providing flexibility for different use cases.
docs/types/QueryGetUpcomingMatchdaysArgs.html (1)
1-3: Query arguments documented correctly.The getUpcomingMatchdays query appropriately requires both associationId and seasonId to scope the results.
Note: The actual GraphQL schema file (schema.graphql) does not appear to be included in this PR. Please ensure the source schema has been reviewed and that these generated docs accurately reflect the intended API changes.
Add new GraphQL types and operations:
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.