MX-263: implement offices self service endpoints#170
Merged
IOhacker merged 1 commit intoMay 27, 2026
Conversation
📝 WalkthroughWalkthroughAdds four self-service office GET endpoints (details, services, geolocation, address) with immutable DTOs, a read-only JDBC service, Liquibase schema and permission changes, MapStruct/Swagger mappings, Bruno request docs, and unit + integration tests. ChangesSelf-Service Office Sub-Resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🧹 Nitpick comments (9)
src/main/java/org/apache/fineract/selfservice/office/data/OfficeGeolocationData.java (1)
19-40: ⚡ Quick winDocument the public geolocation model with Javadoc.
Please add Javadoc to the public class and public methods so the nullability/precision expectations are explicit.
As per coding guidelines, “Public methods and classes MUST have Javadoc.”
🤖 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/selfservice/office/data/OfficeGeolocationData.java` around lines 19 - 40, Add Javadoc to the public class OfficeGeolocationData and its public methods (the factory method instance(BigDecimal latitude, BigDecimal longitude) and accessors getLatitude() and getLongitude()) to document nullability (whether latitude/longitude may be null), expected numeric precision/scale (e.g., decimal degrees and required scale), valid ranges (e.g., latitude -90..90, longitude -180..180 if applicable), and that the constructor is private and instances should be created via instance(...); ensure the class-level Javadoc gives a short description of the model and usage, and each method Javadoc specifies parameters/return values and any validation behavior.src/main/java/org/apache/fineract/selfservice/office/data/OfficeDetailsData.java (1)
17-44: ⚡ Quick winAdd Javadoc for the public DTO API.
The public class and its public methods are missing Javadoc, which makes the contract less explicit for downstream callers.
As per coding guidelines, “Public methods and classes MUST have Javadoc.”
🤖 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/selfservice/office/data/OfficeDetailsData.java` around lines 17 - 44, Add Javadoc comments for the public DTO API: document the class OfficeDetailsData with a one-line description of its purpose, and add Javadoc to each public method instance(Long id, String name, String externalId), getId(), getName(), and getExternalId() describing parameters (for instance: `@param` id, `@param` name, `@param` externalId) and return values (`@return`) as appropriate; keep the constructor private and undocumented, but ensure the instance factory Javadoc explains it returns a new OfficeDetailsData instance representing an office with the given id, name, and externalId.src/main/java/org/apache/fineract/selfservice/office/data/SelfOfficeAddressData.java (1)
17-65: ⚡ Quick winDocument the public address DTO API.
Please add Javadoc to the public class and its public methods to make field semantics clear.
As per coding guidelines, “Public methods and classes MUST have Javadoc.”
🤖 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/selfservice/office/data/SelfOfficeAddressData.java` around lines 17 - 65, Add Javadoc comments to the public DTO SelfOfficeAddressData and all its public methods to document field semantics and usage: add a class-level Javadoc above SelfOfficeAddressData describing the DTO purpose and which entity it represents, and add method-level Javadocs for instance(), getStreetAndNumber(), getPostalCode(), getMunicipality(), getState(), and getCountry() that explain the meaning of each parameter/returned value (e.g., streetAndNumber format, postalCode expectations, municipality/state/country semantics) and note nullability/constraints if any; keep comments concise and follow project's Javadoc style.src/main/java/org/apache/fineract/selfservice/office/starter/SelfOfficeConfiguration.java (1)
25-33: ⚡ Quick winAdd Javadoc to the configuration class and bean factory method.
Please document the bean purpose and conditional-registration behavior.
As per coding guidelines, “Provide proper Javadoc for public APIs.”
🤖 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/selfservice/office/starter/SelfOfficeConfiguration.java` around lines 25 - 33, Add Javadoc to the SelfOfficeConfiguration class and its bean factory method selfServiceOfficeReadPlatformService describing the purpose of the configuration (registering SelfService office read services) and that the bean is conditionally created only when a SelfServiceOfficeReadPlatformService is missing. In the class-level comment mention it provides Spring beans for self-service office support, and in the method-level comment document the returned type SelfServiceOfficeReadPlatformService, the injected parameters (JdbcTemplate and PlatformSelfServiceSecurityContext), and the `@ConditionalOnMissingBean` behavior so callers know this bean will not override existing implementations.src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java (1)
37-155: ⚡ Quick winDocument the public service implementation API with Javadoc.
Add Javadoc to the public class and public methods to make behavior (especially not-found/empty cases) explicit.
As per coding guidelines, “Public methods and classes MUST have Javadoc.”
🤖 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/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java` around lines 37 - 155, Add Javadoc to the public class SelfServiceOfficeReadPlatformServiceImpl and to each public method (isOfficeAddressTableAvailable, retrieveOfficeDetails, retrieveOfficeServices, retrieveOfficeGeolocation, retrieveOfficeAddress) describing purpose, parameters, return values and exceptional/empty cases: document that methods call context.authenticatedSelfServiceUser(), that validateOfficeExistsInHierarchy throws OfficeNotFoundException when office is outside the user hierarchy (refer to validateOfficeExistsInHierarchy/OfficeNotFoundException), that retrieveOfficeDetails throws OfficeNotFoundException if missing, that retrieveOfficeGeolocation and retrieveOfficeAddress return null when no row exists, and that isOfficeAddressTableAvailable reports if the m_office_address table is present; keep Javadoc concise and follow project style for `@param`, `@return`, and `@throws` tags.src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java (1)
23-34: ⚡ Quick winAdd Javadoc to the service interface and each public method.
The interface is public API and should define behavior/edge cases in Javadoc.
As per coding guidelines, “Public methods and classes MUST have Javadoc.”
🤖 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/selfservice/office/service/SelfServiceOfficeReadPlatformService.java` around lines 23 - 34, The public interface SelfServiceOfficeReadPlatformService and all its public methods (retrieveOfficeDetails, retrieveOfficeServices, retrieveOfficeGeolocation, retrieveOfficeAddress, isOfficeAddressTableAvailable) lack Javadoc; add class-level Javadoc on SelfServiceOfficeReadPlatformService describing its responsibility, threading/transaction expectations and error/edge-case behavior, and add method-level Javadoc for each method specifying purpose, parameters (officeId), return values, possible nulls or exceptions thrown, and any preconditions or side-effects so the public API is fully documented.src/main/java/org/apache/fineract/selfservice/office/data/OfficeServiceData.java (1)
17-57: ⚡ Quick winAdd Javadoc for the public office-service data contract.
The public class/factory/getters should be documented to define the API contract clearly.
As per coding guidelines, “Provide proper Javadoc for public APIs.”
🤖 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/selfservice/office/data/OfficeServiceData.java` around lines 17 - 57, Add Javadoc comments for the public API in OfficeServiceData: document the class OfficeServiceData with a brief description of its purpose and immutability, then add Javadoc on the factory method instance(...) describing each parameter (serviceId, serviceName, serviceExternalId, serviceWorkingHours) and what the method returns, and add Javadoc on each public getter (getServiceId, getServiceName, getServiceExternalId, getServiceWorkingHours) describing the returned value and nullability/format expectations; keep comments concise and follow project style (summary line, `@param`, `@return` as appropriate).src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.java (1)
115-126: ⚡ Quick winStrengthen happy-path assertions for geolocation and address resource tests.
These tests currently pass on
assertNotNull(result)only; they should also verify the expected service delegation and serialization call, like the details/services tests do.Proposed test hardening
@@ void retrieveOfficeGeolocation_validId_returnsData() { @@ String result = resource.retrieveOfficeGeolocation(OFFICE_ID); assertNotNull(result); + verify(selfServiceOfficeReadPlatformService).retrieveOfficeGeolocation(OFFICE_ID); + verify(toApiJsonSerializer).serializeResult(data); } @@ void retrieveOfficeAddress_validId_returnsData() { @@ String result = resource.retrieveOfficeAddress(OFFICE_ID); assertNotNull(result); + verify(selfServiceOfficeReadPlatformService).retrieveOfficeAddress(OFFICE_ID); + verify(toApiJsonSerializer).serializeResult(data); }As per coding guidelines, "src/test/java/**: Verify both happy path and edge cases."
Also applies to: 134-144
🤖 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/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.java` around lines 115 - 126, Update the happy-path assertions in SelfOfficesApiResourceTest: for retrieveOfficeGeolocation_validId_returnsData() replace the lone assertNotNull(result) with an assertion that result equals the mocked serialized JSON ("{}") and add Mockito verifications that selfServiceOfficeReadPlatformService.retrieveOfficeGeolocation(OFFICE_ID) was called and that toApiJsonSerializer.serializeResult(data) was invoked with the OfficeGeolocationData instance; apply the same pattern to the other test (the address test around lines 134-144) so it asserts the returned JSON and verifies delegation to selfServiceOfficeReadPlatformService.retrieveOfficeAddress(...) and toApiJsonSerializer.serializeResult(...) with the expected data object.src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.java (1)
168-171: ⚡ Quick winUse deterministic assertions tied to seeded data in success-path tests.
isNotEmpty(),isNotBlank(), andisNotZero()are weak here; assert exact expected values from the seeded records to prevent false positives from unrelated data.As per coding guidelines, "src/test/java/**: Verify both happy path and edge cases."
Also applies to: 197-199
🤖 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/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.java` around lines 168 - 171, The assertions in SelfOfficesApiIntegrationTest (around the test that reads List<?> services = response.jsonPath().getList("$")) use non-deterministic checks (isNotEmpty, isNotBlank, isNotZero); update these to assert exact expected values from the seeded test data (e.g., assert that services.size() equals the known seeded count and that response.jsonPath().getString("[0].serviceName") equals the expected seed name, and similarly assert exact numeric ids or counts instead of isNotZero()). Apply the same change to the second occurrence noted (around lines 197-199) so both happy-path tests validate deterministic, seeded values rather than loose existence checks.
🤖 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 `@api-reference/bruno/SELF` SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING)
- LOCALHOST/OFFICES/05. GET OFFICE DETAILS BY ID.yml:
- Around line 12-13: The file contains a hardcoded Authorization header value
("Authorization: Basic cGVkcm8ubWFybW...") which must be removed; replace the
literal with a parameterized placeholder (e.g., "{{BASIC_AUTH}}" or a variable
like "${{BASIC_AUTH_TOKEN}}") and update the request example to indicate the
value comes from an environment/secret store. Modify the YAML entry named
"Authorization" to reference that placeholder and add a short comment or note
near the request describing how to set BASIC_AUTH in environment/secret
configuration so no credentials are committed.
In
`@src/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResource.java`:
- Around line 247-252: The authorization check must run before validating the
officeId to ensure access control is enforced first; in the
retrieveOfficeDetails method move the call to
context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS)
to occur before the officeId null/<=0 check that throws OfficeNotFoundException,
and apply the same change to the other office endpoints that perform an officeId
validation (the other methods currently validating officeId then calling
validateHasReadPermission) so that all endpoints call validateHasReadPermission
first.
- Around line 215-229: The endpoint retrieveOfficeByExternalId currently ignores
the ApiRequestJsonSerializationSettings parsed into settings which bypasses any
fields= projection; after building the response DTO with
officeSwaggerMapper.toGetOfficesResponse(office) apply the
serialization/filtering using the existing apiRequestParameterHelper and the
settings before returning (e.g., pass the mapped response and settings into the
helper or add an overload to officeSwaggerMapper that accepts
ApiRequestJsonSerializationSettings) so the fields= query is honored; update
retrieveOfficeByExternalId to return the serialized/filtered output instead of
the raw mapped DTO.
In
`@src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java`:
- Around line 29-31: The interface methods retrieveOfficeGeolocation and
retrieveOfficeAddress currently return null to signal missing data; change their
signatures in SelfServiceOfficeReadPlatformService to return
Optional<OfficeGeolocationData> and Optional<SelfOfficeAddressData>, update the
implementations to catch EmptyResultDataAccessException and table-unavailable
cases and return Optional.empty() instead of null, and update callers (notably
SelfOfficesApiResource) to unwrap or handle the Optional before passing data to
toApiJsonSerializer.serializeResult(data) so no nulls are forwarded.
In
`@src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java`:
- Around line 45-59: The checkTableAvailability method in
SelfServiceOfficeReadPlatformServiceImpl currently catches all Exceptions and
silently sets officeAddressTableAvailable=false; change this to use SLF4J
logging (add `@Slf4j` to the class) and catch the specific exceptions you expect
(e.g., SQLException or DataAccessException) while logging the full error and
context via log.error("Failed to detect m_office_address table availability",
e); keep the fallback assignment to officeAddressTableAvailable=false but do not
swallow unexpected exceptions—either rethrow runtime exceptions or let them
propagate after logging so failures are visible; identify the
jdbcTemplate.execute(...) block and update its try/catch accordingly to log
exception details instead of suppressing them.
In
`@src/main/resources/db/changelog/tenant/module/selfservice/parts/020-create-selfservice-office-geolocation-table.xml`:
- Around line 27-31: Add DB-level CHECK constraints to enforce valid coordinate
ranges for the latitude and longitude columns created with DECIMAL(10,7): add
two Liquibase <addCheckConstraint> entries (or inline check clauses) referencing
the latitude and longitude columns (e.g., constraint names like
chk_office_latitude_range and chk_office_longitude_range) with
checkCondition="latitude BETWEEN -90 AND 90" and checkCondition="longitude
BETWEEN -180 AND 180" respectively, leaving the DECIMAL(10,7) types unchanged so
the DB will reject out-of-range geolocation values at insert/update.
In
`@src/main/resources/db/changelog/tenant/module/selfservice/parts/021-grant-selfservice-office-read-permissions.xml`:
- Around line 15-26: The preConditions block currently only checks table
existence and can silently no-op if the 'Self Service User' role or
'READ_OFFICE' permission is missing; update the preConditions (the
<preConditions> surrounding the INSERT into m_role_permission) to add sqlCheck
conditions that verify the role exists (e.g., SELECT COUNT(*) FROM m_role WHERE
name='Self Service User') and the permission exists (e.g., SELECT COUNT(*) FROM
m_permission WHERE code='READ_OFFICE'), and change onFail behavior to HALT (or
FAIL) so the migration fails instead of marking ran when those checks return 0;
optionally add a sqlCheck ensuring the INSERT will affect at least one row (or
that the mapping does not already exist) to avoid silent no-ops.
In
`@src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.java`:
- Around line 201-209: Add a new authenticated integration test that covers the
happy path for the /v1/self/offices/{id}/address endpoint: create a test method
(e.g., retrieveAddress_withAuth_returns200) that uses the same requestSpec setup
as other authenticated self-service tests, performs a GET to SELF_OFFICES_PATH +
"/" + officeId + "/address with valid auth, asserts HTTP 200, and verifies
expected address fields (e.g., street, city, postalCode or whatever fields your
API returns) in the JSON response; place this next to
retrieveAddress_withoutAuth_returns403 in SelfOfficesApiIntegrationTest so both
unauthenticated and authenticated paths are covered.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/office/data/OfficeDetailsData.java`:
- Around line 17-44: Add Javadoc comments for the public DTO API: document the
class OfficeDetailsData with a one-line description of its purpose, and add
Javadoc to each public method instance(Long id, String name, String externalId),
getId(), getName(), and getExternalId() describing parameters (for instance:
`@param` id, `@param` name, `@param` externalId) and return values (`@return`) as
appropriate; keep the constructor private and undocumented, but ensure the
instance factory Javadoc explains it returns a new OfficeDetailsData instance
representing an office with the given id, name, and externalId.
In
`@src/main/java/org/apache/fineract/selfservice/office/data/OfficeGeolocationData.java`:
- Around line 19-40: Add Javadoc to the public class OfficeGeolocationData and
its public methods (the factory method instance(BigDecimal latitude, BigDecimal
longitude) and accessors getLatitude() and getLongitude()) to document
nullability (whether latitude/longitude may be null), expected numeric
precision/scale (e.g., decimal degrees and required scale), valid ranges (e.g.,
latitude -90..90, longitude -180..180 if applicable), and that the constructor
is private and instances should be created via instance(...); ensure the
class-level Javadoc gives a short description of the model and usage, and each
method Javadoc specifies parameters/return values and any validation behavior.
In
`@src/main/java/org/apache/fineract/selfservice/office/data/OfficeServiceData.java`:
- Around line 17-57: Add Javadoc comments for the public API in
OfficeServiceData: document the class OfficeServiceData with a brief description
of its purpose and immutability, then add Javadoc on the factory method
instance(...) describing each parameter (serviceId, serviceName,
serviceExternalId, serviceWorkingHours) and what the method returns, and add
Javadoc on each public getter (getServiceId, getServiceName,
getServiceExternalId, getServiceWorkingHours) describing the returned value and
nullability/format expectations; keep comments concise and follow project style
(summary line, `@param`, `@return` as appropriate).
In
`@src/main/java/org/apache/fineract/selfservice/office/data/SelfOfficeAddressData.java`:
- Around line 17-65: Add Javadoc comments to the public DTO
SelfOfficeAddressData and all its public methods to document field semantics and
usage: add a class-level Javadoc above SelfOfficeAddressData describing the DTO
purpose and which entity it represents, and add method-level Javadocs for
instance(), getStreetAndNumber(), getPostalCode(), getMunicipality(),
getState(), and getCountry() that explain the meaning of each parameter/returned
value (e.g., streetAndNumber format, postalCode expectations,
municipality/state/country semantics) and note nullability/constraints if any;
keep comments concise and follow project's Javadoc style.
In
`@src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java`:
- Around line 23-34: The public interface SelfServiceOfficeReadPlatformService
and all its public methods (retrieveOfficeDetails, retrieveOfficeServices,
retrieveOfficeGeolocation, retrieveOfficeAddress, isOfficeAddressTableAvailable)
lack Javadoc; add class-level Javadoc on SelfServiceOfficeReadPlatformService
describing its responsibility, threading/transaction expectations and
error/edge-case behavior, and add method-level Javadoc for each method
specifying purpose, parameters (officeId), return values, possible nulls or
exceptions thrown, and any preconditions or side-effects so the public API is
fully documented.
In
`@src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java`:
- Around line 37-155: Add Javadoc to the public class
SelfServiceOfficeReadPlatformServiceImpl and to each public method
(isOfficeAddressTableAvailable, retrieveOfficeDetails, retrieveOfficeServices,
retrieveOfficeGeolocation, retrieveOfficeAddress) describing purpose,
parameters, return values and exceptional/empty cases: document that methods
call context.authenticatedSelfServiceUser(), that
validateOfficeExistsInHierarchy throws OfficeNotFoundException when office is
outside the user hierarchy (refer to
validateOfficeExistsInHierarchy/OfficeNotFoundException), that
retrieveOfficeDetails throws OfficeNotFoundException if missing, that
retrieveOfficeGeolocation and retrieveOfficeAddress return null when no row
exists, and that isOfficeAddressTableAvailable reports if the m_office_address
table is present; keep Javadoc concise and follow project style for `@param`,
`@return`, and `@throws` tags.
In
`@src/main/java/org/apache/fineract/selfservice/office/starter/SelfOfficeConfiguration.java`:
- Around line 25-33: Add Javadoc to the SelfOfficeConfiguration class and its
bean factory method selfServiceOfficeReadPlatformService describing the purpose
of the configuration (registering SelfService office read services) and that the
bean is conditionally created only when a SelfServiceOfficeReadPlatformService
is missing. In the class-level comment mention it provides Spring beans for
self-service office support, and in the method-level comment document the
returned type SelfServiceOfficeReadPlatformService, the injected parameters
(JdbcTemplate and PlatformSelfServiceSecurityContext), and the
`@ConditionalOnMissingBean` behavior so callers know this bean will not override
existing implementations.
In
`@src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.java`:
- Around line 168-171: The assertions in SelfOfficesApiIntegrationTest (around
the test that reads List<?> services = response.jsonPath().getList("$")) use
non-deterministic checks (isNotEmpty, isNotBlank, isNotZero); update these to
assert exact expected values from the seeded test data (e.g., assert that
services.size() equals the known seeded count and that
response.jsonPath().getString("[0].serviceName") equals the expected seed name,
and similarly assert exact numeric ids or counts instead of isNotZero()). Apply
the same change to the second occurrence noted (around lines 197-199) so both
happy-path tests validate deterministic, seeded values rather than loose
existence checks.
In
`@src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.java`:
- Around line 115-126: Update the happy-path assertions in
SelfOfficesApiResourceTest: for retrieveOfficeGeolocation_validId_returnsData()
replace the lone assertNotNull(result) with an assertion that result equals the
mocked serialized JSON ("{}") and add Mockito verifications that
selfServiceOfficeReadPlatformService.retrieveOfficeGeolocation(OFFICE_ID) was
called and that toApiJsonSerializer.serializeResult(data) was invoked with the
OfficeGeolocationData instance; apply the same pattern to the other test (the
address test around lines 134-144) so it asserts the returned JSON and verifies
delegation to selfServiceOfficeReadPlatformService.retrieveOfficeAddress(...)
and toApiJsonSerializer.serializeResult(...) with the expected data object.
🪄 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
Run ID: 153e87d1-b057-42e9-ae74-ef1ba00cac06
📒 Files selected for processing (22)
api-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/05. GET OFFICE DETAILS BY ID.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/06. GET OFFICE SERVICES.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/07. GET OFFICE GEOLOCATION.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/08. GET OFFICE ADDRESS.ymlsrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficeSwaggerMapper.javasrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResource.javasrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeDetailsData.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeGeolocationData.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeServiceData.javasrc/main/java/org/apache/fineract/selfservice/office/data/SelfOfficeAddressData.javasrc/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.javasrc/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/office/starter/SelfOfficeConfiguration.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/019-create-selfservice-office-service-table.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/020-create-selfservice-office-geolocation-table.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/021-grant-selfservice-office-read-permissions.xmlsrc/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.javasrc/test/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceTestUtils.java
Comment on lines
+215
to
+229
| public SelfOfficesApiResourceSwagger.GetOfficesResponse retrieveOfficeByExternalId( | ||
| @PathParam("externalId") @Parameter(description = "externalId") final String externalId, | ||
| @Context final UriInfo uriInfo) { | ||
| context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS); | ||
| final ApiRequestJsonSerializationSettings settings = | ||
| apiRequestParameterHelper.process(uriInfo.getQueryParameters()); | ||
| OfficeData office = | ||
| readPlatformService.retrieveOfficeWithExternalId(ExternalIdFactory.produce(externalId)); | ||
| if (settings.isTemplate()) { | ||
| final Collection<OfficeData> allowedParents = | ||
| readPlatformService.retrieveAllowedParents(office.getId()); | ||
| office = OfficeData.appendedTemplate(office, allowedParents); | ||
| } | ||
| return officeSwaggerMapper.toGetOfficesResponse(office); | ||
| } |
There was a problem hiding this comment.
fields query projection is bypassed on the external-id endpoint.
Line 219 parses request serialization settings, but Line 228 returns a mapped DTO directly, so fields= filtering in the documented examples (Line 214) is not applied. This is a behavior regression compared with serializer-based endpoints.
🤖 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/selfservice/office/api/SelfOfficesApiResource.java`
around lines 215 - 229, The endpoint retrieveOfficeByExternalId currently
ignores the ApiRequestJsonSerializationSettings parsed into settings which
bypasses any fields= projection; after building the response DTO with
officeSwaggerMapper.toGetOfficesResponse(office) apply the
serialization/filtering using the existing apiRequestParameterHelper and the
settings before returning (e.g., pass the mapped response and settings into the
helper or add an overload to officeSwaggerMapper that accepts
ApiRequestJsonSerializationSettings) so the fields= query is honored; update
retrieveOfficeByExternalId to return the serialized/filtered output instead of
the raw mapped DTO.
Comment on lines
+29
to
+31
| OfficeGeolocationData retrieveOfficeGeolocation(Long officeId); | ||
|
|
||
| SelfOfficeAddressData retrieveOfficeAddress(Long officeId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites of these two methods and whether they null-check results.
rg -n -C3 '\bretrieveOfficeGeolocation\s*\(|\bretrieveOfficeAddress\s*\(' --type=java
rg -n -C3 '==\s*null|!=\s*null|Optional<OfficeGeolocationData>|Optional<SelfOfficeAddressData>' --type=javaRepository: openMF/selfservice-plugin
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Main-code call sites (exclude tests)
echo "== Call sites in src/main/java =="
rg -n --glob 'src/main/java/**/*.java' 'retrieveOfficeGeolocation\s*\(' .
rg -n --glob 'src/main/java/**/*.java' 'retrieveOfficeAddress\s*\(' .
# 2) Inspect service interface + impl methods
echo "== Interface declaration =="
rg -n 'interface SelfServiceOfficeReadPlatformService' -S src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java
sed -n '1,120p' src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java
echo "== Service impl: retrieveOfficeGeolocation =="
rg -n 'retrieveOfficeGeolocation\s*\(' src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java
sed -n '90,180p' src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java
echo "== Service impl: retrieveOfficeAddress =="
rg -n 'retrieveOfficeAddress\s*\(' src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java
sed -n '180,280p' src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java
# 3) Inspect API resource handling
echo "== API resource: retrieveOfficeGeolocation =="
sed -n '260,360p' src/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResource.java
# 4) Find serializer implementation / behavior with null
echo "== Find serializeResult(...) implementation =="
rg -n 'serializeResult\s*\(' src/main/java
rg -n 'class .*JsonSerializer|interface .*JsonSerializer|toApiJsonSerializer' src/main/java
# Show likely serializer class if found
# (Use best-effort: print files with toApiJsonSerializer definition)
rg -n 'toApiJsonSerializer' src/main/java | head -n 50Repository: openMF/selfservice-plugin
Length of output: 25249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate serializer implementation(s) for serializeResult(...)
echo "== Find DefaultToApiJsonSerializer and serializeResult(...) =="
rg -n "class DefaultToApiJsonSerializer|interface ToApiJsonSerializer|serializeResult\s*\(" src/main/java
# Print likely implementation files around serializeResult
FILES=$(rg -l "serializeResult\s*\(" src/main/java | tr '\n' ' ')
for f in $FILES; do
echo "---- $f (serializeResult context) ----"
rg -n "serializeResult\s*\(" "$f"
done
# Show one or two best matches' contents around serializeResult
# (limit to keep output small)
for f in $(rg -l "serializeResult\s*\(" src/main/java | head -n 5); do
echo "==== $f ===="
sed -n '1,220p' "$f"
doneRepository: openMF/selfservice-plugin
Length of output: 10929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate DefaultToApiJsonSerializer source in this repo =="
rg -n "class DefaultToApiJsonSerializer" . --glob '*.java' || true
rg -n "interface ToApiJsonSerializer" . --glob '*.java' || true
rg -n "serializeResult\s*\(" . --glob '*.java' || true
echo "== Inspect SelfOfficesApiResourceTest for geolocation/address null handling =="
sed -n '1,220p' src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.javaRepository: openMF/selfservice-plugin
Length of output: 7451
Make absent geolocation/address results explicit with Optional
SelfServiceOfficeReadPlatformService currently returns null for both geolocation and address (geolocation: EmptyResultDataAccessException -> return null; address: table unavailable -> return null, and EmptyResultDataAccessException -> return null). Callers in SelfOfficesApiResource pass the result directly to toApiJsonSerializer.serializeResult(data) with no null handling, and the existing tests assert null is returned for these cases.
Proposed contract direction
- OfficeGeolocationData retrieveOfficeGeolocation(Long officeId);
+ Optional<OfficeGeolocationData> retrieveOfficeGeolocation(Long officeId);
- SelfOfficeAddressData retrieveOfficeAddress(Long officeId);
+ Optional<SelfOfficeAddressData> retrieveOfficeAddress(Long officeId);This avoids implicit null handling at the API layer and forces consumers to handle “no data” explicitly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OfficeGeolocationData retrieveOfficeGeolocation(Long officeId); | |
| SelfOfficeAddressData retrieveOfficeAddress(Long officeId); | |
| Optional<OfficeGeolocationData> retrieveOfficeGeolocation(Long officeId); | |
| Optional<SelfOfficeAddressData> retrieveOfficeAddress(Long officeId); |
🤖 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/selfservice/office/service/SelfServiceOfficeReadPlatformService.java`
around lines 29 - 31, The interface methods retrieveOfficeGeolocation and
retrieveOfficeAddress currently return null to signal missing data; change their
signatures in SelfServiceOfficeReadPlatformService to return
Optional<OfficeGeolocationData> and Optional<SelfOfficeAddressData>, update the
implementations to catch EmptyResultDataAccessException and table-unavailable
cases and return Optional.empty() instead of null, and update callers (notably
SelfOfficesApiResource) to unwrap or handle the Optional before passing data to
toApiJsonSerializer.serializeResult(data) so no nulls are forwarded.
Comment on lines
+15
to
+26
| <preConditions onFail="MARK_RAN"> | ||
| <tableExists tableName="m_role_permission"/> | ||
| <tableExists tableName="m_permission"/> | ||
| <tableExists tableName="m_role"/> | ||
| </preConditions> | ||
| <sql> | ||
| INSERT INTO m_role_permission (role_id, permission_id) | ||
| SELECT r.id, p.id | ||
| FROM m_role r | ||
| CROSS JOIN m_permission p | ||
| WHERE r.name = 'Self Service User' | ||
| AND p.code IN ('READ_OFFICE') |
There was a problem hiding this comment.
Harden preconditions to prevent silent permission-mapping no-op.
Line 15-Line 26 only checks table existence. If Self Service User or READ_OFFICE is absent, the changeset completes without granting access and without failing migration.
Proposed Liquibase fix
- <changeSet id="ss-0021-1" author="fineract">
- <preConditions onFail="MARK_RAN">
+ <changeSet id="ss-0021-1" author="fineract">
+ <preConditions onFail="HALT">
<tableExists tableName="m_role_permission"/>
<tableExists tableName="m_permission"/>
<tableExists tableName="m_role"/>
+ <sqlCheck expectedResult="1">
+ SELECT COUNT(*) FROM m_role WHERE name = 'Self Service User'
+ </sqlCheck>
+ <sqlCheck expectedResult="1">
+ SELECT COUNT(*) FROM m_permission WHERE code = 'READ_OFFICE'
+ </sqlCheck>
</preConditions>As per coding guidelines, "Apply the principle of least privilege for access control."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <preConditions onFail="MARK_RAN"> | |
| <tableExists tableName="m_role_permission"/> | |
| <tableExists tableName="m_permission"/> | |
| <tableExists tableName="m_role"/> | |
| </preConditions> | |
| <sql> | |
| INSERT INTO m_role_permission (role_id, permission_id) | |
| SELECT r.id, p.id | |
| FROM m_role r | |
| CROSS JOIN m_permission p | |
| WHERE r.name = 'Self Service User' | |
| AND p.code IN ('READ_OFFICE') | |
| <preConditions onFail="HALT"> | |
| <tableExists tableName="m_role_permission"/> | |
| <tableExists tableName="m_permission"/> | |
| <tableExists tableName="m_role"/> | |
| <sqlCheck expectedResult="1"> | |
| SELECT COUNT(*) FROM m_role WHERE name = 'Self Service User' | |
| </sqlCheck> | |
| <sqlCheck expectedResult="1"> | |
| SELECT COUNT(*) FROM m_permission WHERE code = 'READ_OFFICE' | |
| </sqlCheck> | |
| </preConditions> | |
| <sql> | |
| INSERT INTO m_role_permission (role_id, permission_id) | |
| SELECT r.id, p.id | |
| FROM m_role r | |
| CROSS JOIN m_permission p | |
| WHERE r.name = 'Self Service User' | |
| AND p.code IN ('READ_OFFICE') |
🤖 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/resources/db/changelog/tenant/module/selfservice/parts/021-grant-selfservice-office-read-permissions.xml`
around lines 15 - 26, The preConditions block currently only checks table
existence and can silently no-op if the 'Self Service User' role or
'READ_OFFICE' permission is missing; update the preConditions (the
<preConditions> surrounding the INSERT into m_role_permission) to add sqlCheck
conditions that verify the role exists (e.g., SELECT COUNT(*) FROM m_role WHERE
name='Self Service User') and the permission exists (e.g., SELECT COUNT(*) FROM
m_permission WHERE code='READ_OFFICE'), and change onFail behavior to HALT (or
FAIL) so the migration fails instead of marking ran when those checks return 0;
optionally add a sqlCheck ensuring the INSERT will affect at least one row (or
that the mapping does not already exist) to avoid silent no-ops.
Comment on lines
+201
to
+209
| @Test | ||
| @DisplayName("GET /v1/self/offices/{id}/address without auth returns 403") | ||
| void retrieveAddress_withoutAuth_returns403() { | ||
| given(SelfServiceTestUtils.requestSpec(getFineractPort())) | ||
| .when() | ||
| .get(SELF_OFFICES_PATH + "/" + officeId + "/address") | ||
| .then() | ||
| .statusCode(403); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add authenticated happy-path coverage for /address.
The address endpoint currently has only the unauthenticated 403 test; add a self-service authenticated 200 test with field assertions to cover the primary success path.
Suggested additional integration test
+ `@Test`
+ `@DisplayName`("GET /v1/self/offices/{id}/address with self-service user returns 200")
+ void retrieveAddress_withSelfServiceUser_returns200() {
+ Response response =
+ given(
+ SelfServiceTestUtils.requestSpecWithAuth(
+ getFineractPort(), selfServiceUsername, "password"))
+ .when()
+ .get(SELF_OFFICES_PATH + "/" + officeId + "/address")
+ .then()
+ .statusCode(200)
+ .extract()
+ .response();
+
+ assertThat(response.jsonPath().getString("city")).isNotBlank();
+ assertThat(response.jsonPath().getString("country")).isNotBlank();
+ }As per coding guidelines, "src/test/java/**: Verify both happy path and edge cases."
🤖 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/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.java`
around lines 201 - 209, Add a new authenticated integration test that covers the
happy path for the /v1/self/offices/{id}/address endpoint: create a test method
(e.g., retrieveAddress_withAuth_returns200) that uses the same requestSpec setup
as other authenticated self-service tests, performs a GET to SELF_OFFICES_PATH +
"/" + officeId + "/address with valid auth, asserts HTTP 200, and verifies
expected address fields (e.g., street, city, postalCode or whatever fields your
API returns) in the JSON response; place this next to
retrieveAddress_withoutAuth_returns403 in SelfOfficesApiIntegrationTest so both
unauthenticated and authenticated paths are covered.
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
DeathGun44
force-pushed
the
MX-263-offices-self-service-endpoints
branch
from
dc6f8e5 to
27ca390
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/selfservice/office/data/OfficeServiceData.java`:
- Around line 53-67: Add Javadoc comments to each public getter (getServiceId,
getServiceName, getServiceExternalId, getServiceWorkingHours) describing what
the method returns and include an `@return` tag; ensure the wording matches the
class-level documentation style and notes whether the return can be null (e.g.,
"the service identifier" / "the service name" / "the external identifier for the
service" / "the working hours for the service") so the public API is fully
documented.
🪄 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
Run ID: b414dc6a-01a5-4d46-a81b-01b86cd7d435
📒 Files selected for processing (22)
api-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/05. GET OFFICE DETAILS BY ID.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/06. GET OFFICE SERVICES.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/07. GET OFFICE GEOLOCATION.ymlapi-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/08. GET OFFICE ADDRESS.ymlsrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficeSwaggerMapper.javasrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResource.javasrc/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeDetailsData.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeGeolocationData.javasrc/main/java/org/apache/fineract/selfservice/office/data/OfficeServiceData.javasrc/main/java/org/apache/fineract/selfservice/office/data/SelfOfficeAddressData.javasrc/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.javasrc/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/office/starter/SelfOfficeConfiguration.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/019-create-selfservice-office-service-table.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/020-create-selfservice-office-geolocation-table.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/021-grant-selfservice-office-read-permissions.xmlsrc/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.javasrc/test/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceTestUtils.java
✅ Files skipped from review due to trivial changes (2)
- api-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/07. GET OFFICE GEOLOCATION.yml
- api-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/08. GET OFFICE ADDRESS.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xml
- src/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceTestUtils.java
- src/main/java/org/apache/fineract/selfservice/office/data/OfficeGeolocationData.java
- src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformService.java
- src/main/java/org/apache/fineract/selfservice/office/data/OfficeDetailsData.java
- src/main/resources/db/changelog/tenant/module/selfservice/parts/020-create-selfservice-office-geolocation-table.xml
- src/main/java/org/apache/fineract/selfservice/office/api/SelfOfficeSwaggerMapper.java
- src/main/java/org/apache/fineract/selfservice/office/starter/SelfOfficeConfiguration.java
- src/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceSwagger.java
- src/main/resources/db/changelog/tenant/module/selfservice/parts/021-grant-selfservice-office-read-permissions.xml
- api-reference/bruno/SELF SERVICE PLUGIN/SELF SERVICE (MOBILE OR WEB BANKING) - LOCALHOST/OFFICES/05. GET OFFICE DETAILS BY ID.yml
- src/main/java/org/apache/fineract/selfservice/office/data/SelfOfficeAddressData.java
- src/test/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImplTest.java
- src/test/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResourceTest.java
- src/main/java/org/apache/fineract/selfservice/office/service/SelfServiceOfficeReadPlatformServiceImpl.java
- src/main/java/org/apache/fineract/selfservice/office/api/SelfOfficesApiResource.java
Comment on lines
+53
to
+67
| public Long getServiceId() { | ||
| return serviceId; | ||
| } | ||
|
|
||
| public String getServiceName() { | ||
| return serviceName; | ||
| } | ||
|
|
||
| public String getServiceExternalId() { | ||
| return serviceExternalId; | ||
| } | ||
|
|
||
| public String getServiceWorkingHours() { | ||
| return serviceWorkingHours; | ||
| } |
There was a problem hiding this comment.
Add Javadoc to public getter methods.
These public API methods are undocumented while the class and factory are documented, creating inconsistent API docs.
As per coding guidelines, "Public methods and classes MUST have Javadoc."
🤖 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/selfservice/office/data/OfficeServiceData.java`
around lines 53 - 67, Add Javadoc comments to each public getter (getServiceId,
getServiceName, getServiceExternalId, getServiceWorkingHours) describing what
the method returns and include an `@return` tag; ensure the wording matches the
class-level documentation style and notes whether the return can be null (e.g.,
"the service identifier" / "the service name" / "the external identifier for the
service" / "the working hours for the service") so the public API is fully
documented.
IOhacker
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit