mattwire
commented
4 years ago @artfulrobot It sounds like a typo in the code. The dates themselves should not overlap. The responsibility for valid declarations is with the client not the organisation (apparently) so if someone says they are eligible then says they are not then says they are again (I think) we should record that and use the "latest" matching one. Eg. if they said "No" then said "Yes and past 4 years" within 4 years of the "No" we'd use the "Yes" answer.
artfulrobot
The code here:
https://github.com/mattwire/uk.co.compucorp.civicrm.giftaid/blob/884d55be57023d5f51fd4bd6c262afb87be697c5/CRM/Civigiftaid/Declaration.php#L330
goes to the trouble of looking up a future declaration, but then in line 358, where it would be useful to consult the results of that, we instead look at the incoming
$newParamsarray.I think it probably meant to look at
$futureDeclarationinstead.Although it looks like a simple mis-type in the code, I've not done a PR for this though because I'm finding it hard to understand when this would be useful, and how the integrity of the system holds up in this situation.
e.g.
With the fix, the negative declaration would have an end date of 1 June 2020, but actually it's overridden by the 4 years prior selection for the future declaration. This is ambiguous and impossible to resolve without speaking to the person in question.
With the fixed code, we'd end up with a situation where the eligibility checker - the code that prepares to make claims - could stumble across either the negative one or the positive one, depending on what order things come out of the database in; both describe eligibility for an overlapping period.
This is the sort of case where I don't want to move without writing documented unit tests because opinions on what should be done will be many!