Safer admin overrides

An important case of this issue is "retraction" - i.e. creating a cancellation with start_date far in the past when canRetroactivelyCancel() is false. I've closed #3557360: Support retraction as a duplicate of this.

As a preliminary step, we should check what limits should apply to staff. In particular, I suspect that at the moment they can do retraction, so we need to block that.

In terms of a UI, I feel attracted to something like this, which resembles what I've seen quite often in shops, and is similar to what you wrote on the retraction issue.

• Fix the staff UI so that there is some indication of the restricted status. For example on the declaration edit form, we can print a message "key fields are locked". It could also be useful to show the locked fields in a disabled state rather than simply removing them. For the retraction case, the message would be more like "Gift Aid has already been claimed. Minimum 'cancelled from' date is XXX."
• By default, admins have the same UI as staff. However they have an unlock button/checkbox by the locked message.
• The unlock button causes the fields to be open and the message to change to something like "Warning: you have disabled the safety checks. You can make changes that invalidate the audit trail of submitted claims".
• I can see a few possible implementations. 1) Use javascript and conditional form state 2) Use Ajax to fetch an updated form with all the fields open. 3) Redirect to the same URL but with an unlocked query parameter, which fetches the form with fields open (but would lose existing edits?).
• What happens in the shop, but I doubt you need is: when a staff is logged in then a supervisor/admin can unlock by typing in their password.

The is an existing mechanism for delete that is in effect also an admin override. We can investigate integrating that into this new framework. E.g. we could disable the delete button when the form is locked.

When an override is submitted that potentially alters claims that have already been submitted to HMRC then it would be useful to set a flag on the declaration revision.

Proposed new restrictions for staff (i.e. not yet coded - not necessarily new concepts):

1. We should lock cancellations the same way that we already do for declarations. Otherwise editing an old cancellation could invalidate an already claimed donation.
2. Cancellation start_date can be at most XX days in the past unless canRetroactivelyCancel(). I guess that probably STABILITY_WINDOW (7 days) is too short for XX - if staff are on holiday for a week then they would be too late. 30 days feels good to match RETROSPECTIVE_WINDOW. Then the admin should avoid claiming any donation until 30 days have elapsed in case there is a cancellation sitting on a staff desk. Already we knew they should wait 30 days in case of confirmations so it fits in.

For implementation, I quite like the approach I suggested in the other issue:

Yes this is what I meant in my option 1)

No custom js or ajax needed.

Apologies, my reference to js was confusing. I was referring to the fact that #states is implemented in terms of js, so without any contact with the web server - in contrast to the other options which use Ajax or a new page request and so do contact the server.

I would be very happy to use solution 1). However when I come to write it, I see a possibility that it doesn't work out easily as #states (hence not contacting the server) is fundamentally less flexible than AJAX (which does contact the server). So I'd like to keep AJAX on the table as option B in case it turns out to be both quicker and better UI.

The #states method mostly works well. AFAICS it can't remove the CSS class that highlights the locked fields. This means that after the admin selects to override, the locked fields are still visually highlighted (currently I added the word "locked" in red as a prefix to the label), but they do become enabled. I feel it's OK: the label tells them which fields to be careful about editing. However they might initially be confused that enabling the override checkbox didn't remove the labels.

I found a complication with disabling fields instead of removing them. It would mean that we override the #access = FALSE and replace it with a client-side restriction that can be circumvented with web developer tools. True, we would hope we can trust staff, however I suppose that the Drupal.org security team might class this as a security vulnerability. How do you feel about leaving that part out for now?

I suggested that we could set a flag on the declaration revision to indicate that it potentially alters claims that have already been submitted to HMRC. Are you interested in that??

For retroactive cancellation, I feel that the date should be specifically the exact start date of the declaration. It's not really a wildcard "any date you like". Earlier could be wrong: maybe there was a rock solid declaration for the previous 10 years that ended with a standard forward cancelled and we mustn't disturb it. And later could be wrong as it could fail to cover the declaration and lead to an incorrect claim. So I feel instead we could have a checkbox for retroactive cancel that hides the date input. This is handy, because it gives me something that I can disable by default, then enable with the override.

• The "retroactive" checkbox seems to work well.
• I have found a way to remove the "locked" labels when overriding.
• The "locked" label CSS is based on the CSS classes on the form, which could be theme dependent.

We have similar ideas, and the latest commit covers some of what you suggest already. You are right, there are really two different processes here:

1. A cancellation brings all current declarations to an end; normally the start_date would be the same as received_date (earlier is not allowed), and the end_date empty. It is represented by a Cancellation entity.
2. A denial (better names welcome) is a negative response to a confirmation; it makes a single declaration invalid as if it had never been made. There is no opportunity to specify any dates (HMRC says declarations are invariant); it's only allowed within 30 days of a confirmation. We don't need a cancellation entity, we can just change the validity to a new state: denied.

Create a separate [denial] form and route, with a route parameter that takes a declaration id.

That's a reasonable idea. As an alternative we could try and simplify. We could stay like the current code having one route/form with two options.

1. My past gift aid is fine, but please don't claim in the future
2. The past gift aid declaration(s) that you asked me about are wrong, please ignore

I tend to feel your suggestion is better, if we don't mind a little extra development time/complexity.

the donor could technically choose to allow gift aid in the early years but not later years

I can see two options:

• Due to a staff error, the declaration was entered with missing/wrong end date. An admin could override the lock and edit the information. Or maybe better if the problem is spotted years after is to deny the first one and create a new one that is correct (which doesn't require admin).
• The original declaration was correct, however their circumstances changed, and they forgot to tell us. That sounds like a retraction, i.e. a backdated cancellation. Again it requires an admin to override the lock.

Either way I feel that a denial contains no dates - it simply changes the state of a declaration.

I created #3572561: Invalidation as a separate process from Cancellation.

The intended scope of this issue is admin overrides, i.e. breaking the restrictions of what is allowed in either of the scenarios. I have mostly completed that now. Are you OK for me to commit once I fixed the tests, then we continue in the other issue?

Thanks. I agree this already contains some efforts towards #3572561: Invalidation as a separate process from Cancellation that might not be correct. I propose to commit it anyway and then correct it on the other issue. (Otherwise I feel I will take time making more changes here likely without really saving any time on the other issue.)

Let's discuss it in #3572561: Invalidation as a separate process from Cancellation as you suggest.