specified data types

[nonbinary]

Not sure about the data type for the last two here. Also, looking at them, I see I've mixed some languages. Shall fix.

[nonbinary]

The variables here are taken from the autogiro file fields. I'm a bit uncertain as to what they mean by postal address. It's in the specs for post type TK56, along with postal code. Since it's just those two, my guess would be that it should actually be city, but I'm not sure until I see a file.

Edit: Looked at the autogiro test files, and yes, it's city.

[hanneskod]

So I made some changes to DonorSpec to exemplify how phpspec can simplify the use of mock objects. Hopeing this is helpfull! More comments comming...

[hanneskod]

Travis build failed due to one line i Donor being to long. The travis setup actually runs a number of coding style checks.. Not strictly neccesary. But well, good in the long run. Fixed it in the last commit, now it should pass all tests..

[hanneskod]

The setAccount() question is really tricky and interesting! My gut feeling is that setAccount should not be possible. A mandate is in practice a personal id number and an account number. If either of these numbers change it is from the view of bankgirot a new mandate. And well, my gut feeling is that we should model this definition.

But as you point out, if the payer number identifying a mandate is in fact the same number as the personal id (which I think it will be most of the time) then changing the account number is not such a strange notion. The alternative is that old mandates concerning the personal id in question must be removed prior to adding the new mandate. Which means more steps for our app to handle. So why not allow setAccount()?

It might actually be a good thing that changing an account number involves multiple steps in our app. Since changing the account number might involve multiple changes of donor state. If for example we recieve a paper based mandate with a new account number we should revoke the old mandate from bankgirot before adding the new one. And forcing the deletion of the old mandate models this nicely. What happens if we get a new digital mandate with an account number change from the bank? Will we first get a notice of the old mandate beeing deleted? This is not clear from the specs. Maybe what we need is the posibility to keep multiple mandates for the same personal id, even though only one can be active?

So my temporary conclusion is that we should not support setAccount() and that mandates in our db should be identified by a key that is NOT the payer number. (Our key could be a hash of personal id + account number, as this identifies what actually IS a mandate...)

Sorry for the super long reply.. What do you think!

[hanneskod]

So otherwise it looks really good. On the right track! I'm thinking Donor should probably contain simple string fields for phone number and mail as well. As this is commonly asked for by keepers of these kind of records. And a field for the amount to be donated every month (byrokrat\Amount\Currency). That should probably do it...

[nonbinary]

True with the different mandates, didn't even think about what happends when a new mandate overrides an existing one.
But yes - I like the idea of forcing the user to delete the old mandate if a new mandate is to be added. This corresponds to what is actually happening under the hood (ie in autogiro's systems).
This way the user gets a better understanding of what is happening, and can answer question if an actual donor gets in touch with weird questions ("I received a message that my autogiro to you has been cancelled, am I still in your system?").

[nonbinary]

Good point with more contact info. There is, however, a question of scope here. If the fields exist, people will fill them in. If there exists another database with contact info (as in my intended use case), multiple posts will be created. Some of the contact info will be out of date.
But yes, the possibility should exist.

And, of course, donation amount. Slipped my mind. I'm thinking that in the long run, it would be good if it was possible to keep a record of donation amounts. But it might be better to pull that information from actual past donations, instead of a record of past intended donations.

[nonbinary]

Btw: this messy constructor. Does it make sense to initiate the non-vital fields to "", or should they be null? Or just left out of the constructor alltogether?

[nonbinary]

A few thoughts on contact information: The data protection regulation states that one may only store information for a specific purpose. Contact information in this system may only be used for communication regarding the autogiro mandate etc. The regulation encourages "security by design" and such. I think a good way to apply this here would be to attach a separate object, a ContactPerson object, to a mandate. It should be possible to transfer this contact person to a new mandate, but otherwise it would be deleted along with a deleted mandate. The process of adding a contact person, in the end application, should remind the end user that this information should come from the donor, and not pulled from other records. This would make the purpose of the information more clear to the end user, and discourage abuse of the contact information. But for now, I suggest we put that object in a Todo.

[hanneskod]

Since this is a kind of value object all fields should go to the constructor. The way to make it less messy would probably be to create a dedicated address object that wraps all the address fields. That could actually be a good thing. Then we could have an AddressFormatter and other such helpers... also makes for easier mocking...

[hanneskod]

I am fine with a ContactPerson. Could be a good thing for adding organizations as donors, which might have a different contact setup. I don't think I matters that much to the end user though. As this is a matter of internal modeling and not related to presentation.

Feel free to pen an issue in the issue tracker to track this idea!

[nonbinary]

Btw I understood that PHP doesn't accept default values for objects as arguments, so I didn't put the donationAmount in the constructor. I'm guessing that mandate files doesn't specify an amount, so in that situation we won't have a donationAmount. Do you think this should be in the constructor, and handled when we receive the mandate files?

[nonbinary]

I've been thinking about an adress object as well. It could possibly handle sanity checks for postal codes and such.

[hanneskod]

Quick edit to show how default values for objects are handled in php. The amount would typically come with the mandate in our situations. When people sign up as donors in this way they allow not only to be conected with autogirot, but also that a specific sum may be drawn each month. So thats fine..

[nonbinary]

Oh, right! Null as default value, handsome.

[nonbinary]

I installed syntax checkers on my workstations now.

[nonbinary]

I added the uid property to Donor. It's a full sha256 hash, so 65 characters, and not the 8 characters that we were originally intending. I hope this is OK. It is possible to use a substring of the hash, but I'm not entirely sure how to do this and not risk collisions, or if it is even possible. The sha256 method is mainly to avoid malicious collisions, which is very low risk. But I prefer sha256 on principle, as md5 should be abandoned.

[hanneskod]

I added some comments to Donot, just minor things. Otherwise it looks ready to merge!

[nonbinary]

Btw: I tried to have Travis check if the mandate_key fits the expected hash, but I couldn't get it to work. Something about the generated objects for donor_id & account number, they don't behave as I'd expect. Maybe you could see why, it would be good to have proper checks on the mandate key. But for now it looks like it's working as expected.

[hanneskod]

Yupp. Will do!

[hanneskod]

Fixed mocking in DonorSpec. Take a look if you want.. Great work. Time to merge =)

[nonbinary]

Oh, nice with the mocking. Easy to work with.