api: auth negative tests, family ledger test, superadmin doc, export streaming draft #39
Conversation
…doc, export streaming design draft
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the API's robustness and prepares it for future performance optimizations. It introduces crucial negative-path testing for authentication flows and validates data integrity during the creation of new families. Additionally, it improves developer experience by documenting the default superadmin credentials and lays out a detailed architectural plan for implementing efficient, memory-optimized data export streaming. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances testing coverage and documentation by adding negative authentication tests, a family ledger integrity test, superadmin documentation, and a streaming export design document.
- Adds negative path testing for authentication failures (wrong password, inactive user refresh)
- Implements family default ledger integrity verification test
- Documents superadmin default credentials for easier onboarding
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| jive-api/tests/integration/family_default_ledger_test.rs | New test verifying default ledger creation with proper flags and ownership |
| jive-api/tests/integration/auth_login_negative_test.rs | New negative authentication tests for wrong password and inactive user scenarios |
| docs/EXPORT_STREAMING_DESIGN.md | Technical design document for streaming CSV export implementation |
| README.md | Added superadmin default password documentation with usage examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert_eq!(ledgers.len(), 1, "exactly one default ledger expected"); | ||
| let ledger = &ledgers[0]; | ||
| assert_eq!(ledger.family_id, family_id); | ||
| assert_eq!(ledger.is_default.unwrap_or(false), true, "ledger should be default"); |
There was a problem hiding this comment.
The assertion unwrap_or(false), true is unnecessarily complex. Since you expect the ledger to be default, use assert!(ledger.is_default.unwrap_or(false)) or assert_eq!(ledger.is_default, Some(true)) for clearer intent.
| assert_eq!(ledger.is_default.unwrap_or(false), true, "ledger should be default"); | |
| assert_eq!(ledger.is_default, Some(true), "ledger should be default"); |
Copilot uses AI. Check for mistakes.
| let email = format!("inactive_refresh_{}@example.com", uuid::Uuid::new_v4()); | ||
|
|
||
| // Create inactive user (argon2) | ||
| let salt = argon2::password_hash::SaltString::generate(&mut argon2::password_hash::rand_core::OsRng); |
There was a problem hiding this comment.
[nitpick] The salt generation uses a verbose path. Consider importing the types or using a shorter alias: use argon2::password_hash::{rand_core::OsRng, SaltString}; to improve readability.
Copilot uses AI. Check for mistakes.
| 6. Close stream when no more rows; ensure cancellation drops DB cursor. | ||
|
|
||
| ### Database Access Pattern | ||
| - Option A (simple): repeated `SELECT ... ORDER BY id LIMIT $chunk OFFSET $offset` until fewer than chunk results. |
There was a problem hiding this comment.
Missing article: should be 'until fewer than chunk_size results' or 'until fewer results than chunk_size'.
| - Option A (simple): repeated `SELECT ... ORDER BY id LIMIT $chunk OFFSET $offset` until fewer than chunk results. | |
| - Option A (simple): repeated `SELECT ... ORDER BY id LIMIT $chunk OFFSET $offset` until fewer results than chunk. |
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable additions, including negative path tests for authentication, an integrity test for family creation, and a design document for streaming exports. The tests are well-structured and enhance the robustness of the authentication flow. The design document for streaming is comprehensive and provides a solid plan for improving performance on large exports. My main feedback is regarding the new documentation for the superadmin password in README.md. There appears to be a contradiction between the password mentioned in the new section (SuperAdmin@123) and the one set by the latest database migration and mentioned elsewhere in the README.md (admin123). This could cause confusion for developers. I've left a specific comment with details.
README.md
Outdated
| ### 超级管理员默认密码说明 | ||
|
|
||
| 迁移脚本会为内置超级管理员（`superadmin@jive.money`）设置一个固定 Argon2 哈希，对应初始密码：`SuperAdmin@123`。 | ||
|
|
||
| 注意事项： | ||
| - 如在本地手动用工具（例如 `cargo run --bin hash_password`）更新了该账号密码，再次重建 / 重新应用迁移（或使用全新数据库）时会回退到默认密码。 | ||
| - CI / 新开发环境请按默认密码尝试首次登录后立即修改。 | ||
| - 不要在仓库提交真实生产密码；如需变更默认策略，可新增迁移修改哈希值并在安全文档中注明。 | ||
|
|
||
| 本地若需要快速验证超级管理员登录，可： | ||
|
|
||
| ```bash | ||
| curl -s -X POST http://localhost:8012/api/v1/auth/login \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d '{"email":"superadmin@jive.money","password":"SuperAdmin@123"}' | ||
| ``` | ||
|
|
||
| 返回中包含 `token` 即表示成功；请在生产部署中更换为安全随机密码并限制暴露。 |
There was a problem hiding this comment.
This new section states that the default superadmin password is SuperAdmin@123. However, this seems to contradict other parts of the repository:
- The existing
README.mdsection "默认管理员账号（开发环境）" on line 269 states the password isadmin123. - The latest migration
016_fix_families_member_count_and_superadmin.sqlalso sets the password toadmin123and contains a comment confirming this.
This discrepancy can be very confusing for developers setting up the project. Please unify the password across all documentation and migrations. If the intended password is SuperAdmin@123 as per the PR description, the migration 016 and the older README.md section should be updated. If it's admin123, this new section should be corrected.
1 participant
Summary:
Rationale:
Closes gaps in negative-path coverage for auth and asserts data integrity on family creation. Provides onboarding clarity for superadmin login and prepares performance roadmap for large CSV exports.
Testing:
SQLX_OFFLINE=true cargo test --tests (24 pass) – tests + docs only, no prod code changes.