130 lines
4.3 KiB
Markdown
130 lines
4.3 KiB
Markdown
# JIGAIDO — Clarifying Questions
|
|
|
|
## Context
|
|
|
|
Pre-implementation review of JIGAIDO Telegram bounty bot. Below are questions gathered during code review that need answers before proceeding.
|
|
|
|
---
|
|
|
|
## Questions
|
|
|
|
### 1. `__init__.py` is empty
|
|
File `apps/telegram-bot/__init__.py` is 0 bytes. It's a Python package marker but serves no function.
|
|
- **Can we remove it?** Does anything import from `apps/telegram-bot/` as a package?
|
|
- Or should it contain `__version__`, `__app_name__`, or similar?
|
|
|
|
### 2. `jigaido-bot.service` as a template
|
|
`jigaido-bot.service` currently has hardcoded values:
|
|
- `User=shoko`
|
|
- `Group=shoko`
|
|
- `WorkingDirectory=/home/shoko/repositories/jigaido/apps/telegram-bot/`
|
|
|
|
**Proposal:** Make it a template (`jigaido-bot.service.j2` or similar). `setup.sh` detects the actual user/group/path and generates the real `.service` file.
|
|
|
|
- Is this the intended approach?
|
|
- Should `setup.sh` also handle env file generation?
|
|
|
|
### 3. Logging separation
|
|
Currently logs go to stdout (handled by systemd's `journalctl`).
|
|
|
|
**Proposal:**
|
|
- `jigaido.log` — INFO level
|
|
- `jigaido-error.log` — ERROR level
|
|
- Path: `/var/log/jigaido/` or `~/.local/share/jigaido/logs/`
|
|
- Files should NOT be tracked by git (add to `.gitignore`)
|
|
|
|
**Questions:**
|
|
- Preferred log directory?
|
|
- Log rotation needed (e.g., `maxsize=10MB, rotate=3`)?
|
|
- Should we use Python's `logging` module with `RotatingFileHandler`, or shell redirect in the service file?
|
|
|
|
### 4. What does `/start` actually do?
|
|
|
|
SPEC.md says:
|
|
> "First admin assignment is automatic when the bot detects a new group."
|
|
|
|
Code (`cmd_start`):
|
|
- Group: `ensure_group()` → upserts group + adds creator as admin
|
|
- DM: `ensure_user()` → upserts user record
|
|
|
|
**Questions:**
|
|
- Should `/start` in a group re-trigger admin assignment if creator already exists? (Currently yes — `add_group_admin` is idempotent via `UNIQUE` constraint, so it's safe.)
|
|
- Should `/start` message be idempotent (same message every time) or conditional?
|
|
|
|
### 5. `db.init_db()` — safe to run multiple times?
|
|
|
|
`schema.sql` uses `CREATE TABLE IF NOT EXISTS ...`. `db.init_db()` calls `executescript()`.
|
|
|
|
**Clarification needed:**
|
|
- Is it safe to call `init_db()` every time the bot starts? (Seems yes — tables only created if absent.)
|
|
- Should there be a schema version table for future migrations?
|
|
|
|
### 6. `TELEGRAM_BOT_USERNAME` hardcoded
|
|
|
|
`commands.py:13`:
|
|
```python
|
|
TELEGRAM_BOT_USERNAME = "your_bot_username"
|
|
```
|
|
|
|
This is never used in the current code (searched — no references). The `post_init` in `bot.py` sets commands without using it.
|
|
|
|
**Question:** Should this be removed entirely, or should it be set from `JIGAIDO_BOT_USERNAME` env var?
|
|
|
|
### 7. `REMAINDER_WINDOW_DAYS` duplicated
|
|
|
|
Defined in both:
|
|
- `commands.py:15` — `REMINDER_WINDOW_DAYS = 7`
|
|
- `cron.py:27` — `REMINDER_WINDOW_DAYS = 7`
|
|
|
|
**Decision needed:** User said "drop cron for now." Confirm:
|
|
- Remove `cron.py` entirely?
|
|
- Or keep for future use?
|
|
|
|
### 8. Link dedup — can we clear a link?
|
|
|
|
`update_bounty` uses `COALESCE(?, link)`:
|
|
```python
|
|
SET text = COALESCE(?, text),
|
|
link = COALESCE(?, link),
|
|
due_date_ts = COALESCE(?, due_date_ts)
|
|
```
|
|
|
|
Passing `None` as `link` does NOT clear it — `COALESCE` returns the existing value.
|
|
|
|
**Question:** Is this intentional? Should there be a way to clear/reset a link or due date?
|
|
|
|
### 9. Git as update mechanism
|
|
|
|
Proposal: `git pull` in the deployment directory as the update mechanism.
|
|
|
|
**Questions:**
|
|
- Is the `.service` file and env file generated by `setup.sh` stored outside the git-tracked directory?
|
|
- Or are they gitignored files inside the repo?
|
|
- What's the update procedure? `git pull && systemctl restart jigaido-bot`?
|
|
|
|
### 10. Cron dropped
|
|
|
|
User explicitly said drop cron for now.
|
|
|
|
**Action needed:**
|
|
- Remove `cron.py` and related code from `bot.py`?
|
|
- Or keep but disable?
|
|
- What about `reminder_log` table — keep or remove?
|
|
|
|
---
|
|
|
|
## Priority
|
|
|
|
| # | Question | Priority |
|
|
|---|---|---|
|
|
| 1 | `__init__.py` remove? | Low |
|
|
| 2 | `.service` as template | Medium |
|
|
| 3 | Logging separation | Medium |
|
|
| 4 | `/start` behavior clarification | Medium |
|
|
| 5 | `db.init_db()` safety | Low (already safe) |
|
|
| 6 | `TELEGRAM_BOT_USERNAME` | Low |
|
|
| 7 | `REMAINDER_WINDOW_DAYS` duplication | Low |
|
|
| 8 | Can we clear link/due_date? | Medium |
|
|
| 9 | Git as update mechanism | High |
|
|
| 10 | Cron dropped | Medium |
|