Dates need reworking, inc. Handling of adding a "Yes past 4" declaration assumes today, ignores start_date
Note: this imported from https://github.com/mattwire/uk.co.compucorp.civicrm.giftaid/issues/28 and https://github.com/mattwire/uk.co.compucorp.civicrm.giftaid/issues/15 since they were going in the same direction.
When a yes and past 4 years declaration is added, it's supposed to trump an existing yes donation that's within the "past 4 years" period.
This period should be the 4 years leading up to
start_date but is actually the 4 years leading up to the current date.
This is also related to github issue 15 because it's a case when we muddy the definition of what a declaration record's purpose is.
Ok. I think the sensible next step is to add a "valid from" date into the declarations and an upgrader/api to back-calculate for existing declarations. That will help us simplify some of the code going forward because we can just check if Yes/Yes4years and read the "valid from" date instead of working out the 4 year interval each time we check elibility.
Agreed. (I also hope to move the logic into SQL one day, which should significantly speed up searches).
In an ideal world I might rename the fields:
given_date- Date declaration was given (currently
valid_from_date- Date person tells us they were eligible from. So on a contribution form, this is limited to the day they hit submit, or exactly 4 years previously. But on the back end, it would be possible to correct it with actual dates.
valid_to_date- Date person tells us they are not eligible (like
end_date). I'd suggest that this
However, this will definitely upset any other code (+views/rules) that used the old names, and will make it much harder to maintain an upgrade path from the compucorp one. So maybe I just add a
valid_from_dateas you suggest, and leave
start_datesomewhat misleadingly as the date the declr was given.
Also, I've not tested the existng pattern yet, but I'd suggest that the period that is valid is, in mathematical notation
[ valid_from_date, end_date [or
[ valid_from_date, end_date )meaning that if an end date is 17 May, then contributions on 17 may are NOT covered. This gives us a way to set the valid period to zero, which could be useful - e.g. we got a NO declaration today and tomorrow we got a YES - 4 YEARS one which overrules the NO completely.
Merged other issue
The custom fieldset that stores declarations is used for a mix of purposes, which leads to information that can't be audited without guesswork.
The 2 purposes it's used for are:
Storing exact transactional details about a received declaration - for example 'source' and a scanned copy of the declaration.
Storing state data that can be used to determine eligibility over time periods.
So if someone (e.g. HMRC) says: show us when this person declared their eligibility, things get difficult.
- 1 Jan 2019 Person makes first ever donation, says they're eligible for now (and going forward). Source is "donate for X".
- 1 March 2019 Claim is made
- 1 Sep 2019 Person moves house
- 1 Jan 2020 Person makes another donation, says they're eligible now and since 4 years ago.
This will result in a single declaration which will have:
- 1 Jan 2020 as the date
- the source (and possibly scanned signature) from the original declaration
It is now impossible to say what happened: apparently there was a declaration made in Jan 2020 but the the source of that shows a campaign from a year before it and an address they no longer live at, even though we have an updated address for them now in their contact record. The scanned signed declaration doesn't seem to fit either, perhaps the scan doesn't say anything about "past 4 years", so that looks like an error.
My opinions on this are that:
Every declaration should be kept
Don't merge two+ declarations. Do store multiple declarations if multiple declarations have been given. Each one backs up the veracity of the information and this gives a full and accurate record of what the donor declared.
Declarations should have clearer dates
"Start Date" is used but does not describe the start of a period of eligibility, in the case of "and 4 years prior" declarations. It's actually the date that one of the declarations was made.
I think declarations should have three dates:
- Date declaration received
- Date declaration is valid from (i.e. typically date received or 4 years before, but outside of the simplified yes/no eligibility forms, it ought to be possible to record a declaration for any given period)
- Date declaration is valid until (usually NULL to mean until further notice).
Addresses should not be stored with declarations, but should be kept with claims.
I might be wrong about this one, but here's my understanding: HMRC checks people included in GA claims by comparing their current known address for tax purposes with the one submitted. So the one that gets submitted should be the most up-to-date address, not the address they had when they signed the declaration.
Duplicating address data in declaration records seems to cause more problems that it solves and I can't see HMRC being interested in old addresses.
I think it's a nice feature to record a copy of the address that was used for a claim, though, since then if HMRC come asking "you've said they live at X but this doesn't look right to us" then we can say "Ah, yes, they did live at X at the time we claimed, but they're now at Y".
@artfulrobot I agree the way these are stored (and how easy they are to change/overwrite) is not great and should be reworked/improved. I've separated most of the code that handles declarations into it's own class
CRM_Civigiftaid_Declarationwhich should hopefully make refactoring/improvements easier.
I think declarations should have three dates..
Agree. This would also simplify the code quite significantly because you could just look at the valid from date instead of checking if it was "Yes, past 4 years" or just "Yes". It would also be quite an easy thing to update via an upgrader step.