-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add get_by_guild_id helper to config services #140
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
Conversation
Resolves GH #88 - Add helper methods to reduce duplication and allow usage outside of routes. - Add get_by_guild_id() to GitHubConfigService, SOTagsConfigService, AllowedUsersConfigService, and ForumConfigService - Update controller endpoints to use the new helpers - Add 8 tests for the new methods (success and not found cases) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideAdds typed get_by_guild_id helper methods to guild-related config services, refactors controllers to use them instead of raw get(..., id_attribute='guild_id'), and introduces unit tests validating success and not-found behavior for each service. Sequence diagram for guild GitHub config retrieval via get_by_guild_idsequenceDiagram
actor Client
participant GuildsController
participant GitHubConfigService
participant SQLAlchemyAsyncRepositoryService
participant GitHubConfigRepository
Client->>GuildsController: GET /guilds/{guild_id}/github/config
GuildsController->>GitHubConfigService: get_by_guild_id(guild_id)
GitHubConfigService->>SQLAlchemyAsyncRepositoryService: get(guild_id, id_attribute=guild_id)
SQLAlchemyAsyncRepositoryService->>GitHubConfigRepository: fetch_by_guild_id(guild_id)
alt config exists
GitHubConfigRepository-->>SQLAlchemyAsyncRepositoryService: GitHubConfig
SQLAlchemyAsyncRepositoryService-->>GitHubConfigService: GitHubConfig
GitHubConfigService-->>GuildsController: GitHubConfig
GuildsController-->>Client: 200 GitHubConfigSchema
else config not found
GitHubConfigRepository-->>SQLAlchemyAsyncRepositoryService: None
SQLAlchemyAsyncRepositoryService-->>GitHubConfigService: NotFoundError
GitHubConfigService-->>GuildsController: NotFoundError
GuildsController-->>Client: 404 error response
end
Class diagram for new get_by_guild_id helpers in guild config servicesclassDiagram
class SQLAlchemyAsyncRepositoryService {
+async get(id: int, id_attribute: str) ConfigModel
}
class GitHubConfigService {
+repository_type
+match_fields
+async get_by_guild_id(guild_id: int) GitHubConfig
+async to_model(data: ModelDictT_GitHubConfig, operation: str) GitHubConfig
}
class SOTagsConfigService {
+repository_type
+match_fields
+async get_by_guild_id(guild_id: int) SOTagsConfig
+async to_model(data: ModelDictT_SOTagsConfig, operation: str) SOTagsConfig
}
class AllowedUsersConfigService {
+repository_type
+match_fields
+async get_by_guild_id(guild_id: int) AllowedUsersConfig
+async to_model(data: ModelDictT_AllowedUsersConfig, operation: str) AllowedUsersConfig
}
class ForumConfigService {
+repository_type
+match_fields
+async get_by_guild_id(guild_id: int) ForumConfig
+async to_model(data: ModelDictT_ForumConfig, operation: str) ForumConfig
}
class GitHubConfig
class SOTagsConfig
class AllowedUsersConfig
class ForumConfig
class ModelDictT_GitHubConfig
class ModelDictT_SOTagsConfig
class ModelDictT_AllowedUsersConfig
class ModelDictT_ForumConfig
SQLAlchemyAsyncRepositoryService <|-- GitHubConfigService
SQLAlchemyAsyncRepositoryService <|-- SOTagsConfigService
SQLAlchemyAsyncRepositoryService <|-- AllowedUsersConfigService
SQLAlchemyAsyncRepositoryService <|-- ForumConfigService
GitHubConfigService --> GitHubConfig
SOTagsConfigService --> SOTagsConfig
AllowedUsersConfigService --> AllowedUsersConfig
ForumConfigService --> ForumConfig
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🚅 Deployed to the byte-pr-140 environment in byte
|
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 - I've left some high level feedback:
- The four
get_by_guild_idhelpers are identical across services; consider extracting a common helper/mixin (e.g., a baseGuildScopedConfigServicewith a genericget_by_guild_id) to avoid repetition and keep future changes in one place. - The new
get_by_guild_idtests are very similar across services; you might reduce duplication and make them easier to maintain by parameterizing over service/repository/model details instead of repeating almost identical test bodies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The four `get_by_guild_id` helpers are identical across services; consider extracting a common helper/mixin (e.g., a base `GuildScopedConfigService` with a generic `get_by_guild_id`) to avoid repetition and keep future changes in one place.
- The new `get_by_guild_id` tests are very similar across services; you might reduce duplication and make them easier to maintain by parameterizing over service/repository/model details instead of repeating almost identical test bodies.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Pull request overview
This PR adds get_by_guild_id() helper methods to four configuration service classes to reduce code duplication and enable usage outside of route handlers, resolving issue #88.
Changes:
- Added
get_by_guild_id()helper methods toGitHubConfigService,SOTagsConfigService,AllowedUsersConfigService, andForumConfigService - Refactored four controller endpoints to use the new helper methods instead of direct
get()calls withid_attributeparameter - Added comprehensive test coverage with 8 new tests (success and not found cases for each service)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| services/api/src/byte_api/domain/guilds/services.py | Added get_by_guild_id() helper methods to all four config service classes with consistent implementation and documentation |
| services/api/src/byte_api/domain/guilds/controllers.py | Updated four endpoint methods to use the new helper methods and removed the TODO comment |
| tests/unit/api/test_guilds_services.py | Added 8 new unit tests covering success and error cases for all four new helper methods, updated __all__ exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Documentation preview will be available shortly at https://jacobcoffee.github.io/byte-docs-preview/140 |
|
After further review, this PR adds unnecessary abstraction. The The original TODO (#88) was asking for helper methods usable outside routes - but that capability already exists. Adding 4 identical wrapper methods violates DRY and creates maintenance burden. Closing as unnecessary. |
Summary
get_by_guild_id()helper methods to config servicesChanges
GitHubConfigServiceget_by_guild_id(guild_id: int) -> GitHubConfigSOTagsConfigServiceget_by_guild_id(guild_id: int) -> SOTagsConfigAllowedUsersConfigServiceget_by_guild_id(guild_id: int) -> AllowedUsersConfigForumConfigServiceget_by_guild_id(guild_id: int) -> ForumConfigEach method:
NotFoundErrorif no config existsTest plan
get_by_guild_idmethods (success + not found cases)🤖 Generated with Claude Code
Summary by Sourcery
Add guild ID-based lookup helpers to guild configuration services and update controllers and tests to use and validate them.
New Features:
Enhancements:
Tests: