Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdates install-time user migration to use ChangesApp-agnostic migration implementation
Keycloak realm test fixtures
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pas/plugins/kimug/utils.py`:
- Line 246: The Keycloak admin fetch in the kimug utility is missing a timeout
and can hang the migration indefinitely. Update the `requests.get` call in
`utils.py` to include the same bounded timeout pattern used by the other
Keycloak requests in this module, keeping the fix localized to the admin-API
lookup flow.
- Around line 211-214: The `APP_MIGRATION_CONFIG` entry for `iA.Smartweb` is
missing the required `imio` realm, which breaks the expected migration behavior.
Update the `iA.Smartweb` config in `APP_MIGRATION_CONFIG` so `extra_realms`
includes `imio` while keeping `clean_authentic` unchanged, and ensure any logic
that reads this config (such as `get_app_migration_config` and
`get_keycloak_users`) will now fetch that realm for `iA.Smartweb`.
- Around line 247-254: The Keycloak user fetch logic in the relevant users
helper is treating a valid 200 response with an empty list as an error because
it also checks response.json() before returning. Update the branch in the users
retrieval function so it returns successfully on HTTP 200 regardless of whether
the payload is empty, and only use the raise_on_error path for non-200
responses; keep the existing logger.error and exception behavior for true
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d948ff9f-ec6f-4bf9-9f22-9821ae33accf
📒 Files selected for processing (10)
news/app-agnostic-migration.feature.mdsrc/pas/plugins/kimug/setuphandlers/__init__.pysrc/pas/plugins/kimug/utils.pytests/keycloak/import/realm-municipality1.jsontests/keycloak/import/realm-municipality2.jsontests/keycloak/import/realm-municipality3.jsontests/keycloak/import/realm-municipality4.jsontests/keycloak/import/realm-municipality5.jsontests/keycloak/import/realm-plone.jsontests/utils/test_utils.py
Summary by CodeRabbit
New Features
application_id, fetching users from the app’s configured Keycloak realms and applying the matching migration settings.Bug Fixes
authenticcleanup runs only when enabled by the selected profile.Tests
Documentation