Follow on from #3504041: Consolidate tests. Search for all @todo XX and do something about them. Several of the larger problems have their own issue - this just covers the simpler ones that should be quick to resolve and have left the code in an inconsistent state.
1) A: In GiftAidRelatedContextsSubscriber I removed the test against a draft order because for sure the order is draft during the checkout process when looking for an existing declaration.
J: Your point is valid, but I'm not sure a past declaration on a draft order should be considered to have been truly declared.
A: The code I removed was preventing a draft order/cart from picking up a fully declared past declaration. I believe what you refer to is a different case, covered by line 75.
->condition('state', 'draft', '<>')
2) A: disabled the code in Declaration::postSave() for "If this declaration has had its end date edited, force the same end date on other ongoing declarations for the same context."
J: Your points are good, but I had a good reason we should consider how to address.
A: OK, please explain the reason.
3) A: I simplified the code in DeclarationStorage::getActiveByAnyContext() so it just returns the most recently declared, and removed the corresponding tests. It seems that it shouldn't really matter which one we return if multiple are relevant, and I couldn't conceive of any rule relating to start/end dates that actually works reliably.
J: That code looks ok to me.
A: Yes the code was perfectly good before. However to resolve item 2) above or also #3505387: Improvements to DeclarationStorage I need to alter it. I just noticed that the interface says this:
* 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.
Can I alter the code and tests to match that? Or I could alter the interface to match the current code (most recently declared).
4) A: Charity has accessor methods for some fields that no longer exist so I removed them. The alternative of course would be to put the fields back.
J: I'm not sure what's right here, that might get clearer. I suspect we need to display the charity number and address to users at some point.
A: Legally, the official registered name seems to be recommended identifier, so I guess we should keep that as a required field. We could add various other optional data like address, number, logo, description that might be commonly used - or we could just leave the site owner to add their own fields. What do you prefer?
Wherever we have an address, I feel that this module should use the proper address field (add dependency on drupal/address). The address is a key part of every declaration, and the structured field greatly increases the accuracy of entered data. Anyway the address module is a dependency of Commerce already.
5) I fixed the start/end dates on Declaration, however on reflection I believe it's still not right.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
adamps commentedI updated the IS ready for discussion. Please let me know how to proceed.
Comment #3
adamps commentedComment #4
adamps commentedI'll bump this one back to the top of the list as I'd like to do it first if possible please.
Comment #5
jonathanshaw(1) You're right, the removal is good.
(2) This is related to #3505790: Declaration cancellation. 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 #3505790: Declaration cancellation. 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'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.
Comment #6
adamps commentedThanks
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.
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.
Comment #8
adamps commentedComment #9
jonathanshawFor 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)
getActiveByAnyContextIf 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.
Comment #10
adamps commentedGood point, fixed
This handled by the less-than, whereas start had a less-than-or-equals
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.
I raised #3506166: Use drupal address module
I believe the complexity of your preferred interface will prevent optimising to DB queries in #3505387: Improvements to DeclarationStorage. 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.
Comment #11
jonathanshawAgreed. Can you leave a todo flagging the question and otherwise leave the code in whatever state you think best?
Comment #12
adamps commentedThanks.
Done
That's exactly the description of this issue😃. Not necessarily a final resting point, but at least a bit more consistent.
Comment #14
adamps commented