The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Exploring in #3535158: Declaration states I discovered certain problems with the current approach to validity. We would like to store explicitly the staff assessment of whether a declaration does or does not strictly need confirmation.
Proposed resolution
Change the validity field states to be:
- Invalid
- Valid if confirmed
- Inherently valid
Remaining tasks
These two from original IS now moved out of this issue:
(2) for related reasons, retractions need to handled seperately from regular cancellations and stored as their own validity state.
(3) We shouldn't assume at the API level that a cancellation during the confirmation period is a retrospective retraction. It's possible when validating a declaration years later, that the confirmation would prompt a donor to wish to cancel some but not all of the past years rather than cancelling them all. Even if the UI doesn't support this, the API should.
User interface changes
API changes
Data model changes
Issue fork gift_aid-3536681
Show commands
Start 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
jonathanshawComment #4
adamps commentedMy first point is regarding issues and features. Where possible I feel that we should generate MR related to features with a defined spec in the IS.
We have #3505790: Declaration cancellation for cancellation which refers to #3512674: Flexible contexts and donors for a very long detailed discussion. The current cancellation code in 1.x branch is not especially meaningful or correct, as it predates many important discussions. If you are not happy with the results of the previous discussion then I would suggest to continue it in the other issues although potentially we will never finish this project if we keep changing our mind😃. It seems that you are now using "retract" in a different way than you did in the IS of the other issue.
I feel that cancellation is an important, big, complex feature. I wouldn't want to mix it with anything else, I wouldn't want to do it in parts, I would do the other issue in entirety when we reach the appropriate point.
Then in the roadmap we have a bullet like this:
It would make sense to me to turn this into an issue which again I would class as a feature request. We could have a discussion on that issue about how to do it.
Comment #5
adamps commentedMy second point is an attempt to respond more directly to what you have done here.
I'm mostly confused about the problem/motivation in the IS😃.
1) I believe that we are not currently updating the validity field when the confirmation period passes.
2) According to my understanding of the cancellation issue, neither type of cancellation has a validity state. It's simply a restriction of dates, which could include back-dating the cancellation so that it was cancelled on the day it started.
3) I agree with your first sentence, and I don't believe we plan to make such an assumption.
And I don't understand many of the changes in the MR. It's not clear to me how they relate to the points in the IS. The code is as it is for a reason, and those changes seem to break things. I'm not attracted to the idea that interface concepts are different from the underlying state - especially
isValid(). I don't like the idea that invalid is different from a negation of valid, as I feel that's confusing. I don't really see a need for confirmed date in addition to a claimable date - already there are about 6 dates, and it's quite complex.I guess I don't really get what you are trying to do here, sorry.
Comment #6
jonathanshawDone: #3537036: Written confirmation
Makes sense. And I agree that the retraction related stuff in this MR can be split out.
Which leaves us with the residue of this MR, which basically boils down to this:
Although there's nothing in the code currently that explicitly changes the validity field when confirmation is sent or 30 days after confirmation is sent, the fact that one of the validity constants is named DECLARATION_CONFIRMATION_MISSING and the isValid() method is only concerned about the DECLARATION_VALID constant, suggests an expectation that the validity field does get updated at some point.
And I've realised that this is problematic: it's better to leave the validity field to store the staff's judgment about the validity (which they could change their mind about) and then have more sophisticated logic in isValid() to create a computed judgment of validity that combines the staff's assessment of the evidence with the systems knowledge of the confirmation.
So this is technically a feature ( #3537036: Written confirmation) we haven't yet, but we have existing code that seems to make assumptions related to it.
I could update the IS and MR here accordingly (removing retraction) if you think sensible. It would effectively then be a first stage of #3537036: Written confirmation.
Comment #7
jonathanshawSorry, cross-posted with you.
I'm sympathetic to both of those points. But we do need to distinguish between:
- the staff's evaluation of the validity, which we don't change programatically
- the overall effective validity considering the staff's evaluation and the known facts about confirmation
then we do end up with something like what I've suggested here.
Basically the staff need to choose between "Is this declaration valid without confirmation, valid only with confirmation, or totally irredeemable?", and be allowed to change their minds. But in various contexts the system needs to know if the declaration is good, (e.g. when establishing do we have a working declaration for this donor), which requires computing something based on considering both that staff evaluation and the sent or not sent confirmation.
We could call that isUsable() and isNeverUsable() instead of isValid() and isInvalid().
Comment #8
jonathanshaw(p.S. I confess storing retraction as a validity state is a bit weird and not entirely in keeping with the spirit of some of the other things I said above, but it seemed to work in all the edge cases I considered. Happy to leave that aside).
Comment #9
adamps commentedGreat this is becoming clearer.
The current code is intended to work like this. Staff recording a declaration have 3 options:
What I had expected is that when we send the confirmation (which could be triggered automatically immediately by email, or could be later manually by post), we do 3 things:
I believe that from the above information we can distinguish all the different states as required. We can tell the staff's assessment either that validity was reached by means of a confirmation vs inherently valid by checking if there is a "confirmation sent" date. Staff can indeed change their minds by editing validity and if required other fields (which in some cases we could automate, e.g. can only have a claimable date if also valid). We can tell when it's OK to make a claim. We didn't need any background cron job to change validity after a time period, we didn't need complicated logic in the
isValid()function, and we didn't create caching problems by means of a computed field that is date dependent. The key information is present directly as fields which can be used in views: view all claimable, or view all confirmation required.If we get a cancellation, then we have two options. Ordinarily we set the "cancelled on" date to today, or whatever future date was indicated in the evidence. In case of a retraction (cancellation within 30 days of confirmation letter, implicitly or explicitly stating the declaration was in error) then we set the "cancelled on" to match the start date. The declaration can therefore never be used to make a claim, however its entire history is still evident from the various fields, and we retain the possibility for staff to change their mind.
How does that all sound to you?
Comment #10
jonathanshawSorry for the delay in replying Adam, I was away.
I understood the plan you laid out above in #9, yes. It was a good plan. I thought it was rather elegant.
But what I found when I interrogated the edge cases was that this plan was not completely robust in all circumstances. In some of the edge cases it caused extra complexity that seemed likely to outweigh its simplicity. As if it was doing too much with too few building blocks, in a way that was elegant in the common use cases but got confusing and tricky in edge cases; and we likely end up having to think about edge cases even if we don't implement them.
This is a nice example. In the most obvious use cases that works fine. But what happens when people want to send confirmations even if not strictly necessary? Then it's not possible to use the fact of the confirmation to make a conclusion about the staff's assessment of the validity.
All I'm suggesting here is a simple change:
Confirmation required, and not yet sent (= confirmation missing)
becomes
Confirmation required (whether or not sent)
And instead of programatically changing that validity field when confirmation sent, we have a computed method of some kind instead.
I appreciate that potentially causes downstream problems for things like views that might want to query by validity, but I'd rather we deal with those problems (likely by storing the computed value in an additional field, a simple solution) when we know better what the problem is there that needs to be solved. For now, it seems to me that storing the human-provided data in the most granular way possible creates the most robust foundation for the various needs.
Comment #11
adamps commentedOK, thanks for explaining. Sure, I can see the advantages you refer to. So the 3 states would be
I adjusted the names slightly, looking for something that conveys the idea that the second one would in the majority of cases be valid, as normally we would sent confirmations promptly.
At the UI level, for the status field, then I feel that we still need to stick with the original meanings - the last 5 states are all sub-cases of valid, so we don't really have a way to convey the exact details of how it came to be valid.
At the API level,
isValid()would map to a simple calculation: "Inherently valid" OR ("Valid if confirmed" AND "Confirmation sent"). I agree that we could make a computed field for this later, however we don't need to now.Comment #14
adamps commentedIf you aren't able to review fairly soon then I likely will need to commit so that I can continue on other issues, and then you can review after
Comment #15
jonathanshawI'm on retreat for another week, so please do commit without review. Thanks Adam.
Comment #17
adamps commentedOK thanks. You could do a bulk post-commit review again - it seemed to work well last time.