Skip to content

Conversation

@nevil-mathew
Copy link
Collaborator

@nevil-mathew nevil-mathew commented Oct 28, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Role-based feature filtering now merges default and organization-specific mappings so users see the full set of features their role entitles them to, preventing missing features when org-specific mappings are incomplete.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

The list method in src/services/organization-feature.js now builds separate maps for default-organization and current-organization feature codes and then iterates roles to form a union of features from both maps, replacing the previous per-role fallback logic and changing iteration from Array.forEach to for...of.

Changes

Cohort / File(s) Summary
Feature access union
src/services/organization-feature.js
Reworked list to collect default and current organization feature-code mappings into separate maps, then iterate role titles with a for...of loop to union features from both maps for each role instead of choosing one source per role. Filtering, empty-map handling, and response assembly remain.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Service as org-feature.service
    participant Maps as FeatureMaps
    participant Filter as FeatureFilter
    participant DB as DataStore

    Client->>Service: list(request with roles, org)
    Service->>DB: fetch default-org mappings
    Service->>DB: fetch current-org mappings
    DB-->>Service: default mappings
    DB-->>Service: current mappings
    Service->>Maps: build defaultMap & currentMap
    Maps-->>Service: maps ready
    Service->>Service: for...of over roleTitles
    Note right of Service: For each role, add features from both maps (union)
    Service->>Filter: apply filtering & remove empties
    Filter-->>Service: filtered features
    Service-->>Client: return feature list
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to src/services/organization-feature.js for:
    • Correctness of union logic (no unintended permission expansion).
    • Edge cases when one or both maps are empty.
    • Tests covering each role and combinations of default/current mappings.

Poem

🐇 I hopped through maps both near and far,
Default and current now join the star,
For-of steps counted, features combine,
A rabbit's small cheer for unioned design,
Sniffing out bugs, then back to the vine.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "refactor(organization): optimize feature access logic for role mappings" is clearly related to the main change in the pull request. The title specifically identifies the component being changed (organization), the type of change (refactor), and the focus area (feature access logic for role mappings). The raw summary confirms this is exactly what's being modified—the list function's role-based feature filtering logic. The title is concise, uses appropriate commit convention formatting, and is specific enough that a teammate scanning the history would understand the pull request concerns refactoring how organization features are accessed based on role mappings. While the title doesn't specify the technical details of shifting from a fallback approach to a union approach, it adequately conveys the scope and intent of the change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch org-list-fix

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c72510a and 3150b66.

📒 Files selected for processing (1)
  • src/services/organization-feature.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**

⚙️ CodeRabbit configuration file

This is core business logic. Please check for correctness, efficiency, and potential edge cases.

Files:

  • src/services/organization-feature.js
🔇 Additional comments (2)
src/services/organization-feature.js (2)

335-335: LGTM! Comment now accurately reflects the implementation.

The comment has been updated to correctly describe the union semantics, addressing the concern from the previous review.


338-343: LGTM! Union logic is correctly implemented.

The implementation correctly unions features from both default and current organization role mappings:

  • Optional chaining safely handles roles with no mappings in one or both organizations
  • Using a Set ensures deduplication when the same feature code exists in both mappings
  • The for...of loop is clean and efficient

The logic properly handles all edge cases.


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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c05e2df and c72510a.

📒 Files selected for processing (1)
  • src/services/organization-feature.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**

⚙️ CodeRabbit configuration file

This is core business logic. Please check for correctness, efficiency, and potential edge cases.

Files:

  • src/services/organization-feature.js
🧬 Code graph analysis (1)
src/services/organization-feature.js (1)
src/database/migrations/20250707122553-add-new-columns-to-features-and-org-features-tables.js (1)
  • code (27-27)

@nevil-mathew nevil-mathew merged commit 662bf35 into develop Oct 28, 2025
1 of 2 checks passed
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.

2 participants