alberto-art3ch commented on PR #6060:
URL: https://github.com/apache/fineract/pull/6060#issuecomment-6537476001
> Hi @alberto-art3ch, thanks for the PR.
>
> The move itself looks fine to me: packages are kept, the dependency only
goes one way (`fineract-shares` -> core/charge/accounting/savings, nothing
below it points back to shares) and history is preserved. I'm happy with that
part.
>
> There are 2 things in here though that aren't really modularization, and
I'd take both out of this PR.
>
> The module toggle. Turning off `fineract.module.shares.enabled` only
disables the share command handlers, `AccountsApiResource` and
`ShareDividendApiResource`. Everything else stays on: `ProductsApiResource`
(`/v1/products/share`, so the GETs still work and the POSTs fail because the
handler is missing), the `PostDividentsForShares` job, all the read/write
services in `ShareAccountsConfiguration`, the share part of the client account
summary, the share business events. So with the flag set to false we end up
with a half-disabled module, and nothing tests that the context still boots in
that state. Let's drop the toggle (`FineractSharesModuleProperties`,
`SharesModuleIsEnabledCondition`, the `@Conditional`s, the 2 properties) and do
it properly in a separate PR once the services, job and APIs live in the module
too, with a test that boots with it turned off.
>
> The docs. ~800 lines of new docs and diagrams have nothing to do with the
module split, and some of it is already wrong after this very PR (see inline).
Separate PR pls, and let's have it match the code.
>
> Thanks.
>
> ###
fineract-provider/src/main/java/org/apache/fineract/portfolio/accounts/api/AccountsApiResource.java:70
> ```java
> @RequiredArgsConstructor
> +@Conditional(SharesModuleIsEnabledCondition.class)
> public class AccountsApiResource {
> ```
>
> > If we want a shares toggle, it has to switch off the whole thing. Right
now `ProductsApiResource`, the dividend posting job and the services are still
registered when this is false. I'd take the toggle out of this PR, see the
summary. What do you think?
>
> ### fineract-provider/src/main/resources/application.properties:264
> ```ini
> +fineract.module.shares.enabled=${FINERACT_MODULE_SHARES_ENABLED:true}
> ```
>
> > Same as above. If we ship this, people will expect `false` to fully
disable shares, and that's not what happens.
>
> ### fineract-doc/src/docs/en/chapters/shares/overview.adoc:66
> ```
> +|Command handlers
> +|`fineract-provider` - `shareaccounts.handler`, `shareproducts.handler`
> ```
>
> > This PR moves the handlers to `fineract-shares`. Same for
`ShareProductDataSerializer` on line 73.
>
> ### fineract-doc/src/docs/en/chapters/shares/overview.adoc:79
> ```
> +NOTE: The module is gated by `SharesModuleIsEnabledCondition`
>
+(`fineract-shares/.../shareaccounts/config/SharesModuleIsEnabledCondition.java`).
When the module is
> +disabled, the shares beans are not registered.
> ```
>
> > This isn't true, only the handlers and 2 API resources are gated.
Thanks @galovics, agreed on both. Pushed on top of a rebase onto develop:
- Toggle removed (`FineractSharesModuleProperties`,
`SharesModuleIsEnabledCondition`, the `@Conditional`s and both properties). It
comes back in its own PR once the services, the dividend job and
`ProductsApiResource` live in the module, with a test that boots with the flag
off.
- Docs chapter and diagrams removed; they go in a separate PR aligned with
the final layout.
The PR is now the module move only
--
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]
