It's clear from the Supporters.pm code that when an hold is attempted on
an already held request, it simply returns the id of the existing hold.
I don't actually remember what behavior I really wanted here. There are
certainly possibility for semantic confusion in the API with the current
functionality, since the API caller must actually check to verify if the
hold they got is a new one or just an existing one.
Perhaps this should be rethought. I left a comment in the test code for
that reason.
if we are unable to fulfill a request, this method turns it into an
indefinite hold on the request.
This design model for handling failure in fulfillment may not be the
best one, but it seemed to roughly fit the behavior and data model we're
looking for.
A little information is lost, but is at least saved in the 'why' field
of the request_hold table.
Requests can now be placed "on hold", and getRequest() can ignore held
requests.
This required addition of a table, and another API call holdRequest().
Tests were not written here, which was a mistake. Unit tests and docs
are needed. A FIXME was added, at least.
Also, minor imporvements to reporting on fulfilled requests.
I debated whether to create a getRequestTypes() instead, but this seemed
reasonable. I am too far out of Perl5 programming culture to know if
this sort of interface is recommended practice.
First of all, I originally thought about releaseDate all wrong. We want
to store the date that the hold was released, which is the indicator
that the request is no longer on hold and can be fulfilled.
We also need tests that assure a request is not fulfilled while on
hold. Those are added here.
I shook out a few other changes to the test ordering that are necessary
for the change to testing holdRequest().
The ledger data can, in fact, sometimes have a missing entityId for
various reasons. For now, these donations should just be ignored.
Perhaps in future a warning of some sort should be generated.
These two new tests:
ok 222 - supporterExpirationDate(): same donation amount in year...
ok 223 - supporterExpirationDate(): ...returns the latter date.
did not pass without this change. The list for annuals in
supporterExpirationDate() was sorted in the wrong order, producing
erroneous results.
Since the emailAddress criterion could find more than one entry, this
change has findDonor returning a list rather than a scalar integer and
thus finding multiple items are ok.
Tests are more extensive now that this API change is in effect.
