Conversation
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
|
View your CI Pipeline Execution ↗ for commit c84584b
☁️ Nx Cloud last updated this comment at |
… attribute in local compose Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
WalkthroughThis change extends the LDAP schema and user information system to support a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Gates Failed
Prevent hotspot decline
(2 hotspots with Complex Method)
Enforce advisory code health rules
(2 files with Complex Method)
Gates Passed
4 Quality Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| service.test.ts | 1 rule in this hotspot | 8.21 → 8.01 | Suppress |
| index.ts | 1 rule in this hotspot | 7.73 → 7.71 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| service.test.ts | 1 advisory rule | 8.21 → 8.01 | Suppress |
| index.ts | 1 advisory rule | 7.73 → 7.71 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/tom-server/src/user-info-api/tests/service.test.ts (1)
73-88: workplaceFqdn wiring through useProfileTwake and mockUserDB is correct but increases helper complexityThe new
workplaceFqdnflag is correctly added to:
- the
useProfileTwaketype and defaults (defaulting tofalse), and- the
mockUserDBdestructuring and conditionaldefinePropertylogic,so that
profile.workplaceFqdnis only present when explicitly requested and usesMOCK_DATA.LDAP.workplaceFqdn, mirroring the pattern for other LDAP fields.Given
mockUserDBwas already flagged as complex, this extra branch pushes it a bit further; over time it might be worth extracting a small table‑driven helper for these field toggles to keep cyclomatic complexity in check, especially if more attributes get added.Also applies to: 106-107, 138-142
🧹 Nitpick comments (2)
packages/tom-server/src/user-info-api/services/index.ts (1)
147-162: Directory fetch and propagation ofworkplaceFqdnare consistentIncluding
'workplaceFqdn'in theuserDb.getfield list and assigning it toresult.workplaceFqdnwhendirectoryRow.workplaceFqdnis present fits the existing pattern for other directory‑backed attributes and respects the overall precedence chain (matrix → directory → settings → address‑book).One point to consider: unlike
mobile,workplaceFqdnis always returned when present, without visibility checks. If this value is considered sensitive in your domain, you may want to gate it behind the same visibility rules (or a dedicated setting); if it’s intentionally always public, then the current implementation is fine.Also applies to: 216-245
packages/tom-server/src/user-info-api/tests/service.test.ts (1)
517-565: New workplaceFqdn tests validate presence/absence behavior in the base scenarioBoth tests correctly set up Matrix (displayName+avatar) and UserDB profiles, then assert that:
workplaceFqdnis exposed when the LDAP mock includes it, and- the field is omitted when
workplaceFqdnis not provided,while keeping the expectations for other fields consistent with existing precedence rules. This gives good coverage for the core behavior; if you later extend workplaceFqdn to other data sources or visibility modes, you can mirror these tests in those contexts too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.compose/ldap/ldif/base_ldap_users.ldif(4 hunks).compose/ldap/ldif/config-20230322180123.ldif(1 hunks).compose/ldap/ldif/custom-schema.ldif(1 hunks)packages/tom-server/src/user-info-api/services/index.ts(3 hunks)packages/tom-server/src/user-info-api/tests/service.test.ts(5 hunks)packages/tom-server/src/user-info-api/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build And Test / test
- GitHub Check: update
🔇 Additional comments (5)
packages/tom-server/src/user-info-api/types.ts (1)
17-30: ExposeworkplaceFqdnonUserInformationlooks correctOptional
workplaceFqdn?: stringis a backward‑compatible extension and aligns with how the service now populates this field..compose/ldap/ldif/base_ldap_users.ldif (1)
19-38: LDAP entries correctly extended withworkplaceUser/workplaceFqdnAdding
objectClass: workplaceUseralongsideinetOrgPersonand populatingworkplaceFqdnfor selected users is consistent with the new AUXILIARY schema and should provide good coverage for the new field in tests and local deployments.Also applies to: 48-56, 146-155, 156-165, 198-219
.compose/ldap/ldif/config-20230322180123.ldif (1)
242-253: Custom schema entry forworkplaceFqdn/workplaceUseris coherentThe new
cn={4}customschema fragment definesworkplaceFqdnand the AUXILIARYworkplaceUserclass consistently with how they are used in user entries and incustom-schema.ldif. This should integrate cleanly with the existing cn=config setup..compose/ldap/ldif/custom-schema.ldif (1)
1-15: Standalone custom schema for workplace attributes looks goodThe custom schema LDIF cleanly defines
workplaceFqdnand the AUXILIARYworkplaceUserclass with matching OIDs to the config snapshot and is compatible with how user entries are authored.packages/tom-server/src/user-info-api/tests/service.test.ts (1)
42-43: MOCK_DATA.LDAP extension for mobile/workplaceFqdn is consistent with usageDefining
mobileandworkplaceFqdnonMOCK_DATA.LDAPmatches how later tests referenceMOCK_DATA.LDAP.mobileandMOCK_DATA.LDAP.workplaceFqdn, and keeps the mock payload self‑contained and realistic. No issues here.
Adds support for the
workplaceFqdnfield in the user-info API service, allowing retrieval of workplace FQDN information from the LDAP directory.Changes
Fixed bug in
packages/tom-server/src/user-info-api/services/index.ts:242workplaceFqdnin user information when available from UserDB/LDAPAdded tests in
packages/tom-server/src/user-info-api/tests/service.test.tsMOCK_DATA.LDAPto includeworkplaceFqdn: 'workplace.example.com'useProfileTwaketype to includeworkplaceFqdn: booleanmockUserDBfunction to handle the new fieldLocal Deployment Test Data
.compose/ldap/ldif/base_ldap_users.ldifworkplaceFqdnattribute to 7 test users:tardis.example.combadwolf.example.comjedi-temple.example.comrebel-alliance.example.comjedi-council.example.comacme-corp.example.comtechcorp.example.comSummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.