More robust validity

My 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 🌱 The problem of changes Active for cancellation which refers to 📌 Figure out contexts and donors Active 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:

Written statement. There is a field for Staff to mark a written statement as sent on a specific date. There isn't any mechanism so send one which could be via email or generating text for a letter to print or save as PDF. There isn't any further audit information such as which template version was used (could be a config entity).

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.

My 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.

Then in the roadmap we have a bullet like this:
Written statement ...
It would make sense to me to turn this into an issue which again I would class as a feature request

Done: ✨ Written confirmation Active

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.

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 ( ✨ Written confirmation Active ) 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 ✨ Written confirmation Active .

Sorry, cross-posted with you.

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'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().

(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).

Great this is becoming clearer.

The current code is intended to work like this. Staff recording a declaration have 3 options:

1. Invalid or if you prefer "never valid"
2. Valid: all the parts are present and no confirmation is needed
3. Confirmation required, and not yet sent (= confirmation missing)

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:

1. set "confirmation sent" date to the current day
2. set the "claimable" date to 30 days from today
3. set the validity to valid - because in the absence of any cancellation, then we have done everything that we are required to do

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?

Sorry 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.

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.

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.