hitler winner and spectator poll options #7
Conversation
WalkthroughThis update enhances a Telegram bot project with improved error handling, logging, and database management. It introduces a CI/CD pipeline for the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
docker-compose.yamlis excluded by:!**/*.yamlpyproject.tomlis excluded by:!**/*.toml
Files selected for processing (17)
- .dockerignore (1 hunks)
- .github/workflows/deploy.yml (2 hunks)
- .github/workflows/develop.yml (1 hunks)
- README.md (1 hunks)
- src/main.py (2 hunks)
- src/callbacks/receive_poll_answer.py (1 hunks)
- src/config.py (2 hunks)
- src/data_models/Game.py (1 hunks)
- src/data_models/Player.py (1 hunks)
- src/data_models/Record.py (3 hunks)
- src/db.py (1 hunks)
- src/get_handlers.py (1 hunks)
- src/handlers/error_handler.py (3 hunks)
- src/handlers/game.py (2 hunks)
- src/handlers/save.py (3 hunks)
- src/services/db_service.py (1 hunks)
- src/sql/init.sql (3 hunks)
Files skipped from review due to trivial changes (3)
- .dockerignore
- README.md
- src/get_handlers.py
Additional comments: 12
src/data_models/Game.py (1)
- 13-24: The
validate_resultsmethod's return typeLiteral["CH", "DH", "FW", "LW"]does not match the method's documentation or expected outcomes based on the game's logic. Ensure the return type accurately reflects all possible outcomes and that the method's logic correctly identifies these outcomes.Consider verifying the method's logic against all possible game outcomes to ensure it covers all scenarios accurately.
src/config.py (2)
- 10-14: Setting default values for
TELEGRAM_BOT_TOKENandDEVELOPER_CHAT_IDtoNoneis appropriate for optional environment variables. However, ensure that downstream code properly handlesNonevalues for these variables to avoid runtime errors.- 27-31: The addition of new game poll outcomes is consistent with the PR's objectives. Ensure that all references to these outcomes in the codebase are updated to match the new options.
Verification successful
The examination of
src/data_models/Record.pyandsrc/handlers/game.pyconfirms that the references toGAME_POLL_OUTCOMESare correctly updated to match the new options added insrc/config.py. The code dynamically handles these outcomes, ensuring compatibility with the new options. Therefore, the addition of new game poll outcomes is consistent with the PR's objectives, and all references in the codebase are appropriately updated to match the new options.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to GAME_POLL_OUTCOMES are updated. grep -R "GAME_POLL_OUTCOMES" src/Length of output: 268
Script:
#!/bin/bash # Examine the usage of GAME_POLL_OUTCOMES in src/data_models/Record.py and src/handlers/game.py cat src/data_models/Record.py cat src/handlers/game.pyLength of output: 2735
src/__main__.py (1)
- 33-37: Moving the logging configuration within the exception handling block is a good practice to ensure that logging is set up before any potential errors occur. However, ensure that this change does not affect logging configurations in other parts of the application.
src/data_models/Record.py (1)
- 13-28: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [16-41]
The update to the
shorten_rolemethod to handle additional role mappings and allow forNoneas a return value is consistent with the PR's objectives. However, ensure that all possible roles are covered and that the method's documentation is updated to reflect these changes.src/handlers/game.py (1)
- 35-39: The addition of saving a
Playroominstance is consistent with the PR's objectives. Ensure that thesave_playroomfunction properly handles all edge cases, such as duplicate entries, to avoid database integrity errors.src/callbacks/receive_poll_answer.py (1)
- 16-40: The update to handle poll retractions by deleting previous votes and saving player information upon receiving a poll answer is consistent with the PR's objectives. Ensure that the
save_playerfunction properly handles all edge cases, such as duplicate entries, to avoid database integrity errors.src/db.py (1)
- 10-10: Updating the parameter passed to the
aiosqlite.connectfunction to useconfig.SQLITE_DB_FILE_PATHis consistent with the PR's objectives and ensures that the database connection uses the correct path.src/sql/init.sql (1)
- 3-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-53]
The schema changes, including renaming fields, adding new fields, and modifying existing fields, are consistent with the PR's objectives. Ensure that all references to these fields in the application code are updated to match the new schema.
src/handlers/error_handler.py (1)
- 41-50: Adding a conditional check for
config.DEVELOPER_CHAT_IDbefore sending error messages via Telegram is a good practice to avoid attempting to send messages when the developer chat ID is not set. Ensure that the error logging provides sufficient information for troubleshooting in the absence of Telegram notifications.src/services/db_service.py (1)
- 30-69: The addition of
save_playroom,save_player, andsave_gamefunctions is consistent with the PR's objectives. Ensure that these functions properly handlesqlite3.IntegrityErrorto log appropriate messages without crashing the application. Additionally, verify that the SQL statements are correct and match the database schema.src/handlers/save.py (1)
- 88-97: The enhancements to include saving a game in addition to saving a record are consistent with the PR's objectives. Ensure that the
save_gamefunction is correctly implemented and that all necessary data is accurately saved to the database.
src/data_models/Player.py
Outdated
| first_name: Optional[str | None] | ||
| full_name: Optional[str | None] | ||
| last_name: Optional[str | None] | ||
| is_bot: Optional[bool] = False | ||
| language_code: Optional[str | None] |
There was a problem hiding this comment.
The use of Optional[str | None] is redundant. Optional[str] already implies that the value can be None. Simplify the type annotations for first_name, full_name, last_name, and language_code.
- first_name: Optional[str | None]
+ first_name: Optional[str]
- full_name: Optional[str | None]
+ full_name: Optional[str]
- last_name: Optional[str | None]
+ last_name: Optional[str]
- language_code: Optional[str | None]
+ language_code: Optional[str]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| first_name: Optional[str | None] | |
| full_name: Optional[str | None] | |
| last_name: Optional[str | None] | |
| is_bot: Optional[bool] = False | |
| language_code: Optional[str | None] | |
| first_name: Optional[str] | |
| full_name: Optional[str] | |
| last_name: Optional[str] | |
| is_bot: Optional[bool] = False | |
| language_code: Optional[str] |
src/data_models/Player.py
Outdated
| @field_validator("is_bot", mode="after") | ||
| @classmethod | ||
| def validate_bot(cls, v: bool) -> str: | ||
| return "TRUE" if v else "FALSE" # sqlite3 does not support boolean type |
There was a problem hiding this comment.
The validate_bot method is supposed to return a bool but is annotated to return str. This is incorrect based on the method's purpose and the comment indicating SQLite's lack of boolean support. The method should return a boolean value, and the conversion to a string (if necessary) should be handled where SQLite queries are executed.
- def validate_bot(cls, v: bool) -> str:
+ def validate_bot(cls, v: bool) -> bool:
- return "TRUE" if v else "FALSE"
+ return vCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @field_validator("is_bot", mode="after") | |
| @classmethod | |
| def validate_bot(cls, v: bool) -> str: | |
| return "TRUE" if v else "FALSE" # sqlite3 does not support boolean type | |
| @field_validator("is_bot", mode="after") | |
| @classmethod | |
| def validate_bot(cls, v: bool) -> bool: | |
| return v # sqlite3 does not support boolean type |
src/data_models/Game.py
Outdated
| poll_id: int | ||
| chat_id: int | ||
| results: dict # Literal["Hitler Canceler", "Fascist Law", "Hitler Death", "Liberal Law"] | ||
| creator_id: int |
There was a problem hiding this comment.
The results field is annotated as a dict without specifying key and value types. For better type safety and clarity, specify the expected types for keys and values.
- results: dict
+ results: dict[str, str]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| poll_id: int | |
| chat_id: int | |
| results: dict # Literal["Hitler Canceler", "Fascist Law", "Hitler Death", "Liberal Law"] | |
| creator_id: int | |
| poll_id: int | |
| chat_id: int | |
| results: dict[str, str] # Literal["Hitler Canceler", "Fascist Law", "Hitler Death", "Liberal Law"] | |
| creator_id: int |
src/__main__.py
Outdated
|
|
||
| def main() -> None: | ||
| if not config.TELEGRAM_BOT_TOKEN: | ||
| raise ValueError("TELEGRAM_BOT_TOKEN env variable" "wasn't porpoused.") | ||
| raise ValueError("TELEGRAM_BOT_TOKEN env variable" "wasn't purposed.") |
There was a problem hiding this comment.
The error message concatenation is unnecessary and could be simplified.
- raise ValueError("TELEGRAM_BOT_TOKEN env variable" "wasn't purposed.")
+ raise ValueError("TELEGRAM_BOT_TOKEN env variable wasn't purposed.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| raise ValueError("TELEGRAM_BOT_TOKEN env variable" "wasn't purposed.") | |
| raise ValueError("TELEGRAM_BOT_TOKEN env variable wasn't purposed.") |
.github/workflows/deploy.yml
Outdated
| steps: | ||
| - name: SSH into production server and deploy | ||
| uses: appleboy/ssh-action@master | ||
| env: | ||
| TELEGRAM_BOT_TOKEN: ${{ secrets.PROD_TELEGRAM_BOT_TOKEN }} | ||
| DEVELOPER_CHAT_ID: ${{ secrets.DEVELOPER_CHAT_ID }} | ||
| SQLITE_DB_FILE_PATH: ${{ vars.SQLITE_DB_FILE_PATH }} | ||
| with: | ||
| host: ${{ secrets.HOST }} | ||
| username: ${{ secrets.USERNAME }} |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [33-47]
The addition of environment variables TELEGRAM_BOT_TOKEN, DEVELOPER_CHAT_ID, and SQLITE_DB_FILE_PATH is appropriate for the deployment script. However, there's a typo in the environment variable reference for SQLITE_DB_FILE_PATH. It should use secrets instead of vars.
- SQLITE_DB_FILE_PATH: ${{ vars.SQLITE_DB_FILE_PATH }}
+ SQLITE_DB_FILE_PATH: ${{ secrets.SQLITE_DB_FILE_PATH }}
.github/workflows/develop.yml
Outdated
| env: | ||
| TELEGRAM_BOT_TOKEN: ${{ secrets.DEV_TELEGRAM_BOT_TOKEN }} | ||
| DEVELOPER_CHAT_ID: ${{ secrets.DEVELOPER_CHAT_ID }} | ||
| SQLITE_DB_FILE_PATH: ${{ vars.SQLITE_DB_FILE_PATH }} | ||
| with: | ||
| host: ${{ secrets.HOST }} | ||
| username: ${{ secrets.USERNAME }} | ||
| password: ${{ secrets.PASSWORD }} | ||
| key: ${{ secrets.PRIVATE_KEY }} | ||
| script: | | ||
| cd ./_work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }} | ||
| docker compose down | ||
| docker compose up -d | ||
| docker compose ps | ||
| envs: TELEGRAM_BOT_TOKEN,DEVELOPER_CHAT_ID,SQLITE_DB_FILE_PATH |
There was a problem hiding this comment.
The addition of environment variables TELEGRAM_BOT_TOKEN, DEVELOPER_CHAT_ID, and SQLITE_DB_FILE_PATH is appropriate for the deployment script. However, there's a typo in the environment variable reference for SQLITE_DB_FILE_PATH. It should use secrets instead of vars, similar to the issue in deploy.yml.
- SQLITE_DB_FILE_PATH: ${{ vars.SQLITE_DB_FILE_PATH }}
+ SQLITE_DB_FILE_PATH: ${{ secrets.SQLITE_DB_FILE_PATH }}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| env: | |
| TELEGRAM_BOT_TOKEN: ${{ secrets.DEV_TELEGRAM_BOT_TOKEN }} | |
| DEVELOPER_CHAT_ID: ${{ secrets.DEVELOPER_CHAT_ID }} | |
| SQLITE_DB_FILE_PATH: ${{ vars.SQLITE_DB_FILE_PATH }} | |
| with: | |
| host: ${{ secrets.HOST }} | |
| username: ${{ secrets.USERNAME }} | |
| password: ${{ secrets.PASSWORD }} | |
| key: ${{ secrets.PRIVATE_KEY }} | |
| script: | | |
| cd ./_work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }} | |
| docker compose down | |
| docker compose up -d | |
| docker compose ps | |
| envs: TELEGRAM_BOT_TOKEN,DEVELOPER_CHAT_ID,SQLITE_DB_FILE_PATH | |
| env: | |
| TELEGRAM_BOT_TOKEN: ${{ secrets.DEV_TELEGRAM_BOT_TOKEN }} | |
| DEVELOPER_CHAT_ID: ${{ secrets.DEVELOPER_CHAT_ID }} | |
| SQLITE_DB_FILE_PATH: ${{ secrets.SQLITE_DB_FILE_PATH }} | |
| with: | |
| host: ${{ secrets.HOST }} | |
| username: ${{ secrets.USERNAME }} | |
| password: ${{ secrets.PASSWORD }} | |
| key: ${{ secrets.PRIVATE_KEY }} | |
| script: | | |
| cd ./_work/${{ env.REPO_NAME }}/${{ env.REPO_NAME }} | |
| docker compose down | |
| docker compose up -d | |
| docker compose ps | |
| envs: TELEGRAM_BOT_TOKEN,DEVELOPER_CHAT_ID,SQLITE_DB_FILE_PATH |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (4)
- src/config.py (1 hunks)
- src/data_models/Record.py (3 hunks)
- src/handlers/save.py (2 hunks)
- src/services/db_service.py (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/config.py
- src/data_models/Record.py
- src/handlers/save.py
| from src.data_models.Playroom import Playroom | ||
| import logging | ||
| import sqlite3 | ||
|
|
||
| from src.data_models.Game import Game | ||
| from src.data_models.Player import Player |
There was a problem hiding this comment.
Duplicate imports for logging, sqlite3, and data models Game, Player, and Playroom. Remove the redundant lines to clean up the code.
- import logging
- import sqlite3
- from src.data_models.Game import Game
- from src.data_models.Player import Player
- from src.data_models.Playroom import PlayroomCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| from src.data_models.Playroom import Playroom | |
| import logging | |
| import sqlite3 | |
| from src.data_models.Game import Game | |
| from src.data_models.Player import Player |
| try: | ||
| await execute( | ||
| """INSERT INTO records (creator_id, player_id, playroom_id, game_id, role) | ||
| VALUES (?, ?, ?, ?, ?)""", | ||
| ( | ||
| record.creator_id, | ||
| record.player_id, | ||
| record.playroom_id, | ||
| record.game_id, | ||
| record.role, | ||
| ), | ||
| ) | ||
| except sqlite3.IntegrityError: | ||
| logging.info( | ||
| f"Something went wrong with game: {record.game_id} in playroom {record.playroom_id}" | ||
| ) |
There was a problem hiding this comment.
The save_record function correctly handles sqlite3.IntegrityError with a try-except block and logs an informative message. However, consider enhancing the log message to include more details about the error for better debugging.
- logging.info(
+ logging.error(Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| try: | |
| await execute( | |
| """INSERT INTO records (creator_id, player_id, playroom_id, game_id, role) | |
| VALUES (?, ?, ?, ?, ?)""", | |
| ( | |
| record.creator_id, | |
| record.player_id, | |
| record.playroom_id, | |
| record.game_id, | |
| record.role, | |
| ), | |
| ) | |
| except sqlite3.IntegrityError: | |
| logging.info( | |
| f"Something went wrong with game: {record.game_id} in playroom {record.playroom_id}" | |
| ) | |
| try: | |
| await execute( | |
| """INSERT INTO records (creator_id, player_id, playroom_id, game_id, role) | |
| VALUES (?, ?, ?, ?, ?)""", | |
| ( | |
| record.creator_id, | |
| record.player_id, | |
| record.playroom_id, | |
| record.game_id, | |
| record.role, | |
| ), | |
| ) | |
| except sqlite3.IntegrityError: | |
| logging.error( | |
| f"Something went wrong with game: {record.game_id} in playroom {record.playroom_id}" | |
| ) |
| async def save_playroom(playroom: Playroom) -> None: | ||
| """Add a game room to the bot_data""" | ||
| try: | ||
| await execute( | ||
| "INSERT INTO playrooms (id, name) VALUES (?, ?)", | ||
| (playroom.telegram_chat_id, playroom.name), | ||
| ) | ||
| except sqlite3.IntegrityError: | ||
| logging.info(f"Playroom {playroom.name} already exists in the database") |
There was a problem hiding this comment.
The save_playroom function is duplicated. Remove the duplicate definition to maintain code cleanliness and avoid confusion.
- async def save_playroom(playroom: Playroom) -> None:
- """Add a game room to the bot_data"""
- try:
- await execute(
- "INSERT INTO playrooms (id, name) VALUES (?, ?)",
- (playroom.telegram_chat_id, playroom.name),
- )
- except sqlite3.IntegrityError:
- logging.info(f"Playroom {playroom.name} already exists in the database")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async def save_playroom(playroom: Playroom) -> None: | |
| """Add a game room to the bot_data""" | |
| try: | |
| await execute( | |
| "INSERT INTO playrooms (id, name) VALUES (?, ?)", | |
| (playroom.telegram_chat_id, playroom.name), | |
| ) | |
| except sqlite3.IntegrityError: | |
| logging.info(f"Playroom {playroom.name} already exists in the database") |
Summary by CodeRabbit