Closed nonbinary closed 7 years ago
Yeah cool. Se my comments in #59. I would like the payer-number/mandate-key to be an argument, so that sort of leaves out the id + account path. But otherways I'm all positive.
This now accepts donor key or donor payer number. I put in a flag to force checking by donor key. That is there in case there is a collision, where a donor key would be identical to another donor's payer number. But I'm not 100% sure how the hashids work, is that an actual risk?
Not sure what to say about the CI here. It seems I don't have those tests on the machine I'm working on atm. I do at home. Will look through it later.
Yep good thinking. Collisions are highly unlikely. But since payer number can be any number, and mandate key can consist of only numbers it is technically possible at least..
Will review code later..
What exactly is the difference now between the two pull requests? Close one if not necessary??
Yeah, the difference is rather minimal now. I prefer this branch, so I say I close the other.
This branch has a function to fetch the donor from the database, and this function chooses to use donorkey or payernumber. The other branch just uses payernumber instead of calling the function. I feel this code is cleaner as well.
Some minor comments. Otherwise are you ready to merge?
Yes. I was trying to get the feature tests up and running for this, but I don't think I grasped how to work with those. So I'll add that later, when I understand them better.
So well done. Merging.
Another way of working with #55 . In this version, it's possible to specify what donor the user wants to edit in more ways. Here the application looks for, in order:
If none of these have been supplied, it asks for a payer number.