disable custom roles API & fix role locales #28
Conversation
|
Warning Rate limit exceeded@jokester has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces various modifications across multiple files, primarily focusing on enhancing error handling, refining type annotations, and improving logging messages. Key changes include updates to the Makefile to clarify dependencies, adjustments in API classes for better permission checks and error messages, and alterations in translation files to improve clarity. Additionally, test cases have been modified to reflect new functionality and error handling requirements. Overall, the changes aim to enhance the organization, clarity, and robustness of the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant FileService
participant RoleService
User->>API: Request file access
API->>FileService: Check permissions
FileService-->>API: Permission status
API-->>User: Return access result
User->>API: Create role
API->>RoleService: Validate role creation
RoleService-->>API: Role creation result
API-->>User: Return role creation status
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 79.75% 78.78% -0.98%
==========================================
Files 109 110 +1
Lines 6689 6740 +51
==========================================
- Hits 5335 5310 -25
- Misses 1354 1430 +76 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
Makefile (1)
50-50: LGTM! The dependency ensures correct translation update sequence.The addition of
babel-update-poas a dependency forbabel-update-mois correct, as it ensures that .mo files are only compiled after .po files are updated.Consider chaining
babel-translate-poin the dependency sequence to ensure translations are filled before compilation:-babel-update-mo: babel-update-po +babel-update-mo: babel-update-po babel-translate-poThis would enforce the complete translation pipeline: update → translate → compile.
app/apis/role.py (1)
Line range hint
77-106: Add localization support for role names.The implementation needs to be updated to align with the PR objectives:
- System roles should use Chinese names as identifiers
- Role names should be localized based on the API request locale
Consider adding locale handling to the role creation/editing logic:
def put(self, group: Union[Team, Project], role_id: str): if not self.current_user.can(group, group.permission_cls.CREATE_ROLE): raise NoPermissionError data = self.get_json( RoleSchema(), context={"current_user_role": self.current_user.get_role(group)}, ) + # Get locale from request headers or default to Chinese + locale = request.headers.get('Accept-Language', 'zh-CN') group.edit_role( id=role_id, - name=data["name"], + name=self._get_localized_name(data["name"], locale), level=data["level"], permissions=data["permissions"], intro=data["intro"], operator=self.current_user, )Similar changes should be applied to the role creation logic in the
postmethod.tests/__init__.py (1)
38-38: LGTM! Helpful addition for debugging locale-related test failures.Setting
maxDiff = Noneis a good improvement that will show complete diffs in test failure messages instead of truncating them. This will be particularly valuable when debugging failures related to role locales and Chinese name identifiers, which are key aspects of this PR.Consider documenting this change in the class docstring to help other developers understand why unlimited diff output is necessary for these test cases.
app/apis/file.py (1)
Line range hint
152-159: Consider implementing the watermark TODO for better access control.The TODO comment indicates a need to implement watermark protection for team users and prevent direct access to original images for non-team members. This is an important security consideration that should be tracked.
Would you like me to:
- Create a GitHub issue to track this watermark implementation requirement?
- Propose a detailed implementation plan for the watermark feature?
app/core/rbac.py (1)
137-137: Add type hints for system role data structure.The change from dict to list of dicts aligns with the PR objectives for system roles management. However, let's improve type clarity.
Add more specific type hints to document the expected structure:
- system_role_data: List[dict] = [] + system_role_data: List[dict[str, Any]] = [] # List[{"name": str, "permissions": List[int], "level": int, "system_code": str, "intro": str}]app/models/user.py (2)
Line range hint
73-73: Fix typo in BooleanField parameter name.There's a typo in the
bannedfield declaration wheredefultshould bedefault. This will prevent the default value from being properly set.Apply this fix:
- banned = BooleanField(defult=False, db_field="b") # 是否被封禁 + banned = BooleanField(default=False, db_field="b") # 是否被封禁
246-252: LGTM: Type hints improve code clarity.The addition of type hints to the
teamsmethod parameters is well-implemented. Consider also adding a type hint for theroleparameter for consistency.Consider adding a type hint for the
roleparameter:def teams( self, - role=None, + role: Optional[Union[str, list[str]]] = None, skip: Optional[int] = None, limit: Optional[int] = None, word: Optional[str] = None, ):app/models/file.py (1)
Line range hint
414-524: Consider future improvements to the move_to method.While the current changes are good, here are some suggestions for future improvements:
- Consider using transactions to ensure cache updates are atomic
- Optimize the multiple database operations by using bulk operations
- Add more detailed error messages for each failure case
tests/api/test_team_api.py (1)
118-120: LGTM! Consider adding explicit default team verification.The updated assertion correctly accounts for the default team in the count. However, consider adding an explicit test case to verify the default team's presence in the search results.
Add a test case like:
# Verify default team is included data = self.get("/v1/teams", query_string={"word": "default"}, token=token) self.assertErrorEqual(data) self.assertEqual(1, len(data.json)) self.assertTrue(data.json[0]["name"].startswith("default"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
Makefile(1 hunks)app/apis/file.py(1 hunks)app/apis/role.py(2 hunks)app/apis/urls.py(2 hunks)app/core/rbac.py(1 hunks)app/factory.py(2 hunks)app/models/file.py(1 hunks)app/models/project.py(7 hunks)app/models/team.py(6 hunks)app/models/user.py(2 hunks)app/translations/en/LC_MESSAGES/messages.po(7 hunks)app/translations/zh/LC_MESSAGES/messages.po(6 hunks)tests/__init__.py(1 hunks)tests/api/test_role_api.py(4 hunks)tests/api/test_team_api.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/apis/urls.py
- tests/api/test_role_api.py
🔇 Additional comments (19)
app/apis/role.py (1)
1-1: LGTM: Import changes are appropriate.
The new imports support the type annotations and follow Python's import style guidelines.
Also applies to: 7-8
app/factory.py (2)
120-120: LGTM: Improved log message clarity
The changes to logging messages enhance readability by using clear English descriptions.
Also applies to: 136-136
122-122: 🛠️ Refactor suggestion
Review needed: Team name and intro text changes
A few concerns regarding these changes:
- The PR objectives mention using Chinese names as identifiers, but the default team name has been changed to English. Should we maintain consistency with the Chinese naming convention?
- The intro text formatting could be improved to enhance readability.
- The reference to "站点管理-自动加入的团队 ID" in the intro text is hardcoded and could become outdated if the UI changes.
Let's verify the team name consistency across the codebase:
Consider these improvements:
def create_default_team(admin_user):
from app.models.team import Team, TeamRole
from app.models.site_setting import SiteSetting
if Team.objects().count() == 0:
logger.debug("creating default team")
team = Team.create(
- name="Default Team",
+ name="默认团队", # Keep Chinese name for consistency
creator=admin_user,
)
team.intro = """
- All new users will automatically join this team. This can be disabled by site administrator.
- 所有新用户会自动加入此团队，如不需要，站点管理员可以在"站点管理-自动加入的团队 ID"中删除此团队 ID。""".strip()
+ All new users will automatically join this team.
+ This can be disabled by site administrator in the site settings.
+
+ 所有新用户会自动加入此团队。
+ 如不需要，站点管理员可以在站点设置中禁用此功能。""".strip()Also applies to: 125-127
app/apis/file.py (1)
159-159: LGTM! Error message update improves clarity.
The error message change from a specific operation-based message to a more general access permission message is appropriate here, as it better reflects the actual permission check being performed (ProjectPermission.ACCESS).
Let's verify consistency with similar permission error messages:
✅ Verification successful
Error message is consistent with similar permission checks across the codebase
The verification shows that the updated error message "您没有此项目的访问权限" (You don't have access permission to this project) is consistently used in both file.py and project.py for general access permission checks. The only variation found is "您没有此项目的上传文件权限" which is specifically used for file upload permissions, making the distinction clear and appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar permission error messages to ensure consistency
# Expected: Find similar permission error messages using gettext for comparison
# Search for permission error messages
rg -A 1 'NoPermissionError.*gettext.*项目.*权限' --type py
Length of output: 600
app/models/team.py (3)
3-3: LGTM: Type import added correctly
The List import from typing module is properly added to support type annotations.
134-134: LGTM: Type annotation improves code clarity
The type annotation List[dict] accurately represents the structure of system_role_data.
136-136: 🛠️ Refactor suggestion
Verify the impact of hardcoded Chinese role names
The change from lazy_gettext to hardcoded Chinese strings aligns with the PR objective but has several implications:
- Removes i18n support at the model level
- May affect existing code expecting localized role names
- Could impact non-Chinese interfaces
Let's check for potential impacts:
Consider adding code comments to explain:
- The intentional use of Chinese identifiers
- How localization is handled at the API level
- The rationale behind this design decision
Example:
class TeamRole(RoleMixin, Document):
permission_cls = TeamPermission
group = ReferenceField("Team", db_field="g")
+ # NOTE: Role names are intentionally in Chinese as they serve as canonical identifiers.
+ # Localization is handled at the API level based on request locale.
system_role_data: List[dict] = [Also applies to: 168-168, 199-199, 220-220, 239-239
✅ Verification successful
Role names can be safely hardcoded in Chinese
Based on the verification results, the change to use hardcoded Chinese role names is safe and aligns with the system's design:
-
The role names are used as identifiers in the system, not display strings
-
User-facing messages are properly internationalized using gettext:
gettext("此用户是项目所在团队成员，已直接加入")gettext("加入成功，管理员继承自团队")- Other UI messages
-
The system has proper i18n infrastructure in place:
- Translations directory at
app/translations/ - Multiple translation-related files
- Locale handling in API layer
- Language model and API endpoints for language selection
- Translations directory at
The change is consistent with the codebase's architecture where:
- Role names serve as internal identifiers
- User-facing strings are handled through the translation system
- API layer manages localization based on request locale
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for direct usage of role names in code
echo "Checking for direct role name references..."
rg -l "创建人|管理员|资深成员|成员|见习成员" --type py
# Check for gettext usage with these role names
echo "Checking for gettext usage with role names..."
rg -l "gettext.*('creator'|'admin'|'senior'|'member'|'beginner')" --type py
Length of output: 1125
Script:
#!/bin/bash
# Let's examine the actual usage context of these role names in key files
echo "=== Checking role name usage in models ==="
rg -A 2 -B 2 "创建人|管理员|资深成员|成员|见习成员" app/models/team.py app/models/user.py
echo -e "\n=== Checking role name usage in APIs ==="
rg -A 2 -B 2 "创建人|管理员|资深成员|成员|见习成员" app/apis/
echo -e "\n=== Checking for any i18n/translation related code ==="
rg -l "gettext|i18n|translate|babel" --type py
echo -e "\n=== Checking for locale handling in API layer ==="
rg -A 2 -B 2 "locale|language|accept.*lang" app/apis/
Length of output: 12667
app/core/rbac.py (2)
Line range hint 178-183: LGTM: Role localization implementation is solid.
The implementation correctly handles system role localization through gettext integration, which aligns with the PR objective of localizing system role names based on API request locale.
137-137: Verify impact on system role initialization.
Let's ensure this change doesn't break existing system role initialization logic.
✅ Verification successful
No impact on system role initialization
The change is safe because:
- The empty
system_role_datalist inrbac.pyserves as a base class definition - Actual system role data is properly defined in the implementing classes:
app/models/team.pydefinessystem_role_datafor team rolesapp/models/project.pydefinessystem_role_datafor project roles
- The initialization logic in
init_system_roles()works correctly with these implementations, as evidenced by:- Usage in
factory.pyfor both TeamRole and ProjectRole initialization - Test cases in
test_role_model.pyandtest_project_model.pyvalidating the system role counts and initialization
- Usage in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any other files define system_role_data and verify their structure
# Search for system_role_data definitions
echo "Searching for system_role_data definitions..."
rg -l "system_role_data\s*=" --type py
# Search for system role initialization calls
echo "Searching for init_system_roles calls..."
rg -l "init_system_roles" --type py
# Search for potential dict access patterns that might break
echo "Searching for potential dict access patterns..."
rg "system_role_data\[" --type py
Length of output: 441
Script:
#!/bin/bash
# Let's examine the implementation details of init_system_roles and its usage
# Check init_system_roles implementation and usage in rbac.py
echo "=== Checking rbac.py ==="
rg -A 10 "init_system_roles" app/core/rbac.py
# Check how it's used in factory.py
echo "=== Checking factory.py ==="
rg -A 5 "init_system_roles" app/factory.py
# Look for any system role related constants or configurations
echo "=== Checking for system role configurations ==="
rg "system_role" --type py -A 3
Length of output: 10858
app/models/user.py (1)
4-4: LGTM: Import addition aligns with type annotation updates.
The addition of Optional from typing module is appropriate for the type hints being added to the teams method parameters.
app/translations/zh/LC_MESSAGES/messages.po (2)
1122-1250: LGTM: Team permission translations are clear and consistent.
The translations for team-related permissions and their descriptions are well-written and provide clear context to users about what each permission entails.
1262-1290: LGTM: User-related messages provide good feedback.
The translations for user join/invite scenarios are clear and provide appropriate feedback for different situations (e.g., direct join, pending approval, etc.).
app/translations/en/LC_MESSAGES/messages.po (4)
10-10: LGTM!
The POT-Creation-Date has been correctly updated.
54-54: LGTM!
The permission error messages are clear and consistent.
Also applies to: 62-62
1049-1049: LGTM!
The UI text changes from "Visit" to "View" improve consistency across the interface.
Also applies to: 1151-1151
1410-1439: Verify removal of role translations.
The commented-out role translations align with the PR objective to disable custom roles API. However, let's verify that these roles are not referenced elsewhere in the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Role translations can be safely removed
The commented-out role translations in messages.po are safe to remove as they are only used in test files and system code that defines the role types. The actual role system is still in place but the custom roles API is being disabled, which aligns with these translations being removed.
Key findings:
- References to roles like "Creator", "Administrator", etc. appear mainly in:
- Test files (
tests/) which don't affect production - System role definitions in models (
app/models/) where they are hardcoded - Core RBAC implementation (
app/core/rbac.py) handling system roles
- Test files (
- No user-facing code relies on these specific translations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed role translations
# Test: Search for role names in the codebase
echo "Checking for role references..."
rg -i "Creator|Administrator|Supervisor|Proofreader|Translator|Inpainter|Senior Member|Member|Trainee Member" \
--type py \
--glob '!app/translations/**'
Length of output: 56618
app/models/project.py (2)
143-143: LGTM: Type annotation improves code clarity.
The addition of List[dict] type annotation for system_role_data enhances code maintainability and IDE support.
Line range hint 145-274: Document the rationale for using Chinese role names as identifiers.
While using Chinese names as role identifiers aligns with the PR objectives, please consider:
- Add a code comment explaining why Chinese names are used as identifiers
- Verify that role names are still properly localized in the UI through other means
- Consider using English identifiers in the code while maintaining Chinese as the canonical form in the database
Let's check how these role names are used in the UI:
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Chinese role names are correctly used as system identifiers with proper UI localization
The codebase shows a well-structured localization system:
-
The Chinese role names in
project.pyare used as system identifiers, while UI text is properly localized through:- Flask-Babel integration (
flask_babel) - Translations stored in
app/translations/ - Proper use of
gettext/lazy_gettextfor UI strings
- Flask-Babel integration (
-
The role names are consistently used across:
- Permission checks
- Database operations
- API endpoints
- Test cases
No documentation is needed since this follows the established pattern in the codebase where system identifiers use Chinese while user-facing text is properly internationalized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UI components or API endpoints that might reference these role names
rg -l "创建人|管理员|监理|校对|翻译|嵌字|见习翻译" --type py
# Check for any gettext usage with these role names
rg "gettext.*[\"'](Creator|Admin|Coordinator|Proofreader|Translator|Picture Editor|Supporter)[\"']" --type py
Length of output: 1430
Script:
#!/bin/bash
# Let's check how these role names are actually used in the UI/API layer
rg -A 5 "创建人|管理员|监理|校对|翻译|嵌字|见习翻译" app/apis/ app/templates/ --type py
# Check for any translation/i18n related imports or setup
rg "from.*gettext|import.*gettext|i18n|babel|translation" --type py
# Check if there's any documentation about the localization strategy
fd -e md -e rst . | xargs rg -i "local.*|i18n|translat.*"
Length of output: 56707
app/models/file.py (1)
414-414: LGTM! The type annotation change improves flexibility.
The updated signature allows the move_to method to handle root-level moves by accepting None as a valid parent value. The implementation correctly handles this case by setting empty values for dir_sort_name and ancestors.
Let's verify the usage of this method to ensure all callers handle the new None case correctly:
✅ Verification successful
The move_to method's None handling is well-tested and correctly used
The verification shows that:
- The API endpoint in
app/apis/file.pycorrectly handles bothNoneand parent ID cases - Extensive test coverage in
tests/model/test_file_model.pyandtests/other/test_multi_target_cache.pydemonstrates:- Moving files/directories to root level (parent=None)
- Moving between directories
- Error cases (moving to subdirectories, same directory, duplicate names)
- Cache updates after moves
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for calls to move_to method to verify they handle None case
# Test: Look for method calls. Expect: Calls should handle None case appropriately
# Search for direct method calls
rg -A 3 '\.move_to\('
# Search for method definition to understand the full context
ast-grep --pattern 'def move_to($$$)'
Length of output: 6040
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Translations