fix(api): register flow owner_id + schema; add route e2e for register(+enhanced) #52
Conversation
… register and enhanced register; refresh SQLx cache
|
approve |
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! 此拉取请求旨在完善用户注册流程，确保新用户在注册时能够正确地与家庭 (family) 和默认账本 (ledger) 关联起来。它解决了数据库写入的准确性问题，更新了用户数据插入的模式，并引入了全面的端到端测试来验证注册功能的健壮性。 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
Fixes authentication registration flow by ensuring proper database schema alignment, adding owner_id to families, and providing comprehensive route-level e2e testing for both standard and enhanced registration endpoints.
- Corrected families table to include owner_id and ledgers to use created_by with is_default flags
- Updated user creation to match actual schema with name/full_name/is_active/email_verified fields
- Added comprehensive e2e tests validating full registration flow including CSV export functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jive-api/tests/integration/mod.rs | Added module declaration for new e2e test files |
| jive-api/tests/integration/auth_register_route_e2e_test.rs | New e2e test for standard registration route with family/ledger validation |
| jive-api/tests/integration/auth_register_enhanced_route_e2e_test.rs | New e2e test for enhanced registration route with CSV export verification |
| jive-api/src/services/family_service.rs | Updated family creation to include owner_id and ledger creation with created_by |
| jive-api/src/main.rs | Fixed route handler mapping from register_with_family to register |
| jive-api/src/handlers/enhanced_profile.rs | Added error handling and logging for family creation |
| jive-api/src/handlers/auth.rs | Restructured user/family creation with proper schema alignment and owner_id |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // 先创建用户（避免 families.owner_id 外键约束失败） | ||
| tracing::info!(target: "auth_register", user_id = %user_id, family_id = %family_id, email = %final_email, "Creating user then family with owner_id"); |
There was a problem hiding this comment.
[nitpick] The Chinese comment should be consistent with the codebase language. Consider using English: '// Create user first (to avoid families.owner_id foreign key constraint failure)'
Copilot uses AI. Check for mistakes.
| .await | ||
| .map_err(|e| ApiError::DatabaseError(e.to_string()))?; | ||
|
|
||
| // 再创建家庭（带 owner_id） |
There was a problem hiding this comment.
[nitpick] The Chinese comment should be consistent with the codebase language. Consider using English: '// Then create family (with owner_id)'
| // 再创建家庭（带 owner_id） | |
| // Then create family (with owner_id) |
Copilot uses AI. Check for mistakes.
| .map_err(|e| ApiError::DatabaseError(e.to_string()))?; | ||
|
|
||
| // 创建默认账本 | ||
| // 创建默认账本（标记 is_default，记录创建者） |
There was a problem hiding this comment.
[nitpick] The Chinese comment should be consistent with the codebase language. Consider using English: '// Create default ledger (mark as is_default, record creator)'
| // 创建默认账本（标记 is_default，记录创建者） | |
| // Create default ledger (mark as is_default, record creator) |
Copilot uses AI. Check for mistakes.
| .await | ||
| .map_err(|e| ApiError::DatabaseError(e.to_string()))?; | ||
|
|
||
| // 绑定用户的当前家庭并提交事务 |
There was a problem hiding this comment.
[nitpick] The Chinese comment should be consistent with the codebase language. Consider using English: '// Bind user's current family and commit transaction'
| // 绑定用户的当前家庭并提交事务 | |
| // Bind user's current family and commit transaction |
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 is a solid improvement, correctly fixing the owner_id assignment in the registration flow and aligning database schemas. The addition of comprehensive end-to-end tests for both registration paths is a major plus, greatly increasing the reliability of these critical features. My review includes a few suggestions to improve test hygiene by ensuring proper cleanup of test data and to enhance code clarity and idiomatic error handling in one of the handlers.
| let family = match family_service.create_family(user_id, family_request).await { | ||
| Ok(f) => f, | ||
| Err(e) => { | ||
| tracing::error!(target: "enhanced_register", error=?e, user_id=%user_id, "create_family failed"); | ||
| return Err(ApiError::InternalServerError); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The error handling for family_service.create_family has been improved by adding logging, which is great. However, the match statement with an early return can be written more idiomatically using Result::map_err. This makes the code more concise and follows a more common pattern for error handling in Rust, improving maintainability.
| let family = match family_service.create_family(user_id, family_request).await { | |
| Ok(f) => f, | |
| Err(e) => { | |
| tracing::error!(target: "enhanced_register", error=?e, user_id=%user_id, "create_family failed"); | |
| return Err(ApiError::InternalServerError); | |
| } | |
| }; | |
| let family = family_service | |
| .create_family(user_id, family_request) | |
| .await | |
| .map_err(|e| { | |
| tracing::error!(target: "enhanced_register", error=?e, user_id=%user_id, "create_family failed"); | |
| ApiError::InternalServerError | |
| })?; |
| let resp2 = app.clone().oneshot(req2).await.unwrap(); | ||
| assert_eq!(resp2.status(), StatusCode::OK); | ||
| let body_bytes = hyper::body::to_bytes(resp2.into_body()).await.unwrap(); | ||
| assert!(body_bytes.starts_with(b"Date,Description"), "CSV header missing or incorrect"); |
There was a problem hiding this comment.
This test creates a user but doesn't clean it up afterwards. This can pollute the test database and cause other tests to fail on subsequent runs. To ensure test isolation and reliability, the created user should be deleted at the end of the test, similar to how it's done in auth_register_route_e2e_test.rs.
assert!(body_bytes.starts_with(b"Date,Description"), "CSV header missing or incorrect");
// Cleanup user to prevent polluting the test database
let user_id_val = v.pointer("/data/user_id").expect("user_id not in response");
let user_id: Uuid = serde_json::from_value(user_id_val.clone()).expect("user_id is not a valid UUID");
sqlx::query("DELETE FROM users WHERE id = $1")
.bind(user_id)
.execute(&pool)
.await
.expect("Failed to clean up user after test");| let _ = sqlx::query("DELETE FROM users WHERE id = $1") | ||
| .bind(user_id) | ||
| .execute(&pool) | ||
| .await; |
There was a problem hiding this comment.
The result of the database query for cleaning up the test user is not handled. This could lead to silent failures in test cleanup, causing subsequent test runs to fail due to data conflicts. It's better to handle the Result explicitly, for example by using .expect(), to ensure that cleanup failures are immediately visible.
sqlx::query("DELETE FROM users WHERE id = $1")
.bind(user_id)
.execute(&pool)
.await
.expect("Failed to clean up test user");1 participant
Local validation
Risk/Notes