Dhanno98 commented on PR #6448:
URL: https://github.com/apache/fineract/pull/6448#issuecomment-6345713676
> Hi @Dhanno98, thanks for the PR. The test migration itself looks clean, my
problem is the production API added to make it work.
>
> * Why a new public `/v2/accounts/{type}` API (new paths, new Feign
`ShareAccountV2Api`) just to get a typed request body for a test? I can't find
any other `@Path("/v2/...")` resource, so there is no versioning or naming
convention to follow, and a new endpoint is a permanent contract. The legacy
`AccountsApiResource` already handles `redeemshares` via `handleCommands`. Why
not fix the typed Swagger model (`PostAccountsTypeAccountIdRequest`) like you
already did for `locale` / `dateFormat` / `activatedDate`? If there is a JIRA /
dev-list agreement on a v2 convention, please link it.
Thanks @galovics for the detailed review!
Regarding the V2 API, I introduced it because while migrating
`ShareAccountChargeRoundingTest`, I found that the generated Swagger model for
`PostAccountsTypeAccountIdRequest` cannot represent all existing uses of
`requestedShares`.
For `redeemshares` and `applyadditionalshares`, `requestedShares` is parsed
as a number, for ex `100`, whereas for `approveadditionalshares` and
`rejectadditionalshares` it is parsed as an array of transaction IDs, for ex
`[{"id":35}]`.
I discussed this with @adamsaghy on
[FINERACT-2165](https://issues.apache.org/jira/browse/FINERACT-2165). After
confirming that both forms are actively used, Adam suggested splitting the
endpoint and recommended introducing new V2 endpoints. I proposed separate V2
operations for `applyadditionalshares`, `redeemshares`,
`approveadditionalshares` and `rejectadditionalshares`, while keeping the
existing V1 endpoint unchanged and reusing the existing command-processing flow.
JIRA discussion:
[https://issues.apache.org/jira/browse/FINERACT-2165](https://issues.apache.org/jira/browse/FINERACT-2165)
Since then `ShareAccountChargeRoundingTest` has already been migrated to
Feign in this merged PR #6419 by @DeathGun44 using a custom Feign request model
against the existing V1 endpoint for `redeemShares()` which avoids introducing
a new production API.
So I think the main point now is whether the project wants to address this
ambiguity using the V2 API approach discussed in FINERACT-2165 or whether the
existing V1 endpoint with custom Feign request model used in the already merged
PR is the preferred approach?
If the V2 API approach is not desired, then the already merged
implementation covers the test migration and I don't think my PR would add
anything meaningful beyond that. If the V2 approach is preferred, then the main
contribution of my PR would be to have `redeemShares()` use the V2 API.
Please let me know which direction you would prefer.
--
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]
