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