Aman-Mittal commented on code in PR #155: URL: https://github.com/apache/fineract-consumer-facing/pull/155#discussion_r4232577695
########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/data/SubmitSavingsApplicationCommandRequest.java: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.data; + +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; +import java.math.BigDecimal; +import java.time.LocalDate; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@RequiredArgsConstructor +@Builder +@ToString(onlyExplicitlyIncluded = true) +public final class SubmitSavingsApplicationCommandRequest { + + @NotNull + @Positive + private final Long productId; + + @NotNull + private final LocalDate submittedOnDate; + + private final BigDecimal nominalAnnualInterestRate; Review Comment: **Blocking:** this lets the consumer pick their own interest rate. When `nominalAnnualInterestRate` is present, Fineract uses it in place of the product's rate. Unlike loan products, savings products have no min/max rate band to clamp against, and the server-side validation is only zero-or-positive. A self-service caller could submit an application at 50% p.a., and it would rely entirely on a back-office reviewer noticing before approval. There is also no `@DecimalMin`/`@Digits` here, so negative or absurdly precise values go straight upstream. The rate is a product term. The consumer already sees it through `GET /api/v1/savings/template?productId=…`. I'd suggest dropping this field from both the submit and modify requests and letting Fineract apply the product default. That also removes the need for the `nominalAnnualInterestRate` sanitizer patch. ########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/data/ModifySavingsApplicationCommandRequest.java: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.data; + +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; +import java.math.BigDecimal; +import java.time.LocalDate; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@RequiredArgsConstructor +@Builder +@ToString(onlyExplicitlyIncluded = true) +public final class ModifySavingsApplicationCommandRequest { + + @NotNull + @Positive + private final Long productId; + + @NotNull + private final LocalDate submittedOnDate; + + private final BigDecimal nominalAnnualInterestRate; Review Comment: Same concern as on `SubmitSavingsApplicationCommandRequest`: the consumer can rewrite the rate on a pending application. Please remove it here too. ########## consumer/build.gradle: ########## @@ -307,6 +307,49 @@ def fixClientImageResponseShape(paths) { ]] } +def addMissingSavingsAccountCommandFields(schemas) { + def missingAccountFields = [ + withdrawnOnDate: [type: 'string'], + note : [type: 'string'] + ] + def properties = schemas.PostSavingsAccountsAccountIdRequest?.properties + if (properties == null) { + throw new GradleException("Fineract spec schema 'PostSavingsAccountsAccountIdRequest' missing — revisit addMissingSavingsAccountCommandFields") + } + missingAccountFields.each { propName, propDef -> + if (!properties.containsKey(propName)) { + properties[propName] = propDef + } + } + + def missingPostFields = [ + nominalAnnualInterestRate: [type: 'number'] + ] + def postProps = schemas.PostSavingsAccountsRequest?.properties + if (postProps != null) { Review Comment: This block, and the `putProps` block below it, silently skip when the schema is missing. `docs/consumer/architecture/fineract-integration.adoc` documents that schema-targeted patches throw a `GradleException` when their target disappears, so a Fineract spec upgrade can't drop the fix unnoticed. The first block in this function does that; these two should as well. Otherwise an upgrade surfaces as an unexplained "cannot find symbol `productId(…)`" compile error instead of a pointer to this function. Also, if `nominalAnnualInterestRate` is dropped (see the comment on the request DTO), the `missingPostFields` block can go away entirely. Please add the new patch to the sanitizer list in `fineract-integration.adoc`. ########## consumer/src/test/java/org/apache/fineract/consumer/savings/command/service/SavingsCommandServiceImplTest.java: ########## @@ -0,0 +1,238 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.service; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import feign.FeignException; +import java.math.BigDecimal; +import java.time.LocalDate; +import java.util.UUID; +import org.apache.fineract.consumer.infrastructure.access.data.ConsumerAction; +import org.apache.fineract.consumer.infrastructure.access.service.AccessPolicyEvaluator; +import org.apache.fineract.consumer.infrastructure.access.service.OwnedAccountsCache; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.api.SavingsAccountApi; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PostSavingsAccountsResponse; +import org.apache.fineract.consumer.infrastructure.fineractclient.generated.model.PutSavingsAccountsAccountIdResponse; +import org.apache.fineract.consumer.infrastructure.idempotency.service.IdempotencyKeyDeriver; +import org.apache.fineract.consumer.infrastructure.idempotency.service.IdempotencyKeyHolder; +import org.apache.fineract.consumer.savings.command.data.ModifySavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.data.SavingsApplicationCommandData; +import org.apache.fineract.consumer.savings.command.data.SubmitSavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.data.WithdrawSavingsApplicationCommand; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandInProgressException; +import org.apache.fineract.consumer.savings.command.exception.SavingsCommandUpstreamUnavailableException; +import org.apache.fineract.consumer.user.query.data.UserQueryData; +import org.apache.fineract.consumer.user.query.data.UserStatus; +import org.apache.fineract.consumer.user.query.service.UserQueryService; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InOrder; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpStatus; +import org.springframework.security.oauth2.jwt.Jwt; + +@ExtendWith(MockitoExtension.class) +class SavingsCommandServiceImplTest { + + private static final UUID PUBLIC_ID = UUID.fromString("3f2c8a1e-0000-4000-8000-000000000001"); + private static final Long CLIENT_ID = 42L; + private static final Long SAVINGS_ID = 5L; + private static final String EMAIL = "[email protected]"; + private static final String IDEMPOTENCY_KEY = "savings-op-key-1"; + + @Mock + private UserQueryService userQueryService; + + @Mock + private AccessPolicyEvaluator accessPolicyEvaluator; + + @Mock + private OwnedAccountsCache ownedAccountsCache; + + @Mock + private IdempotencyKeyHolder idempotencyKeyHolder; + + @Mock + private SavingsAccountApi savingsAccountApi; + + @InjectMocks + private SavingsCommandServiceImpl service; + + private static Jwt jwt() { + return Jwt.withTokenValue("token") + .header("alg", "none") + .subject(PUBLIC_ID.toString()) + .claim("scope", "read") + .build(); + } + + private static UserQueryData user() { + return UserQueryData.builder() + .id(1L) + .publicId(PUBLIC_ID) + .fineractClientId(CLIENT_ID) + .email(EMAIL) + .status(UserStatus.BOUND) + .build(); + } + + private static SubmitSavingsApplicationCommand submitCommand() { + return SubmitSavingsApplicationCommand.builder() + .productId(1L) + .submittedOnDate(LocalDate.of(2026, 7, 1)) + .nominalAnnualInterestRate(new BigDecimal("3.5")) + .externalId("ext-123") + .idempotencyKey(IDEMPOTENCY_KEY) + .build(); + } + + @Test + void submitAuthorizesSavingsApplicationSubmit() { + Jwt jwt = jwt(); + when(userQueryService.findByPublicId(PUBLIC_ID)).thenReturn(user()); + when(savingsAccountApi.submitSavingsApplication(any())) Review Comment: All three upstream calls are matched with `any()` for the request body, so none of the request-building code is covered. The tests would still pass if `clientId`, `locale`, `dateFormat`, the date string, or `note` were dropped or wrong. Could you add `ArgumentCaptor` assertions for: - submit: `clientId` is resolved from the user (not taken from the request), `productId`, `submittedOnDate` as `yyyy-MM-dd`, `locale=en`, `dateFormat`, and optional fields are absent when null - modify: the same field mapping on `PutSavingsAccountsAccountIdRequest` - withdraw: `withdrawnOnDate` and `note` are forwarded Please also add `verify(accessPolicyEvaluator).authorize(jwt, SAVINGS_APPLICATION_MODIFY / WITHDRAW, SAVINGS_ID, …)` for modify/withdraw, and a test that an access denial means `savingsAccountApi` is never called. ########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/data/SubmitSavingsApplicationCommandRequest.java: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.data; + +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; +import java.math.BigDecimal; +import java.time.LocalDate; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@RequiredArgsConstructor +@Builder +@ToString(onlyExplicitlyIncluded = true) +public final class SubmitSavingsApplicationCommandRequest { + + @NotNull + @Positive + private final Long productId; + + @NotNull + private final LocalDate submittedOnDate; + + private final BigDecimal nominalAnnualInterestRate; + + private final String externalId; Review Comment: `externalId` is the identifier back-office systems and integrations use to correlate accounts, and Fineract enforces uniqueness on it. If consumers can set it (and change it via PUT), a user can claim an id that an integration later tries to use and make that integration fail, or overwrite a correlation id an operator set. The loan command side doesn't expose it. Unless there's a concrete consumer use case, I'd drop it from submit and modify. If it stays, it needs at least `@Size(max = 100)` to match the Fineract column. ########## consumer/src/main/java/org/apache/fineract/consumer/savings/command/data/WithdrawSavingsApplicationCommandRequest.java: ########## @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.savings.command.data; + +import jakarta.validation.constraints.NotNull; +import java.time.LocalDate; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@RequiredArgsConstructor +@Builder +@ToString(onlyExplicitlyIncluded = true) +public final class WithdrawSavingsApplicationCommandRequest { + + @NotNull + private final LocalDate withdrawnOnDate; + + private final String note; Review Comment: `note` is unbounded free text that gets persisted upstream. Please add a `@Size(max = …)` matching Fineract's note column so oversized input is rejected here with a clean 400, not by an upstream failure. Separately, `WithdrawSavingsApplicationCommand` has a full `@ToString`, so this user-entered text will appear wherever the command is logged. Consider excluding it. ########## consumer/src/main/java/org/apache/fineract/consumer/infrastructure/fineractclient/spec/fineract.json: ########## Review Comment: Please revert this file. It's the vendored upstream spec and is replaced wholesale on a Fineract upgrade, which is why fixes go through `sanitizeFineractSpec`. A structural JSON diff against `main` shows only the same 7 properties that `addMissingSavingsAccountCommandFields` already injects at build time, so the edit is redundant. The file was also re-serialized (every `'` became `'`, about 2 KB of churn), which makes the one-line diff unreviewable and will conflict with the next spec bump. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
