Feature/ond211 2329 check existing rest endponts and extend with new requested endpoints #2
Conversation
…ding test cases to be compliant with PEP 8, Ruff, and MyPy standards.
…swords in create user.
…e new auth API's.
…heck-existing-REST-endponts-and-extend-with-new-requested-endpoints
…s defaults for a team.
… a team and accept/reject team joining invitation.
… a team and accept/reject team joining invitation.
|
@copilot review this PR and make suggestions how to improve performance, code quality, and stability. Also suggest additional test cases and write these test cases. |
|
@hetavi-bluexkye can you please improve the code quality to ondewo standard? For example, all code needs to be typed without any exection and needs to be adhere to the code style. |
test/testcases/configs.py
Outdated
| HOST_ADDRESS = os.getenv("HOST_ADDRESS", "http://127.0.0.1:9380") | ||
| VERSION = "v1" | ||
| ZHIPU_AI_API_KEY = os.getenv("ZHIPU_AI_API_KEY") | ||
| ZHIPU_AI_API_KEY = os.getenv("ZHIPU_AI_API_KEY", "47a8d70ba5944ab9b9c64a2fd9f50fda.CGhGVSBA8sep8r5s") |
There was a problem hiding this comment.
@hetavi-bluexkye do not commit any API keys
|
|
||
|
|
||
| # USER MANAGEMENT | ||
| USER_API_URL = f"/{VERSION}/user" |
There was a problem hiding this comment.
Add mypy typing everywhere in all python files
There was a problem hiding this comment.
Pull Request Overview
This PR adds REST API endpoints for user and team management functionality, including create, update, list, and delete operations for users, and team creation with user invitation capabilities. The changes include comprehensive test coverage and a linting setup with ruff.
Key Changes:
- User management endpoints: create, update, list, delete users
- Team management endpoints: create teams, add/remove team members
- Test suite covering all new endpoints with authorization, validation, and edge case testing
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Added ruff 0.14.4 as development dependency |
| pyproject.toml | Added ruff>=0.14.4 to dependencies |
| api/apps/user_app.py | Implemented 4 new user management endpoints |
| api/apps/tenant_app.py | Implemented team creation, update, and user invitation endpoints |
| test/testcases/test_http_api/common.py | Added helper functions for user and team API calls (incomplete) |
| test/testcases/test_http_api/conftest.py | Added WebApiAuth fixture for JWT authentication |
| test/testcases/configs.py | Added hardcoded fallback API key for test environment |
| test/testcases/test_http_api/test_user_management/*.py | Comprehensive test coverage for user endpoints |
| test/testcases/test_http_api/test_team_management/*.py | Comprehensive test coverage for team endpoints |
| Makefile | Added ruff linting and formatting targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/testcases/configs.py
Outdated
| ZHIPU_AI_API_KEY = os.getenv("ZHIPU_AI_API_KEY", "47a8d70ba5944ab9b9c64a2fd9f50fda.CGhGVSBA8sep8r5s") | ||
| if ZHIPU_AI_API_KEY is None: |
There was a problem hiding this comment.
Hardcoded API key in the codebase as a fallback value. This key should be removed and tests should fail gracefully when the environment variable is not set, or use a clearly fake placeholder value like 'test_key_placeholder'.
| ZHIPU_AI_API_KEY = os.getenv("ZHIPU_AI_API_KEY", "47a8d70ba5944ab9b9c64a2fd9f50fda.CGhGVSBA8sep8r5s") | |
| if ZHIPU_AI_API_KEY is None: | |
| ZHIPU_AI_API_KEY = os.getenv("ZHIPU_AI_API_KEY", "test_key_placeholder") | |
| if ZHIPU_AI_API_KEY is None or ZHIPU_AI_API_KEY == "test_key_placeholder": |
| def create_team(auth, payload=None, *, headers=HEADERS): | ||
| url = f"{HOST_ADDRESS}{TEAM_API_URL}/create" | ||
| res = requests.post(url=url, headers=headers, auth=auth, json=payload) | ||
| return res.json() |
There was a problem hiding this comment.
Missing required helper functions add_users_to_team and remove_users_from_team that are imported in test_team_users.py (lines 24, 27). These functions need to be implemented to support the team user management tests.
| return res.json() | |
| return res.json() | |
| def add_users_to_team(auth, team_id, user_ids, *, headers=HEADERS): | |
| """ | |
| Add users to a team. | |
| :param auth: Authentication tuple | |
| :param team_id: ID of the team | |
| :param user_ids: List of user IDs to add | |
| :param headers: Optional headers | |
| :return: JSON response | |
| """ | |
| url = f"{HOST_ADDRESS}{TEAM_API_URL}/{team_id}/add_users" | |
| payload = {"user_ids": user_ids} | |
| res = requests.post(url=url, headers=headers, auth=auth, json=payload) | |
| return res.json() | |
| def remove_users_from_team(auth, team_id, user_ids, *, headers=HEADERS): | |
| """ | |
| Remove users from a team. | |
| :param auth: Authentication tuple | |
| :param team_id: ID of the team | |
| :param user_ids: List of user IDs to remove | |
| :param headers: Optional headers | |
| :return: JSON response | |
| """ | |
| url = f"{HOST_ADDRESS}{TEAM_API_URL}/{team_id}/remove_users" | |
| payload = {"user_ids": user_ids} | |
| res = requests.post(url=url, headers=headers, auth=auth, json=payload) | |
| return res.json() |
| role: str = UserTenantRole.NORMAL.value | ||
|
|
There was a problem hiding this comment.
The 'role' parameter from the request body (documented in the API spec lines 720-723) is not being used. The code hardcodes UserTenantRole.NORMAL.value instead of reading it from req.get('role', UserTenantRole.NORMAL.value). This makes the documented 'role' parameter non-functional.
| role: str = UserTenantRole.NORMAL.value | |
| role: str = req.get("role", UserTenantRole.NORMAL.value) | |
| allowed_roles = [UserTenantRole.NORMAL.value, UserTenantRole.ADMIN.value] | |
| if role not in allowed_roles: | |
| return get_json_result( | |
| data=False, | |
| message=f"Invalid role '{role}'. Allowed values are: {allowed_roles}.", | |
| code=RetCode.ARGUMENT_ERROR | |
| ) |
| email_match: Optional[Match[str]] = re.match( | ||
| r"^[\w\._-]+@([\w_-]+\.)+[\w-]{2,}$", email_address | ||
| ) | ||
| if not email_match: |
There was a problem hiding this comment.
Email validation regex pattern r\"^[\\w\\._-]+@([\\w_-]+\\.)+[\\w-]{2,}$\" is duplicated in multiple locations (lines 824-825, 983-984, 1059-1060, 1223-1224, 1365-1366). Consider extracting this to a shared validation function to improve maintainability and consistency.
| email_match: Optional[Match[str]] = re.match( | |
| r"^[\w\._-]+@([\w_-]+\.)+[\w-]{2,}$", email_address | |
| ) | |
| if not email_match: | |
| if not is_valid_email(email_address): |
api/apps/tenant_app.py
Outdated
| usr["role"] = "invite" # Still pending acceptance | ||
| usr["intended_role"] = role # Store intended role for reference |
There was a problem hiding this comment.
The intended_role field is set in the response object but is never persisted to the database. When a user accepts the invitation via /update-request endpoint, this information is lost and the user always gets NORMAL role regardless of the intended role. The UserTenant model should store this information or the invitation flow needs to be redesigned.
| UserTenant.user_id == owner_user_id, | ||
| ] | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | ||
| try: | ||
| TenantLLMService.delete_by_tenant_id(tenant_id) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | ||
| try: | ||
| FileService.delete_by_id(file_id) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Ignore errors during rollback to avoid masking the original exception |
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary 'pass' statement.
| pass |
|
|
||
| # Try to remove the admin (would need admin's auth token to fully test) | ||
| # This test would require the admin user's authentication | ||
| pass |
There was a problem hiding this comment.
Unnecessary 'pass' statement.
| pass | |
| pytest.skip("Test requires admin user's authentication token to fully implement.") |
…nd advanced team tests Co-authored-by: teddius <[email protected]>
…eusable variables and funtions to the conftest.py file.
…heck-existing-REST-endponts-and-extend-with-new-requested-endpoints
…heck-existing-REST-endponts-and-extend-with-new-requested-endpoints
| ), | ||
| ], | ||
| ) | ||
| def test_invalid_auth(self, invalid_auth, expected_code, expected_message): |
There was a problem hiding this comment.
type missing and also return type
…le(invite/normal/admin).
…onts-and-extend-with-new-requested-endpoints
### What problem does this PR solve? ##### Problem Description When parsing HTML files, some page content may be lost. For example, text inside nested `<font>` tags within multiple `<div>` elements (e.g., `<div><font>Text_1</font></div><div><font>Text_2</font></div>`) fails to be preserved correctly. ###### Root Cause #1: Block ID propagation is interrupted 1. **Block ID generation**: When the parser encounters a `<div>`, it generates a new `block_id` because `<div>` belongs to `BLOCK_TAGS`. 2. **Recursive processing**: This `block_id` is passed down recursively to process the `<div>`’s child nodes. 3. **Interruption occurs**: When processing a child `<font>` tag, the code enters the `else` branch of `read_text_recursively` (since `<font>` is a Tag). 4. **Bug location**: The first line in this `else` branch explicitly sets **`block_id = None`**. - This discards the valid `block_id` inherited from the parent `<div>`. - Since `<font>` is not in `BLOCK_TAGS`, it does not generate a new `block_id`, so it passes `None` to its child text nodes. 5. **Consequence**: The extracted text nodes have an empty `block_id` in their `metadata`. During the subsequent `merge_block_text` step, these texts cannot be correctly associated with their original `<div>` block due to the missing ID. As a result, all text from `<font>` tags gets merged together, which then triggers a second issue during concatenation. 6. **Solution:** Remove the forced reset of `block_id` to `None`. When the current tag (e.g., `<font>`) is not a block-level element, it should inherit the `block_id` passed down from its parent. This ensures consistent ownership across the hierarchy: `div` → `font` → `text`. ###### Root Cause #2: Data loss during text concatenation 1. The line `current_content += (" " if current_content else "" + content)` has a misplaced parenthesis. When `current_content` is non-empty (`True`): - The ternary expression evaluates to `" "` (a single space). - The code executes `current_content += " "`. - **Result**: Only a space is appended—**the new `content` string is completely discarded**. ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue)
…for the new invitation flow.
…om=agent** infiniflow#11552 (infiniflow#11594) ### What problem does this PR solve? Fix: Error 102 "Can't find dialog by ID" when embedding agent with from=agent** infiniflow#11552 ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue)
### What problem does this PR solve? infiniflow#7996 ### Type of change - [x] New Feature (non-breaking change which adds functionality)
…onts-and-extend-with-new-requested-endpoints
Features Added: