Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow lookup of custom AccountancyEntry extensions via Accountancy #314

Closed
cmrschwarz opened this issue Jan 29, 2020 · 1 comment
Closed
Assignees
Labels
module: accountancy Accountancy prio: 2 - medium Medium priority issues
Milestone

Comments

@cmrschwarz
Copy link

cmrschwarz commented Jan 29, 2020

This issue was raised during SWP2019/20 evaluation. By request of @martinmo I'm resubmitting it here. Currently it's quite cumbersome to add custom data to UserAccounts and AccountancyEntry objects. A positive Salespoint example of a solution is the catalog package. The main Catalog's interface type is:

interface Catalog<T extends Product>

This allows the Salespoint user to subclass Product and add all the fields they want. The additional fields are still be managed by the Catalog repository, and especially when you query that repository you get out objects of your subtype, not some generic Product instances.

In contrast, Accountancy and UserAccountManager don't allow this kind of flexibility.

Therefore I suggest changing the type of Accountancy to

interface Accountancy<T extends AccountancyEntry> { … }

and of UserAccountManager to

interface UserAccountManager<T extends UserAccount> { … }

which would make them much more flexible and applicable.

@odrotbohm
Copy link
Member

odrotbohm commented Feb 1, 2020

I think what you suggest is fine for Accountancy. UserAccount is deliberately not designed for extension but delegation: a customer has an account, it is not one. All examples of additional data that folks intended to add to the UserAccount type have been better off in a custom domain type that's then holding a reference to the UserAccount.

Generally speaking, we shouldn't equate "extensibility" with "possible to inherit from" or "easy to inherit from". Extensibility is more often achieved by using delegation or exposing dedicated strategy interfaces so that custom implementations can be used. See LineItemFilter in the inventory module for example. Adding extensibility to the mix in the way suggested adds significant complexity for students that want to use the types as is as they need to understand why they'd have to declare Accountancy<AccountancyEntry> everywhere, which looks like stating the obvious in the first place.

I guess we even have to carefully test this for Accountancy as all other abstractions that use that pattern assume that only a single extension type is used. E.g. if you customize Order to MyOrder, using OrderManagement<MyOrder> only work if all instances added are MyOrder instances. For Accountancy, we already persist ProductPaymentEntry instances. So if a user created CustomAccountancyEntry and used Accountancy<CustomAccountancyEntry>, the call to AccountancyEntryRepository.findByDateBetween(…) would return both the ProductPaymentEntry and CustomAccountancyEntry with the former resulting in ClassCastExceptions as they don't implement CustomAccountancyEntry.

@odrotbohm odrotbohm self-assigned this Feb 1, 2020
@odrotbohm odrotbohm added the module: accountancy Accountancy label Feb 1, 2020
@odrotbohm odrotbohm changed the title Better extensibility of AccountancyItem and UserAccount by switching to generic repositories Allow lookup of custom AccountancyEntry extensions via Accountancy Feb 1, 2020
@odrotbohm odrotbohm added this to the 8.0 milestone Sep 24, 2020
@odrotbohm odrotbohm added the prio: 2 - medium Medium priority issues label Sep 24, 2020
odrotbohm added a commit that referenced this issue Sep 6, 2023
Accountancy now allows handling custom Accountancy entry sub-types. ProductPaymentEntry has been renamed to OrderPaymentEntry.
@odrotbohm odrotbohm changed the title Allow lookup of custom AccountancyEntry extensions via Accountancy Allow lookup of custom AccountancyEntry extensions via Accountancy Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: accountancy Accountancy prio: 2 - medium Medium priority issues
Projects
None yet
Development

No branches or pull requests

2 participants