MX-388: Add tenant branding endpoint for client applications - #184
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds tenant-scoped branding storage with a default blue color, supported-color validation, authenticated retrieval, and permission-protected updates through ChangesTenant branding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticatedClient
participant TenantBrandingApiResource
participant TenantBrandingService
participant TenantBrandingRepository
AuthenticatedClient->>TenantBrandingApiResource: Request tenant branding
TenantBrandingApiResource->>TenantBrandingService: Retrieve or update branding
TenantBrandingService->>TenantBrandingRepository: Read or save tenant branding
TenantBrandingRepository-->>TenantBrandingService: Branding data
TenantBrandingService-->>TenantBrandingApiResource: TenantBrandingData
TenantBrandingApiResource-->>AuthenticatedClient: JSON response
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/org/apache/fineract/branding/api/TenantBrandingApiResource.java (1)
61-106: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffSecurity enforcement uses
PlatformSecurityContextrather than@PreAuthorize.The coding guideline for
**/*.javacalls for method-level security via@PreAuthorize. This resource instead usescontext.authenticatedUser().validateHasPermissionTo(...), matching the established Fineract JAX-RS security pattern rather than Spring MVC's@PreAuthorize. Functionally, permission checks are enforced correctly for bothretrieveBranding()andupdateBranding(). Treat this as a codebase-wide convention rather than a defect specific to this file.As per coding guidelines, "All endpoints must be secured with appropriate permissions using
@PreAuthorizeannotations for method-level security."🤖 Prompt for 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. In `@src/main/java/org/apache/fineract/branding/api/TenantBrandingApiResource.java` around lines 61 - 106, Replace the manual PlatformSecurityContext permission checks in retrieveBranding() and updateBranding() with appropriate method-level `@PreAuthorize` annotations, preserving the existing authentication and UPDATE_PERMISSION requirements for each endpoint. Remove only the redundant in-method security enforcement while keeping branding retrieval and update behavior unchanged.Source: Coding guidelines
src/main/java/org/apache/fineract/branding/service/TenantBrandingService.java (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPackage placement deviates from the project's DDD structure guideline.
This service sits under
org.apache.fineract.branding.service. The project structure guideline forsrc/main/java/**expects services and orchestrators under anapplicationpackage. This is likely consistent with the rest of the codebase's module layout, so treat this as an optional, deferred consideration rather than something to change for this PR alone.As per path instructions, "Follow DDD-inspired modular structure" with services under the
applicationpackage.🤖 Prompt for 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. In `@src/main/java/org/apache/fineract/branding/service/TenantBrandingService.java` around lines 29 - 31, Defer package relocation for TenantBrandingService; no code change is required in this PR. Retain the current org.apache.fineract.branding.service placement and consider moving it under an application package in a separate structural change.Source: Path instructions
src/main/java/org/apache/fineract/branding/domain/TenantBranding.java (1)
27-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffJPA annotations in the
domainpackage deviate from the layered-architecture guideline.This entity is placed under
org.apache.fineract.branding.domainand usesjakarta.persistenceannotations directly. The domain-layer path instruction expects entities free of framework dependencies. This appears consistent with the existing pattern of JPA entities living underdomainpackages elsewhere in the codebase, so treat this as an optional, deferred cleanup rather than a blocking concern for this PR.As per path instructions, the domain layer should have "No dependencies on Spring, frameworks, or infrastructure."
🤖 Prompt for 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. In `@src/main/java/org/apache/fineract/branding/domain/TenantBranding.java` around lines 27 - 60, Defer this optional cleanup; no blocking change is required for TenantBranding. If addressing it later, move persistence-specific mapping out of the domain-layer TenantBranding class into the established infrastructure or persistence entity pattern, while preserving its tenant uniqueness, default color, identifiers, timestamps, and lifecycle behavior.Source: Path instructions
🤖 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/main/java/org/apache/fineract/branding/domain/TenantBranding.java`:
- Around line 27-31: Replace Lombok `@Data` on the TenantBranding entity with
`@Getter` and `@Setter` to avoid generating lifecycle-unstable equals() and
hashCode() from the generated id. If equality is required, define equals() and
hashCode() explicitly using the natural key tenantId.
In
`@src/main/java/org/apache/fineract/branding/service/TenantBrandingService.java`:
- Around line 50-56: Update retrieveCurrentTenantBranding() so an existing
branding row’s primaryColor is normalized before constructing
TenantBrandingData: retain it only when non-null and included in
SUPPORTED_PRIMARY_COLORS, otherwise use DEFAULT_PRIMARY_COLOR. Preserve the same
default for tenants with no row and ensure the returned primaryColor is never
null.
In
`@src/main/resources/db/changelog/tenant/module/selfservice/parts/063-add-tenant-branding.xml`:
- Line 27: Update the primary_color column definition in the tenant branding
changelog to enforce NOT NULL and use a computed default fallback, preserving
"blue" as the database-level default for inserted rows.
- Around line 24-34: Remove the redundant uniqueness declaration in the tenant
branding table definition: retain either the tenant_id column’s unique="true"
constraint or the addUniqueConstraint block named uk_tenant_branding, but not
both.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/branding/api/TenantBrandingApiResource.java`:
- Around line 61-106: Replace the manual PlatformSecurityContext permission
checks in retrieveBranding() and updateBranding() with appropriate method-level
`@PreAuthorize` annotations, preserving the existing authentication and
UPDATE_PERMISSION requirements for each endpoint. Remove only the redundant
in-method security enforcement while keeping branding retrieval and update
behavior unchanged.
In `@src/main/java/org/apache/fineract/branding/domain/TenantBranding.java`:
- Around line 27-60: Defer this optional cleanup; no blocking change is required
for TenantBranding. If addressing it later, move persistence-specific mapping
out of the domain-layer TenantBranding class into the established infrastructure
or persistence entity pattern, while preserving its tenant uniqueness, default
color, identifiers, timestamps, and lifecycle behavior.
In
`@src/main/java/org/apache/fineract/branding/service/TenantBrandingService.java`:
- Around line 29-31: Defer package relocation for TenantBrandingService; no code
change is required in this PR. Retain the current
org.apache.fineract.branding.service placement and consider moving it under an
application package in a separate structural change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5612f5a7-7b02-4331-97cf-74a1ef6e51f8
📒 Files selected for processing (10)
src/main/java/org/apache/fineract/branding/api/TenantBrandingApiResource.javasrc/main/java/org/apache/fineract/branding/data/TenantBrandingData.javasrc/main/java/org/apache/fineract/branding/domain/TenantBranding.javasrc/main/java/org/apache/fineract/branding/domain/TenantBrandingRepository.javasrc/main/java/org/apache/fineract/branding/service/TenantBrandingService.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/063-add-tenant-branding.xmlsrc/test/java/org/apache/fineract/branding/api/TenantBrandingApiResourceTest.javasrc/test/java/org/apache/fineract/branding/domain/TenantBrandingTest.javasrc/test/java/org/apache/fineract/branding/service/TenantBrandingServiceTest.java
feat: add tenant branding API
6101040 to
f12162f
Compare
|
@IOhacker This PR is ready for your review! |
2 participants
What
Adds a tenant scoped primary colour so a single Mifos web app deployment can brand each tenant, exposed as
GET/PUT /v1/branding.Jira: #MX-388
Why here rather than in Apache Fineract
This started as a global configuration PR against apache/fineract (apache/fineract#6212, FINERACT-2731). Maintainers declined it: Fineract is a client agnostic core banking backend and should not carry settings that exist to serve one specific frontend. This implementation keeps the data in the plugin's own table, so nothing in Apache Fineract changes.
Changes
063-add-tenant-branding.xmlcreatesm_selfservice_tenant_branding(
tenant_idunique,primary_color, timestamps), modelled on061-add-date-time-preferences.xml. Picked up by the existingSelfServiceLiquibaseConfig, so it runs per tenant with no new wiring.TenantBranding,TenantBrandingRepository,TenantBrandingService,TenantBrandingApiResource.UPDATE_CONFIGURATION.blue, so branding can neverstop a client from rendering.
Why the endpoint is not under
/v1/selfSelfServiceSecurityConfigurationuses.securityMatcher("/api/v1/self/**", "/v1/self/**")and authenticates self-service users only. Branding is read by staff facing clients, so the resource sits at/v1/branding, leaving it on Fineract's main security chain where ordinary platform credentials apply. Registration is unaffected:JerseyConfigregisters every@Pathbean regardless of path.Testing
19 unit tests across the service, resource and entity. Full suite passes (360 tests, 0 failures) with the JaCoCo coverage gate met. Coverage of the new code:
TenantBrandingApiResource100%,TenantBranding100%,TenantBrandingData100%,TenantBrandingService98%.Also verified end to end against Fineract 1.16.0-SNAPSHOT with PostgreSQL 18.3, with both plugin jars mounted on
/app/plugins:GET /v1/brandingreturns{"primaryColor":"blue"}with staff credentialsPUT {"primaryColor":"green"}persists and reads backm_selfservice_tenant_branding, audit timestamps populatedDeployment note
The self-service plugin cannot boot alone: it needs
savings-pluginon the classpath forBccrExchangeRateService, otherwise Fineract fails to start withNoClassDefFoundError. Pre-existing and documented inpom.xml, but worth knowing if you reproduce the run above.Open question
The table is named
m_selfservice_tenant_brandingto follow the repo'sm_selfservice_*convention and avoid colliding with a future Fineract core table, though branding is not strictly a self-service concern. Happy to rename if you'd prefer something neutral.Related
Consumed by openMF/web-app #3784 (#WEB-1084), which degrades to the default colour and reports the feature unavailable when this plugin is not installed.
Summary by CodeRabbit
New Features
Tests