Invalidation as a separate process from Cancellation

Sorry wrong issue.

For staff

Potentially we don't need a new route. It's just a matter of editing a declaration and changing the validity. The only catch is that the validity field will be locked. We could have a checkbox for invalidate that maps to a change in validity inside the forms save.

There is a similar case if staff manually record a confirmation (e.g. sent by Royal Mail). They would need to alter confirmation_date, but that's locked. And for good reason, as the don't want to allow removing the confirmation_date from a claimed declaration.

Another solution then is to make the locking more subtle: some field transitions are OK, some are blocked. We could compare with original in the verify function.

For self

I guess we need to duplicate gift_aid_user.context.cancel including everything that follows on, and everything that links to it. Should we also have a link to invalidate on the Gift Aid tab, if there is a suitable declaration? It would be a bit hard to explain/describe it. But if we don't, then only way to access it is via the email link. Perhaps the donor distrusts the legitimacy of the email and prefers to navigate to the site themself (sometimes I might do that).

I think it's simpler (and fine for 99% of use cases) to limit the focus to a single declaration.

Do you mean we don't need a route parameter that takes a declaration id? What about the other 1% of the time😃? I guess we invalidate them all??

I realise that I was just a moment ago in favour of the above, but now I look at the duplication and extra complexity I am having doubts. We could just put a simple choice on the cancel page, as described in #3570296: Improve self-cancellation UI. Most of the time there wouldn't even be a choice as only one option would be available.

• I am no longer eligible for Gift Aid. Please don't claim Gift Aid on future donations.
• The Gift Aid declaration record on this site is incorrect, please delete it.

Actually we can keep #3570296: Improve self-cancellation UI separate, we don't have to do it now.

OK we can do it like this:

