fix: include specific IDs in owner validation errors#7116
fix: include specific IDs in owner validation errors#7116gagantrivedi wants to merge 1 commit intomainfrom
Conversation
Address review feedback from PR #7067: - Include user IDs in error message when users don't have project access - Include group IDs in error message when groups belong to wrong organisation This helps users understand exactly which owners/groups are invalid.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7116 +/- ##
=======================================
Coverage 98.34% 98.34%
=======================================
Files 1336 1336
Lines 50128 50132 +4
=======================================
+ Hits 49298 49302 +4
Misses 830 830 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
khvn26
left a comment
khvn26
left a comment
There was a problem hiding this comment.
Trying to understand which error messages would be more helpful to the user.
| if not user.has_project_permission(VIEW_PROJECT, project) | ||
| ] | ||
| if invalid_users: | ||
| invalid_user_ids = [user.id for user in invalid_users] |
There was a problem hiding this comment.
Returning email would be a data leak and returning uuid when you have supplied id is even more confusing?
| if group.organisation_id != project.organisation_id | ||
| ] | ||
| if invalid_groups: | ||
| invalid_group_ids = [group.id for group in invalid_groups] |
There was a problem hiding this comment.
Same as above — I don't think it makes sense to output different params than the input.
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Addresses 2 nit comments from @khvn26 on #7067:
validate_owners: error message now includes the specific user IDs that lack project access (e.g."Users with ids [3, 7] do not have access to this project.") instead of the generic"Some users do not have access to this project."validate_group_owners: error message now includes the specific group IDs that belong to the wrong organisation (e.g."Groups with ids [5] do not belong to this project's organisation.") instead of the generic"Some groups do not belong to this project's organisation."Both validators now collect all invalid entries before raising, so a single error reports every problematic ID at once.
How did you test this code?
test_create_feature__group_owner_from_different_org__returns_400andtest_create_feature__owner_without_project_access__returns_400) to assert the specific IDs appear in the error response.