Fix problems arising from "Consolidate tests"

I updated the IS ready for discussion. Please let me know how to proceed.

I'll bump this one back to the top of the list as I'd like to do it first if possible please.

(1) You're right, the removal is good.

(2) This is related to 🌱 The problem of changes Active . I'm OK with you removing the code here if necessary to pass tests, or leave the todo to resolve in the context of that issue.

(3) Consider this scenario. A user makes a declaration saying we can claim gift aid indefinitely. Later a user makes a declaration seperately saying we can claim gift aid this year. Does the later more limited one win out over the earlier one here? I was thinking it shouldn't because it might be an artifact of a more narrowly focused form or something, but now I doubt that and think maybe the newer one should be taken as a partial cancellation per 🌱 The problem of changes Active . So I suggest we need to revisit this question in that context, but the removal may well be OK. However, even if the removal is technically OK because the circumstances should not arise, I'm not sure whether or not it's good to use this point-of-loading to enforce the cancellation.

(4) The only thing that's making me hesitate about the fields is the commerce store, which has some of the same information. Probably that's just harmless duplication and we should put the fields in. Yes, fine to use address module.

5)

I fixed the start/end dates on Declaration, however on reflection I believe it's still not right.

I'm not sure what you mean. Maybe what you did in
https://git.drupalcode.org/project/gift_aid/-/merge_requests/3/diffs
I overlooked that earlier, there's various things about it that don't seem right to me at first glance.

Thanks

2) Sure I can wait if you like. However in my view, the current code is unacceptable in terms of audit requirements as declarations change their end date in baffling ways without an audit trail. We could safely remove it even if we don't know what we'll use instead.

3) In terms of your scenario, I'd say that it depends. The staff would need to look at wording of the second declaration and interpret it as either "I grant you permission to do X" or "please cancel all previous permissions and do only X in future". If the user makes the change themselves then the UI will force them to be explicit.

In terms of this function, then I don't really see any dependency on the above. We have a function with a clearly stated interface, see below. The function is not currently called except in tests. In this issue I'd like to change the implementation and tests to match the interface - that brings us into a tidy state of consistency. In future, developers can decide to use the function as is, to alter it or ignore it.

   * Gets the active declaration for a context (or related contexts) and charity.
   *
   * If there are multiple ongoing date-based declarations for this charity in this context,
   * then the active one is the end with no end date or the end date furthest in the future.

4) The only overlap is the address. Potentially even it could be a different address - one is for official purposes, the other for warehousing. I agree, let's keep the current interface and I'll put the fields back, as that avoids deleting what we might later want. It's easy enough to remove a field later.

5) Yes, exactly that diff. Sorry I should have asked you to review it. I had the idea that it was just a quick safe fix, but that was a wrong judgement.

It's surprisingly tricky to be 100% accurate in all edge cases. We need to define explicit time points for start and end. Counter-intuitively, perhaps the current storage format is unsuitable - it should only store day divisions in UCT. The tax year end is not such a point, being during DST. If the donor wrote a letter containing some explicit dates, then we need to choose a timezone in order for it to have a defined point in time. Given that Gift Aid is UK, we might reasonably expect Europe/London. Same as above, we can't actually store that during DST.

My best idea so far is to ignore the STORAGE_TIMEZONE convention, and interpret our date as being in London timezone.

2) Sure I can wait if you like. However in my view, the current code is unacceptable in terms of audit requirements as declarations change their end date in baffling ways without an audit trail. We could safely remove it even if we don't know what we'll use instead.

For right now, I don't mind which you do. But the MR is weird in that it reintroduces $later, but also removes $newer. That will definitely produe bad results I would think. Originally bother were present.

(3) getActiveByAnyContext

If 2 declarations had the same end date, we should prefer the newer of the 2.

(5) The dates in Declaration entity.

I'd have had 3 concerns with the MR:
- it used \Datetime to get the current time, not Drupal's time service. That's untestable and not best practice.
- it seemed to remove the edge case handling associated with this comment:
// The declaration ends at the end of the day, not beginning of the day.
- I thought that anytime we stored a date in Drupal we needed to update the timezone to the storage timezone before we format it using the storage format. Because when it gets loaded it will be assumed that the stored string is in the storage timezone. Seems dangerous to break that assumption for any custom code accessing these fields.

I like your idea that maybe assuming Europe/London is a good rule of thumb in this module. And your point that the tax year end is in BST might be important.

My current thought is that getStartDate() and getEndDate() should rebuild the timezone to Europe/London:
$formatted = $stored->format('Y-m-d H:i:s');
$new = new DateTime($formatted, new DateTimeZone('Europe/London'));

Additionally, getEndDate() should use "23:59:59" as the time.

it used \Datetime to get the current time, not Drupal's time service. That's untestable and not best practice.

Good point, fixed

- it seemed to remove the edge case handling associated with this comment:
// The declaration ends at the end of the day, not beginning of the day.

This handled by the less-than, whereas start had a less-than-or-equals

    return ($end < $today);
    return ($start <= $today);

I thought that anytime we stored a date in Drupal we needed to update the timezone to the storage timezone before we format it using the storage format.

My current thought is that getStartDate() and getEndDate() should rebuild the timezone to Europe/London:

Additionally, getEndDate() should use "23:59:59" as the time.

OK, done. My reasoning was that setting the timezone has no effect when we are dealing with a date without a time. However I can see that it wasn't always going to work as I expected, so I've made everything explicit again.

Yes, fine to use address module.

I raised 📌 Use drupal address module Active

If 2 declarations had the same end date, we should prefer the newer of the 2.

I believe the complexity of your preferred interface will prevent optimising to DB queries in 📌 Improvements to DeclarationStorage Active . Sorting by declared date we can do. Sorting by end date with special handling of NULL, probably we can't, see https://stackoverflow.com/a/12767777 comment by user330315. As I pointed out before, this function is currently not used, so I feel it's premature to keep spending time altering it.

As I pointed out before, this function is currently not used, so I feel it's premature to keep spending time altering it.

Agreed. Can you leave a todo flagging the question and otherwise leave the code in whatever state you think best?

Thanks.

Can you leave a todo flagging the question

Done

otherwise leave the code in whatever state you think best?

That's exactly the description of this issue😃. Not necessarily a final resting point, but at least a bit more consistent.