toeb / accounting

3 stars 0 forks source link

#3 added BillTransaction command handler to accounting facade, checks ar... #24

Closed toeb closed 9 years ago

toeb commented 9 years ago

...e performed on billtransaction command wether it is valid or not and if so it is created

Fail Tests are missing however they are hard to perform if #23 is not answered.

Also modifications are necessary if #22 or #21 are merged.

matthiaszoellner commented 9 years ago

regarding PartialTransactions: is there value in requiring PartialTransactions, or would it be a valid choice, to have a Transaction Command without any transfered values?

Context: I think we should allow adding of balanced partials to any (non-reverted) transaction anytime. This would make initial empty transactions somehow useless but acceptable.

matthiaszoellner commented 9 years ago

if (command.PartialTransactions.Any(p => p.Amount <= 0.0m)) throw new InvalidOperationException("partial transactions may not have an amount of 0 or less");

Do we handle reverting differently? Else we need negative amounts.

matthiaszoellner commented 9 years ago

The whole idea of command.PartialTransactions looks suspicious to me. In my oppinion it should be something like a nested command, which processes the necessary input information to a partial transaction. It looks like an anti-pattern, to have full PartialTransaction (as in data layer) objects inside the command properties, when the partials are just about to be created.

// €dit: maybe not a nested command, since it is not an atomic transaction in itself, but something like a subcommand or a list of properties from which the command creates the actual partial transactions.

toeb commented 9 years ago

I thought about creating another datatype that is used to create partial transactions but what would be the point? DRY? WET?

toeb commented 9 years ago

Allowing 0 Partials could be possible. But If you are against a draft transaction then you should be against adding partials to a later date. If you change a transaction you need to reject it and then create a new one.

Yes rejecting a transaction is handled differently and only a rejected transaction may have a negative amount

matthiaszoellner commented 9 years ago

Well what I am against is unbalanced transactions. As long as it is a balanced 'draft' state, there are no real drawbacks to later completion (and no need to handle them differently).

The Problem with unbalanced transactions is, that they are throwing the very basic assumption of 'every piece of money goes from one account to another' away and they can't be in their final state, while a balanced incomplete transaction can well exist in the program without causing technical problems.

matthiaszoellner commented 9 years ago

The Point of an input type for PartialTransaction creation would be, that PartialTransaction has fields which are not requested from the user but filled in the Business/Datalayer. If we don't do this for Partials, we could go without it for all objects, but we decided to take separate data for things like OpenAccount and only return the completed Account object.

toeb commented 9 years ago

Ok. However I believe think we have to talk about the style as this is getting brittle when we have multiple models for the same thing which all have to be touched when performing a change

toeb commented 9 years ago

I actually believe that the model classes should be used as much as possible and I'd like to merge it as is. It is our job in the business layer to validate model objects or to complete information in them.

also taking out the stricter constraints will be easily possible later so I would suggest postponing this until we have a use case

toeb commented 9 years ago

Please let me merge :dancer:

toeb commented 9 years ago

ping

matthiaszoellner commented 9 years ago

Back from HC :)

I'm not generally against exposing datalayer to view, but I don't like the inconsistency between the commands.

@flom please review and decide

toeb commented 9 years ago

I'm not 100 % sure but I think a new Id is generated when using Repository.Create *from data context

toeb commented 9 years ago

but go ahead and add checks

flom commented 9 years ago

I would prefer doing BillTransaction like OpenAccount, meaning only providing 'primitives' as input parameters and not PartialTransaction. The front-end is not allowed to set properties like Created, Modified, Id or Transaction. Those are all created by the BusinessLogic/DataLayer. For the BillTransactionCommand I would suggest something like:

as parameters set by front-end.

toeb commented 9 years ago

Okay I now have only primitives inputs in the command - merge now possible

matthiaszoellner commented 9 years ago

After outsourcing the partial transaction handler into a private static method, ship it.

Doesn't matter if the handler takes the whole list and contains the loop or takes a single partial and processes the loop content.

toeb commented 9 years ago

yes master :P

toeb commented 9 years ago

fixes #3