Skip to content

Conversation

@luisina-santos
Copy link
Contributor

@luisina-santos luisina-santos commented Dec 22, 2025

API Documentation > https://developers.onelogin.com/api-docs/1/privileges/list-privileges

Sample response:

[
    {
        "id": "2c963197-bee2-4607-abc0-4786f1bfa55a",
        "name": "User Administrator",
        "description": "Can administer users",
        "privilege": {
            "Version": "2018-05-18",
            "Statement": [
                {
                    "Effect": "Allow",
                    "Action": [
                        "users:Delete",
                        "users:ForceLogout",
                        "users:ResetPassword",
                        "users:Unlock",
                        "users:Get"
                    ],
                    "Scope": [
                        "*"
                    ]
                }
            ]
        }
    }
]

Summary by CodeRabbit

  • Updates
    • Account privileges now use string-based identifiers instead of numeric IDs for improved consistency.
  • Bug Fixes
    • Error messages shown when listing privileges have been corrected to display identifier values properly.

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

@luisina-santos luisina-santos requested a review from a team December 22, 2025 15:44
@coderabbitai
Copy link

coderabbitai bot commented Dec 22, 2025

Walkthrough

Replaced the embedded BaseResource in AccountPrivilege with an explicit public Id field of type string, and adjusted error formatting in privilege creation to treat the privilege ID as a string; no other public fields were changed.

Changes

Cohort / File(s) Summary
AccountPrivilege struct update
pkg/onelogin/models.go
Removed anonymous BaseResource embedding and added Id string (json:"id"). Other public fields (Name, Description, Privilege) unchanged.
Privilege error formatting
pkg/connector/privilege.go
Changed error formatting to use %s for privilege.Id (string) when reporting creation failures during listing. No control flow changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring attention:
    • Search for usages of AccountPrivilege.Id expecting an integer and update callers or conversions.
    • Verify JSON (un)marshalling and any DB/storage interactions expecting numeric IDs.
    • Confirm the updated error formatting matches the new Id type and logging conventions.

Poem

A rabbit nibbling code at night, 🐇
IDs now quoted, crisp and bright,
No more embedded shadows deep—
Strings hop forward, numbers sleep. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: fixing the privilege type mapping from int to string for the Id field to align with the OneLogin API documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 luisinasantos/fix-privileges-sync

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 718e470 and 5f6b2ba.

📒 Files selected for processing (1)
  • pkg/connector/privilege.go
🔇 Additional comments (1)
pkg/connector/privilege.go (1)

70-70: Format specifier correctly updated for string-typed ID.

The change from %s is appropriate since AccountPrivilege.Id is defined as a string type, consistent with OneLogin's API which returns privilege IDs as strings. All usages of privilege.Id in the file align with this string type.


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

@luisina-santos luisina-santos merged commit 75639b7 into main Dec 23, 2025
4 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.

4 participants