Skip to content

Conversation

@hmacr
Copy link

@hmacr hmacr commented Jan 25, 2026

Closes SER-541

Summary by CodeRabbit

  • Tests

    • Expanded coverage for team roles with project-based dimensions and added validation cases for verified/unverified and label role variants.
  • Refactor

    • Role validation now uses dedicated validator objects for clearer, unified identifier and dimension checks.
  • Bug Fixes

    • Increased allowed identifier/dimension length to 60 characters and updated validation messages accordingly.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 25, 2026

📝 Walkthrough

Walkthrough

Replaces inline Key/Label checks in the Roles validator with dedicated validator instances (identifierValidator, labelValidator, dimensionValidator with maxLength 60) and updates tests to cover team roles with project-scoped dimensions and the increased 60-char limit.

Changes

Cohort / File(s) Summary
Validator refactor
src/Database/Validator/Roles.php
Replaced local Key/Label validation branches with identifierValidator (chooses Key or Label), labelValidator, and dimensionValidator (maxLength 60); unified validation/error-message paths and removed direct $key/$label checks.
Role parsing/stringification tests
tests/unit/RoleTest.php
Added assertions for parsing and toString of team roles with project-scoped dimensions (team:123/project-456-owner, team:123/project-456) and corresponding Role constructions.
Roles validator tests
tests/unit/Validator/RolesTest.php
Added PHPUnit coverage for DIMENSION_VERIFIED/UNVERIFIED, team roles with custom IDs and project-scoped dimensions, and label-role cases; added ID helper import.
Permissions validator tests (length limit update)
tests/unit/Validator/PermissionsTest.php
Increased expected max-length from 36 to 60 for identifier/dimension-related assertions and added boundary cases to reflect the new 60-char limit.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I traded a tangle of checks for three,
identifier, label, dimension — carefree.
Projects, teams, and longer names now play,
Tests hop along and chase bugs away.
🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Dimension for project-specific role' accurately reflects the main change: adding support for project-based dimensions in team roles, as evidenced by test additions for project-456-owner and project-456 variants.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@hmacr
Copy link
Author

hmacr commented Jan 25, 2026

CodeQL and Pool tests seem to be failing in other recent PRs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants