-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: add intermediate page for project-language upload #17534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| {% if user_can_upload_translation %} | ||
| <li> | ||
| <a class="dropdown-item" data-bs-target="#upload" data-bs-toggle="tab">{% translate "Upload translation" %}</a> | ||
| </li> | ||
| {% endif %} |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new upload tab functionality for project-language pages lacks test coverage. Consider adding a test to verify that:
- The upload tab appears when a user has the appropriate permissions
- The upload tab is hidden when a user lacks permissions
- The informational message is displayed correctly
- The permission check doesn't cause runtime errors with ProjectLanguage objects
This doesn't provide upload, but tells users where to perform it. Temporary solution util WeblateOrg#4283 is implemented. Fixes WeblateOrg#15695
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| return check_can_edit(user, permission, obj) and ( | ||
| # Normal upload | ||
| check_edit_approved(user, "unit.edit", translation) | ||
| check_edit_approved(user, "unit.edit", obj) | ||
| # Suggestion upload | ||
| or check_suggestion_add(user, "suggestion.add", translation) | ||
| or check_suggestion_add(user, "suggestion.add", obj) | ||
| # Add upload | ||
| or check_suggestion_add(user, "unit.add", translation) | ||
| or check_suggestion_add(user, "unit.add", obj) | ||
| # Source upload | ||
| or translation.is_source | ||
| or (isinstance(obj, Translation) and obj.is_source) | ||
| ) |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permission checks for check_upload now accept ProjectLanguage, but the function doesn't properly handle all the permission validation logic for ProjectLanguage objects. Specifically, when obj is a ProjectLanguage, the function proceeds directly to line 510 where it calls check_can_edit(user, permission, obj) and then checks various upload permissions.
However, the checks at lines 512-518 call functions like check_edit_approved, check_suggestion_add, etc., which expect either a Translation or a ProjectLanguage. While check_suggestion_add has been updated to support ProjectLanguage, these permission checks are designed to work on individual translations, not on project-language combinations. This could lead to confusing or incorrect permission results.
Consider adding explicit handling for ProjectLanguage in the check_upload function that either:
- Returns early with an appropriate message explaining that actual upload is not supported at this level (since this is meant to be an informational page)
- Implements proper permission logic that makes sense for a project-language context
| @register_perm("upload.perform") | ||
| def check_upload( | ||
| user: User, permission: str, translation: Translation | ||
| user: User, permission: str, obj: Translation | ProjectLanguage | ||
| ) -> bool | PermissionResult: | ||
| """ | ||
| Check whether user can perform any upload operation. | ||
| The actual check for the method is implemented in | ||
| weblate.trans.util.check_upload_method_permissions. | ||
| """ | ||
| # Source upload | ||
| if translation.is_source and not user.has_perm("source.edit", translation): | ||
| return Denied(gettext("Insufficient privileges for editing source strings.")) | ||
| # Bilingual source translations | ||
| if ( | ||
| translation.is_source | ||
| and not translation.is_template | ||
| and not issubclass(translation.component.file_format_cls, BilingualUpdateMixin) | ||
| ): | ||
| return Denied( | ||
| gettext("The file format does not support updating source strings.") | ||
| ) | ||
| if translation.component.is_glossary: | ||
| permission = "glossary.upload" | ||
| return check_can_edit(user, permission, translation) and ( | ||
| if isinstance(obj, Translation): | ||
| # Source upload | ||
| if obj.is_source and not user.has_perm("source.edit", obj): | ||
| return Denied( | ||
| gettext("Insufficient privileges for editing source strings.") | ||
| ) | ||
| # Bilingual source translations | ||
| if ( | ||
| obj.is_source | ||
| and not obj.is_template | ||
| and not issubclass(obj.component.file_format_cls, BilingualUpdateMixin) | ||
| ): | ||
| return Denied( | ||
| gettext("The file format does not support updating source strings.") | ||
| ) | ||
| if obj.component.is_glossary: | ||
| permission = "glossary.upload" | ||
|
|
||
| return check_can_edit(user, permission, obj) and ( | ||
| # Normal upload | ||
| check_edit_approved(user, "unit.edit", translation) | ||
| check_edit_approved(user, "unit.edit", obj) | ||
| # Suggestion upload | ||
| or check_suggestion_add(user, "suggestion.add", translation) | ||
| or check_suggestion_add(user, "suggestion.add", obj) | ||
| # Add upload | ||
| or check_suggestion_add(user, "unit.add", translation) | ||
| or check_suggestion_add(user, "unit.add", obj) | ||
| # Source upload | ||
| or translation.is_source | ||
| or (isinstance(obj, Translation) and obj.is_source) | ||
| ) |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to support ProjectLanguage in permission checks (check_upload, check_suggestion_add, and check_edit_approved) lack test coverage. Given that comprehensive permission tests exist in weblate/auth/tests/test_permissions.py, new tests should be added to verify that the permission checks work correctly when called with a ProjectLanguage object.
Consider adding tests that verify:
upload.performpermission works correctly withProjectLanguageobjectssuggestion.addpermission works correctly withProjectLanguageobjectsunit.editpermission works correctly withProjectLanguageobjects- The informational upload tab is displayed correctly when permissions are granted/denied
| return True | ||
|
|
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enable_suggestions property for ProjectLanguage is hardcoded to return True, which may not reflect the actual configuration of the components within the project-language combination. Some components within the project might have enable_suggestions set to False, or might have workflow settings that disable suggestions. This could lead to showing the upload tab to users even when suggestions are not actually enabled for any of the components in this project-language combination.
Consider checking if any translations within the project-language have suggestions enabled, or documenting why this always returns True is an acceptable approach for this use case.
| return True | |
| """Return whether suggestions are enabled for this project-language. | |
| Suggestions are considered enabled if any related component for this | |
| project-language has suggestions enabled, or if workflow settings | |
| explicitly enable them. This avoids advertising suggestions in the UI | |
| when none of the components actually support them. | |
| """ | |
| # Check per-component configuration via related translations. | |
| for translation in self.translation_set: | |
| component = getattr(translation, "component", None) | |
| if component is not None and getattr(component, "enable_suggestions", False): | |
| return True | |
| # Fallback to workflow settings, if they define suggestion behavior. | |
| workflow = self.workflow_settings | |
| if workflow is not None and hasattr(workflow, "enable_suggestions"): | |
| return bool(getattr(workflow, "enable_suggestions")) | |
| return False |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
weblate/auth/permissions.py:478
- The
check_suggestion_addfunction now acceptsProjectLanguagebut only checks for contributor agreement when the object is aTranslation. However, when aProjectLanguageis passed, the function will skip the agreement check entirely and proceed directly to the permission check. This could allow users to bypass contributor agreement requirements at the project-language level. Consider adding proper handling forProjectLanguageobjects or explicitly documenting why this check is intentionally skipped for this type.
if (
not user.is_bot
and isinstance(obj, Translation)
and obj.component.agreement
and not ContributorAgreement.objects.has_agreed(user, obj.component)
):
return False
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (90.90%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This doesn't provide upload, but tells users where to perform it. Temporary solution util #4283 is implemented.
Fixes #15695