1. Create a new validity state DECLARATION_INVALIDATED. That's rather similar to DECLARATION_INVALID, so we could instead say denied, rejected, or refuted. ("Invalidated" potentially doesn't carry the specific meaning relating a negative of a confirmation??).
2. On CancellationForm, remove the "Retroactive" checkbox. Put an explanation that this form is only for forward cancellation, and please edit the declaration itself for invalidation.
3. Remove static from RecordBase::getCriticalFields() (we don't use it statically anyway). Change Declaration::getCriticalFields() so that it adds 'confirmation_date' only if isConfirmationSent(), and adds 'validity' only if isClaimable() is TRUE.
4. Change DonorCancelForm to remove $startDate. Edit getDescription() to use the text from the final 2 bullets in the previous comment. In submitForm(), if $this->retroactive</ code> is TRUE, then instead of creating a cancellation, we find all declarations with <code>canRetroactivelyCancel() and set them to invalidated.

In A&B you seem to be against DECLARATION_INVALIDATED and in C-E in favour, so I'm a bit confused. I absolutely agree that we wouldn't use DECLARATION_INVALIDATED for retroactive (backdated) cancellation - perhaps that's what you are saying too?

I would like to create clarity in our discussion in terms of data model vs user interface.

reporting a cancellation (staff/self/from-confirmation-email) is always something that happens at the donor level, not the declaration level

Good idea. I agree with your suggestion that at a UI level in the main-line cancellation should be done via the donor.

However we don't necessarily need to eliminate more subtle choices for the admin. If they wished to invalidate one declaration specifically but not any others, then they could potentially still edit that declaration. We wouldn't have to write any extra code to allow it - rather we'd have to write extra code if we wanted to block it.

use the ordinary cancellation form for ordinary donors coming from a confirmation email to deny confirmation

This matches what I was suggesting, good.

when ordinary donors cancel or invalidate, the start date is forced to now, they don't get allow retroactive anyway, that's a staff thing.

Absolutely I agree, donors don't get to choose a date. This is already true, and I have no wish to alter it. Even if we do #3570296: Improve self-cancellation UI they still don't get to choose a date.

- always create a cancellation entity, it's a sensible and consistent audit trail
- whenever a cancellation is recorded, by any means, update any declarations whose status is invalid without confirmation and for which a confirmation was sent within 30 days, and change declaration status to invalidated.

These 2 seem to imply that at a data-model level, we would disallow invalidation without cancellation or vice versa. It potentially creates a limitation built into the design that would be difficult to reverse.

What if a donor calls and says:

1. the old declarations are fine, but your email reminded me that I've just stopped paying tax so please don't claim in the future
2. OR I believe that specific old paper declaration from 2013 you just emailed me about is a mistake, but my electronic self-declaration from 2019 is fine, and please keep claiming in future.

I feel that

1. at minimum the data-model should be flexible to allow both cancellation and invalidation separately from each other (and even to invalidate a specific declaration).
2. it could be useful if expert Admins can access this flexibility using the UI
3. if you prefer that Staff and Donors can only choose to do both together that seems a reasonable limitation

Another point is that currently the design supports flexible contexts (which you have been very enthusiastic about😃). If a cancellation is discovered to be on the wrong donor, it can be moved and everything automatically recalculated in calculateCancellation() - the calculation takes no function arguments and is not time-based so can be rerun with identical results at any time. Likewise, donors can potentially be merged or separated.

The changes here would lose the possibility to recalculate. We wouldn't know whether or not to invalidate, as wouldn't know what the declaration state was at the time of cancellation. Cancellation is global to the Donor, but Invalidation is specific to certain declarations, normally precisely one. So there is a good reason for storing them in the corresponding location.

So in summary of a long comment, I propose to keep the data model as I described it, however I am supportive of your suggestions to simplify the UI. If you are happy with that, the main remaining item is to clarify what each role (Admin/Staff/User) will see on the cancel form:

1. Is there a choice for cancel/invalidate/both? (The choice would only be shown where multiple different behaviours are possible.)
2. If not, does the UI wording vary depending on which of them will actually take place? E.g. see DonorCancelForm::getDescription()

OK I have had some more ideas. I believe I see a way to use a cancellation entity to include the case of invalidation while avoiding the problems I've mentioned above.

In the normal simple case for invalidation, there will be precisely one recent declaration with a recent a confirmation (both within 30 days). The guidelines state that cancellation should be be "as if never made". We can handle this in Declaration::calculateCancellation(). We add a new test for $cancellation->getReceivedDate() is between 0 and 30 days after $this->getConfirmationDate() and validity == DECLARATION_VALID_IF_CONFIRMED, and if so set $result = $this->getStartDate() (and we might as well break from the loop). NB we don't change validity - it remains at DECLARATION_VALID_IF_CONFIRMED. In this way, I believe we keep the flexible contexts, and the ability to recalculate later and get the identical result. We are safe no matter what order records are entered and for example if recovering a backup on a new server, at the end we can just call calculateCancellation() on everything.

I believe this code will satisfy your requirement C. B is easy - we just make a cancellation with a start date in the past. E will work the way you are asking too.

However, as I pointed out in my previous comment, I'm not really convinced that the world is always going to be as simple as suggested by E.

1. If we have a "years old" declaration with a recent confirmation, the Donor might make a clear request for a proactive cancellation.
2. If we have multiple declarations each with a recent confirmation, the Donor might make a clear request to cancel just some.
3. If we have a "years old" declaration with a recent confirmation, and another overlapping one that's solid, then the Donor might make a clear request to retroactively cancel the former but keep the latter.
4. If we have a "years old" declaration with a recent confirmation, and another overlapping one that's solid, then the Donor might accept our offer to cancel Gift Aid "as if never made". Then they are surprised or even upset that we are still claiming on the solid one - because throughout the entire process we never mentioned it - and HMRC chase them a penalty. Sure, hopefully we have solid evidence, but it doesn't seem good for our relationship with either the Donor or HMRC.

This reinforces your comment, that some of the cases here are hard. I'm not proposing to solve them all right now, but I would like to have at least an idea that our design can support a solution later.

1) We could have a flag on the confirmation entity for "proactive only" (and perhaps another option "retroactive only").
2+3) We can set the validity to invalid or invalidated on specific declarations. Or if you prefer, have cancellations with a reference to a specific declaration (but that seems harder to me).
4) Perhaps we shouldn't send out the default confirmation mail in these cases, but instead leave the staff to resolve the process manually?

So overall I believe this design works and it matches your preferred outcomes.

I raised #3573666: Flexibile cancellation and #3573667: Staff cannot record a confirmation for things that came up in the comments.

Ready for review please.

I feel that #3573666: Flexibile cancellation is a good place to continue discussion of your concerns from #17. Yes you are right about DonorCancelForm::getDescription() and this is similar to point 4) of the above issue. I pasted your paragraphs from here onto that issue. Also there is #3570296: Improve self-cancellation UI which could be a good place for the donor UI side of things.

we seem to have lost the ability for an admin to retroactively cancel gift aid for a time period

I believe it's fine. In our new terminology, retraction as per #3557360: Support retraction is not an invalidation, but a backdated cancellation. This feature is not required by HMRC only by human kindness. If the admin clicks the new override button, then they can enter a start date in the past. They could also enter an end date.

OK to commit here, and continue on the other issues?

The retro element was something I put in speculatively in #3560177: Safer admin overrides then we agreed it was wrong and we would remove it here.