Rename months_expired() to months_expired_at_return() and have the
method return an integer representing the month count rather than an
enum-like so that the API user has more flexibility with respect to
what they then do with the result. Thanks to Brett for the review
that suggested this, and for much of the clever math in this change.
The results produced by returning_report.py (which is updated here to
use the new API) are almost identical to the results produced before
the change. In rare cases (about 2-5% of the time) a supporter's
lapsed month count will fall into an adjacent bucket instead of the
one it fell into before, usually because the previous result was wrong
to begin with (due to the ugly days-per-3-months table that we used
previously, which this change thankfully eliminates).
A reporting script similar to status_report.py that uses the new
returning supporter method (added in 7aaba96) to provide a list of
how many supporters have started supporting again after lapsing,
bucketed by the number of months for which they had been lapsed.
A new public method, months_expired(), which is similar to the
existing status() in layout, but instead of returning whether the
supporter is new/active/lapsed/etc., it checks to see whether the
supporter is returning after having been lapsed, and if so, returns
the month range corresponding to how long they had been lapsed before
returning as a supporter in the current month.
These tests lay out the basic functionality for adding an email error.
BTW, I decided we need an entirely different but mirrored setup for
postal errors, since the database tables for email and postal addresses
are hard-coded.
Requests on hold can never be fulfilled.
If you want to fulfill a request that is currently on hold, the right
semantic is that you should remove the hold, then fulfill the request.
Note that this test now passes and it didn't before:
fulfillRequest: .... but undef is returned when attempting to fulfill a held request.
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.
