🇬🇧United Kingdom @jonathanshaw

I suspect this is a duplicate of 🐛 \Drupal\plugin\PluginType\PluginTypeManager::getPluginTypes() does not use file cache correctly Needs review

jonathanshaw → created an issue.

See https://git.drupalcode.org/project/entity/-/tree/8.x-1.x/src/BundlePlugin

And

https://git.drupalcode.org/project/entity/-/tree/8.x-1.x/tests/modules/e...

Commerce_payment uses this for bundles of the payment gateway or payment method entity type I think.

But also when I said about putting logic in postSave() on the bundle, I was mixing it up with the bundle classes of https://www.drupal.org/node/3191609 → .

Unfortunately a class can't be both a bundle plugin and a bundle class as they need a different parent class.

But we could have an onCancel() method on a GiftAidRecordBundlePluginInterface and call that from the GiftAidRecord::postSave() entity method.

I'm thinking MCP is the way to go here. A ddev extension could provide a set of MCP tools that help working with Drupal codebase but which would work in any AI dev tool, Claude code, roo code, cline, cursor, etc. They all support MCP.

I guess that stripe might not know that the card is disposable

They must know, or otherwise why do they prevent it being attached to a customer?

(1) If you were able to provide a complete [redacted] payment intent object JSON here that might be useful.

(2) Also, it's be good to know what the exception was that was thrown, what it's message was, and what stripes cloud logs show.

(3) Stripe's test docs say that pm_card_visa_chargeDeclined can't be attached to a customer. So you could try creating a test customer and attaching that and seeing what error you get.

💬 Anonymous Checkout Denied When Order is Programmatic Fixed is an old discussion but possibly relevant.

jonathanshaw → created an issue.

I think this is ready now, but I've not tested it properly and I'm reluctant to put more work into it without an thumbs-up in principle from a maintainer.

What you say makes sense to me.

But there might be a more precise solution.

The root problem is likely:

       $is_reusable = $this->isReusable() && $payment_method_type->isReusable();
        ...
        if ($is_reusable) {
          // We cannot attach if the payment is not reusable.
          $stripe_payment_method->attach(['customer' => $customer_id]);
        }

There is one additional source of information present on the payment intent at this point that we might be able to use:
payment_method_options.card.setup_future_usage

Use none if you do not intend to reuse this payment method and want to override the top-level setup_future_usage value for this payment method.

Maybe we can check this property of the payment method options and add it to the logic deciding whether or not the payment method is reusable. If we do this, I think we need to add an ::isResuablePaymentMethod(\Stripe\PaymentMethod $method) method to the gateway plugin, I'm seeing other places where this logic needs to be refined.

Could you check what the value of this property is on one of your Revolut card payment intents that fail?

jonathanshaw → created an issue.

if they return without completing the payment and modify their order, we need to update the PaymentIntent amount accordingly.

@gaydabura thanks for the reply, much appreciated.

(1) I agree that the scenario you mention is valid, and we need to be sure it addressed.

(2) However, I'm suspect that the fix you suggest is not right. If you look at the examples screenshots in payment intent docs at https://docs.stripe.com/payments/paymentintents/lifecycle it illustrates that at the 'require_action' stage the customer is no longer concerned with verifying the amount and may not even be shown the amount. The change you made in OrderIntentSubscriber might conceivably allow other processes (like staff or price changes) to update the payment intent and change the amount charged but without the customer being aware of the final amount they were paying.

(3) Is there are any reason why this issue of customers returning without completing the payment and modifying their order is a bigger concern with paypal than with card payments?

(4) Given that this concern seems to be equally valid for all payment methods, I believe we should open a new issue for it and address it there for all payment methods. The general experience in the Drupal community seems to be that the more problems we try to solve in a single issue, the less likely we are to solve any of them. Smaller scoped issues make for faster progress.

(5) I wonder if the right solution to the problem you give is in the StripeReview checkout pane. Perhaps if it finds an existing payment intent, it should check the amount of intent still matches the order price.

@gaydabura why did you make the change you made to src/EventSubscriber/OrderPaymentIntentSubscriber.php?

That change may have sigificant consequences on payment types other than paypal, so it is unlikely this can be committed with it.

ApplePay is already supported in the current version using the Stripe Payment Element.

There is no Apple Pay payment method type plugin provided by commerce_stripe yet, so that is not exactly true as I understand it. Isn't there still work to do?

I've also found out that because onReturn throws a redirect exception directly, instead of use CheckoutFlowInterface::redirectToStep() it bypasses CheckoutFlowBase::onStepChange() and any logic there.

Again, this non-standard approach has all kinds of side-effects. At the very least it would be good to document why it's done the way it is, so that anyone tinkering with it understands the consequences of changing it.

jonathanshaw → created an issue.

This non-standard approach also has unexpected side effects like 🐛 Order balance is not updated when order placed Active .

jonathanshaw → created an issue.

I think this may be an achievable and desirable feature. The multiple customer ids are a consequence of commerce's default assumption of one remote id per gateway, but there are valid circumstances for having differntly configured gateways on the same provider that share the same remote customer id.

PaymentGatewayBase assumes that the remote id is gateway specific:
$provider = $this->parentEntity->id() . '|' . $this->getMode();

I think our payment gateway plugin could just override get/setRemoteCustomerId:
$provider = 'commerce_stripe_shared|' . $this->getMode();

Backwards compatability here is not trivial as there may be custom code making expectations about the provider id. Unfortunately there is no getProvider() method on the PaymentGatewayInterface to help with this.

We could adopt a permissive approach where if getRemoteCustomerId finds no customer id, it identifies all of the other stripe payment gateway plugins and gets the remote customer id from any of them that have one.

The only one of these edge cases that I actually care about in my project is the anonymous donor whose order can later be identified by having the order email match one of the users emails.

Given that I could workround this by updating the order entity retrospectively to add the customer, I'm sympathetic if you want to say "this contexts business is a nightmare, how about if I create the module without it and then we see if you'v got any budget left to worry about it?"

I'm struggling to understand the idea of "related contexts". I can see that we need to be flexible with context. Staff might detect duplicate donors and combine, or the website might even do it automatically (name/address similar). We might also need to split a donor apart if two individuals got accidentally merged. Or correct an error when a declaration was assigned to the wrong donor in case of two that are similar. However I see all of these as direct edits of the entities, rather than storing a separate entity to record that two contexts are related. I would like to have a goal of 1-to-1 correspondence Donor <=> individual. Could we then remove the idea of related contexts? It would mean that the list of declarations could be found by direct SQL query and we can use standard mechanisms like EVA to show the declarations for a Donor.

Imagine an organisation that has
1. CRM contact entities who may have offline ddeclararions recorded for them
2. Users able to access the website and make or cancel declarations for themselves
3. A commerce checkout where donors can mae a declaration at checkout

This creates a family of nasty edge cases:
A. Not all CRM contacts will have corresponding user entities.
B. A CRM contact may begin without a user but with an offline declaration, but later become linked up with a user and get a declaration or cancellation from that's users actions
C. A donation may be made with a declararion at checkout without being logged in. The user account could be created at the end of the checkout, or it could even never be set as the order customer but still identifiable as the same donor on the basis of email address.

The idea of 'contexts' was an atempt to handle these edge cases. The declaration stores the original context, there's no attempt to dynamically update what's stored on the declaration when further user/contact entitie are added/merged becaus that gets too complex and fragile. But nonetheless the system is able to pull these declarations together for a single donor when needed.

The only one of these edge cases that I actually care about in my project is the anonymous donor whose order can later be identified by having the order email match one of the users emails.

Store the address on the Declaration, correct at time of declaration. This is important if we need to move declarations between donors, or to unpick any errors in combining donors. The Donor address is used whenever we want the most recent known address. HMRC explicitly state that we don't need to update declarations if the address changes.

I've no problem with that as an implementation. I don't have clarity whether it's needed, but I've no problem with it and it feels plausible to me that it's a good solution.

Create an Entity type for Records such as cancellation or change of address. The alternative strategy of using revisions on the Donor is inherently inflexible because they are then locked into the specific Donor. NB I feel the Record entity approach is likely quicker to develop: the extra entity type is balanced by avoiding the need for Donor revisions and we get forms for free from the entity rather than having to design custom forms. In particular Cancellation was a bit strange as a Donor revision because it didn't change any Donor fields; it carried different information which probably we could only store by converting to a string and putting it in the log message.

The idea of a records entity type doesn't worry me as long as we also have a declarations entity type.

I think you probably still want donor revisions anyway as they're the most natural way of handling donor change of address.

I agree cancellation is strange as a donor revision. But there would be an evidence entity that would need linking to the donor as part of the cancelling which might hold a lot of the information.

I agree that a records entity type could well end up a nice clean solution. If you're going this way, I'd suggest not having a bundle entity type for the records. Instead maybe use bundle plugins from the contrib entity module. This means no config needed, and you get a class for each bundle where you can put specific logic, like putting cancellation logic in a postSave() on the cancel bundle.

Sure

I would use the flag so the web types don't appear in the list for staff to create (they only work as evidence for a self-declaration; even if staff create a commerce order on behalf of a donor they would need to use one of the other evidence types). Also the flag could affect access/validity: staff wouldn't need edit-access for 30 days to correct a data-entry error, hence we could allow claims to HMRC without the delay.

FWIW neither of those arguments seem right to me.

You don't need the flag *on the declaration entity* to remove web types in the list for staff. You need it on the evidence bundle. The staff form can show all the bundle types of evidence that have the bundle flag. The self-declare form doesn't overtly mention evidence bundles at all.

The validity argument is worth considering, but that seems like what we have the claimable_date field onthe declaration for. The self-declare form can just set the claimable date to today, the staff declaration recording form sets it to 30 days from today.

For example it seems to imply that Commerce-based declarations need to put the user as context rather than the order (else they will each have a separate donor referencing their separate order). If there is no user, perhaps we put the profile??

I think we better open an issue to explore this. It indicates that we have to be careful to make sure that we don't lose somehting of what the context system offered when we introduce donors.

#3 the evidence entity. I had been imagining a new specific new entity type rather than a dynamic reference to any entity. We can make bundles for each evidence type

Yes.

Probably next we need entity #4 the record that isn't a declaration - at least we need somewhere to store the key data e.g. for a cancellation: date of cancellation, and what does it cancel - a specific declaration (maybe relevant later for non-date-based declarations?) or all?

I'm doubtful we need a record entity type in addition to the evidence entity type. I think revision of the donor and declararion entity types, combined with the addition of new referenced evidence entities, is likely sufficient. This will probably mean things like cancellation need custom forms with a tailor made UX that do bespoke entity manipulation, rather than a simple use of an entity form. But that's ok.

However web and written declarations are fundamentally different evidence types for our software. The first is entered by the donor; has automatically generated evidence that references the order, and the displayed UI text; has corresponding code so is locked in the UI or at least can't be deleted or created. The latter is entered by staff, needs evidence such as a PDF, photo or email, and we should prompt staff to verify its various parts.

I'm not particularly fussed about 2 methods vs 3. But FWIW I don't yet fully share your perspective. Your argument there for me mostly points to the need to have 2 seperate forms, a self one and a staff one. I'm less certain whether that will involve an explicit need to flag web vs written or whether it will be obvious from the fact that declarant = creator.

Perhaps you mean the declared date, which I agree is important and we do have a field for.

Thankyou, you're right.

Create a multi-state validity, merging the two fields, good idea. This becomes very close to the status field, so I'm attracted to drop status as a separate computed field

Agreed. I suspect we need to postpone status until later when we have more clarity over when we need it and what for.

I feel inclined to avoid making the effective date be part of the validity state as that would require a cron hook, batching, and possible error cases

Possibly we will have a computed and a stored validiity (obviously not called the same!).

I will try inline entity form for creating the evidence entity.

I have UX concerns there as per #12.2 above. So by all means investigate, but please don't sink too much time into it. I have found IEF to be one of the most fragile, buggiest, complex and poorly maintained parts of the Drupal ecosystem. So I'm nervous on making this module's complicance critially dependent on significant customisations of it.

More specifically, AccessResult implements RefinableCacheableDependencyInterface so where we handle the query execution we could collect the cacheable metadata from the access result.

Thankyou, now I understand. The proposed queryAccess() handler method allows both modifying the passed in query, and returning an access result with cacheability metadata. Clever, although not a pattern I remember seeing in core before.

In principle having this on the handler seems good. But I see it as a small addition to the current system, not a simplification. And something that could be a separate issue.

I'm interested in the cacheability question you raise, but I don't understand how your handler proposal helps it. Can you say more?

Sorry to be slow to reply.

Replace "written_record_required" with "method" field with 3 states: oral; writing; web. The corresponding state of "written_record_required" is yes; prompt (staff to validate the evidence and tick a box); no.

I feel unsure here. My thinking on this hasn't fully adapted from the old plan where there where different bundles and so staff would encounter a page where they first selected the bundle with an explanation of when to use which, to the new plan where there may not even be declaration bundles and it's the evidence field referencing evidence entities of different type that handles the different delaration methods.

A few thoughts I'm having:

(1) The guidelines distinguish between oral and everything else. And we could actually treat web as a species or written. So maybe the method could just be a boolean is_oral.

(2) How do we create a good UX for staff to select which kind of evidence entity to attach? IEF for example tends to offer just a dropdown with the bundle label, which is not a reliable UX for an important compliance-relevant decision like this. Two possibilities:
(A) use a modal for selecting the evidence bundle when adding evidence. Maybe there's an IEF variant for this.
(B) Reverse the entity creation flow. First the staff choose the evidence bundle from a standard bundle-based entity add page, then they go to a declaration form prepopulated with empty evidence of the chosen bundle.

(3) If we use option B, then we could programatically set the is_oral field depending on the evidence type chosen (if we had an is_oral field on the evidence type, specified by the sitebuilder).

I see that the word record is predominant in 3.10.1 so it's inconsistent! Also A few times it uses "written confirmation". The word "record" is least distinctive, used in many other ways a total of 101 times whereas statement is only used in this context.

"written statement" is used in 3.8 to describe a process for retrospectively validating invalid declarations. "written record" is used in 3.10.1 for what needs to be sent to oral declarants. The two are functionally similar and ideally our work on 3.10.1 should made it easy to support 3.8 in the future, but 3.8 is out of scope for now and a rarer use case. So that's an argument for "written record". "Written confirmation" could be argued to be good because it covers both situations without being overidentifed with either.

DONE Change field "invalid" to "valid". It seems more straightforward, and implies an active choice to demonstrate validity. Optional

Validity is an interesting one. There's 2 main flows in the guidance:
1. An oral declaration is not "effective" if there's no recording until a "written record" is sent. And it its "cancelled" within 30 days of the written record being sent then it is treated as if it's "retrospectively" cancelled and treated as if it were never effective.
2. An declaration with evidentiary or data problems can be deemed "invalid" retrospectively. A charity can "validate" it by sending a "written statement". If undelivered, then treat as "cancelled". If not "cancelled" within 30 days, then the declaration can be considered "valid".

Notably, it's not obviously impossible to send multiple written statements if something weent wrong with the first one, or multiple evidentiary problems were found successively.

So there's 3 states:
1. Invalid/ineffective before written statement sent
2. Pending, will be effective in 30 days
3. No problems, all good

It seems like we might need:
1. An effective_date field that indicates the date from which the declaration can be relied on.
2. A mechanism to set the effective_date to 30 days subsequently when a written record/statement is sent.
3. A way to mark a declaration as cancelled from the moment it was created, being never effective.

Not sure how best to handle all this.

Sure, merge it.

SmartDateDurationFormatter already passes the timestamps.

As I uderstand it: SmartDateDailyRangeFormatter, SmartDateRecurrenceFormatter and SmartDateRecurTrait all seem to built of top of the smartdate field type, which stores timestamps not iso_8601 strings and so already passes timestamps to the augmenter. And as the files aren't opted in to strict types, we don't need to explcitly cast to integer.

So I think we are ok?

The reason why this wasn't done previously is probably this code:

      // If only one of start and end are displayed, alter accordingly.
      if ($parts['start'] xor $parts['end']) {
        if (in_array('start', $parts)) {
          $end_ts = $start_ts;
        }
        else {
          $start_ts = $end_ts;
        }
      }

The code modifies the timestamps permanently, effecting in all the subsequent contexts in which they are used.

Some of these contexts - like the augmenter - need the original stord timestamps, but some of them probably want these modified timestamps. In my fix here, I have passed the stored timestamps to the augmenter only. Figuring out which other contexts need the stored timestamps and which the modified timestamps is beyond me though, that needs deep understanding of this module.

Fortunately, we don't have to solve that here. What I've done in this MR is a step forward, even if there's more that could be done.

jonathanshaw → created an issue.

Does making it optional solve the security issue sufficiently for ✨ Allow users to login using either their username OR their e-mail address Active to advance? The IS needs to explain this aspect I think.

Move the charity and context fields into it (from declaration)

This is the bit I hesitate about.

I think perhaps it needs its own context field, but the declaration still needs a context field too. So the declaration finds its Donor indirectly via the context.

But then from a compliance point of view I guess your thinking its better to be more fixed about the donor's address "on" the declaration. That's a good argument.

But how do we ensure single donor reuse across multiple declarations? By finding any existing donor via context when creating the declaration. So context is still a crucial intermediary. Maybe this is OK.

The name can change and still be the same person.

This point makes me nervous, it would look really bad at audit to be offering a name and adress for a person who was not the one who made the declaration. I think maybe we need a checkbox or confirmation form where they can explicitly choose to apply the new default customer address to their gift aid record.

- Some need it conditionally, depending on 'Advised of liability' (and we can write clear advice to staff for filing out 'Advised of liability')

That's a good idea.

For my legacy paper forms scenario, I can create a bundle specific to that scenario where confirmations are always sent, not optional for staff to decide. It means I need a seperate bundle for new (non-legacy) paper forms, but that's acceptable solution for an edge case like this.

Actually staff shouldn't be creating declarations for the bundles that don't need validation (such as web self-declaration), and we could enforce that (with admin override).

Agreed. We could have permission per bundle, but actually I think it's slightly confusing to use the normal Drupal idiom for something that has compliance implications. Better probably to have a bundle setting "Allow declarations of this type to be recorded by other users." because that way we can add more explanatory notes etc in the UI. The permissions UI is not a good place for stuff that has complicance implications.

My current preferred idea is a hybrid of the 2 solutions: a cancellation would automatically update all declarations to alter their validity. We could separate fields for the original end date (immutable, entered in the UI) and the current end of validity (calculated, read/only in the UI).

I like it. It's very robust and clear. I had hoped to not need it, but it's good and not so hard really.

Note that this "end of validity" is just a "shortcut" which could be recalculated at any stage (someone could write code to do this automatically). This is great for untangling data inaccuracies, such as the "complication" in the IS or if one person has ended up with 2 distinct contexts (database records) which need to be combined.

My concern there is distinguishing between originally specified end dates and later applied ones. So maybe 2 different fields?

In 🌱 Product requirements Active I found this interesting line in HMRC guidance:

3.9.3 If a donor wishes to alter the description of the gift or gifts shown on a declaration [i.e. start and end dates] , they must cancel the declaration and make a fresh one.

I feel we should leave the site owner free to choose how they use declaration types. E.g. they might instead want declaration types to correspond to donation types: one for commerce, one for direct debit, one for a sponsorship form etc. If we have a field for "method" the software can calculate when a written record is required automatically (it would also be true if we are missing some information).

Evidence is required by HMRC - if we have a base field we can enforce that. We could make evidence a reference to a media type, and add a field to say what it is evidence for. Then we could write some code that checked that the current changes that are being saved have matching evidence.

The debate is good, but I stand by my position.

The key point is that what amounts to evidence, and what is a good UI for entering that evidence, is not something we can know. Only the charity will know its existing systems for capturing and storing evidence & how to integrate with them, and what the predominant use cases are for data entry in its situation and how best to optimise UX for them.

We can't know that a media reference field is the best UX for any or most use cases. For example, when we create an email bundle I will suggest that we put fields "email address" and "email text received" on it as that's the best way to create a gift aid trail for an email.

The bundles should focus on optimal UX for different declaration scenarios.

I'm imagining we should provide configured (so optional and alterable) bundles:
- commerce (i.e. checkout)
- web (i.e. self declare by web UI)
- email (staff declare)
- phone (staff declare)
- face to face (staff declare)
- letter (staff declare)
- paper form (staff declare)

The question of when to send a written confirmation is rather complex. I do agree it needs careful discussion. See 🌱 Product requirements Active 3.2.
It looks like site builders should be able to require a written confirmation on a bundle, disallow a written confirmation on a bundle, or make it optional as staff specify.

'Advised of liability' is an interesting one. HMRC seem quite hot on it as part of an audit trail. I have some concern that the fact that e.g. a checkout is configured to show the liability message could provoke a query - can we prove that on such and such a date the user actually saw a liability message and what exact message did they see? It makes me wonder whether it's good even for commerce, web, paper form contexts to have a field that we can set to indicate that the liability advice was given at time of declaration. I'm wondering if even if we prefill it and don't allow staff to untick it on some bundles, it might still be useful at audit and 'look more convincing'.

I have created 🌱 Product requirements Active as a replacement for this issue.

jonathanshaw → created an issue.

The profile type automatically comes with lots of free stuff, including a user tab, however there's a risk it will end up being not quite the stuff that we want and difficult/impossible to alter.

Yes.

So the choice between "Donor entity" and "gift aid profile type" is likely just a matter of ease of implementation.

Yes.

I feel that if we use the existing commerce profile, then we don't actually solve the Problem/Motivation in the IS: profile might get deleted too soon; order might not have an address; revisions are off by default; staff not required to provide evidence; lack of clarity which profiles relate to gift aid.

Good considerations.

I'm definitely open to the idea that it'd be no harder, and likely cleaner or more robust, to have a donor type.

One aspect of handling the commerce customer profile integration could be that if a user edits their default customer profile, then we add a checkbox saying "I have changed my name & tax address for UK Gift Aid too." and update the matching donor correspondingly. Or possibly, we should just assume this is what happens as long as the name is unchanged - HMRC would probably prefer our best guess as to the up-to-date contactable customer address, rather than what we can rigorously prove.

1 and 2 are what I was thinking, great.

I don't fully understand the first and second option in 3. I wonder about having a checkbox when staff create a new declaration with an end date "Enforce this end date on relevant previous declarations?"

we could make a Donor entity type

That's probably got advantages. My hesitation about using a solution other than profiles, is that if one is using commerce, then the customer naturally has a set of already provided addresses in the customer profiles, which it makes sense to allow as options.

My suspicion is that this will be hard to answer until we have a more precise articulation of the exact UI scenarios we need to cover.

Good spot.

Cache::invalidateTags([$old_host_entity->getRegistrationListCacheTag()]);

FWIW I would have done this in Registration::postSave().

jonathanshaw → created an issue.

In \Drupal\registration\RegistrationAccessControlHandler::checkAccess, there is no explicit check to grant access to users with the 'administer registration' permission.

No. But we do have:
$result = parent::checkAccess($entity, $operation, $account);

And EntityAccessControlHandler checks this permission:

protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
...
    if ($admin_permission = $this->entityType
        ->getAdminPermission()) {
        return AccessResult::allowedIfHasPermission($account, $admin_permission);
    }
...
}

But we call that parent check AFTER we check for host/configured.

Yes, Queue::prepareJob() should be able to return NULL, meaning 'discard this job'. This makes sense because prepaeJob() relies on job type plugin's handleDuplicateJobs() method, which has the design feature that it can return nulls, meaning "discard the current because it's the duplicate of an already queued one".

Queue:

  protected function prepareJob(Job $job): ?Job {
    ...
      if ($duplicates = $this->getBackend()->getDuplicateJobs($job)) {
        $job = $job_type_plugin->handleDuplicateJobs($job, $duplicates, $this->getBackend());
      }
    }
    return $job;
  }

JobTypeInterface:

  /**
   * Handles existing jobs detected as duplicates when enqueing a new job.
   *
   * A function can be used to execute a range of different strategies with
   * regard to duplicate jobs:
...
   * - to discard the new job and leave the duplicate intact, return NULL.
...
   * @return \Drupal\advancedqueue\JobResult|null
   *   A new job to enqueue on the backend, or null if no new job should
   *   be enqueued.
   */
  public function handleDuplicateJobs(Job $job, array $duplicates, BackendInterface $backend): ?Job;

I think your concerns about audit are worth taking seriously for sure. Maybe revisions and Entity Reference Revisions for the profile reference field would address it. Or just store the address *at the time of declaration* on the declaration in an address field that doesn't get updated.

When you make a Gift Aid claim you don't show the declarations to the Inland Revenue or break it down by declaration. Instead the claim is specific to a financial year and shows the donors name, address and the total amount of donations in the year for that donor that you're claiming Gift Aid on. The address used should be the best address for the donor that you have, even if you're making a claim for 3 years ago when they gave you an older address.

So the concept of address is really more at the level of the donor rather than the declaration, even if it needs to be present on the declaration.

By cancelled, I mean to set the end date to now. I believe this solves the problem you refer to.

That sounds like my second solution from the IS?

I don't feel intuitively connected to your idea that there is a special active declaration, ... It seems that at best it could only apply ... at a certain moment in time... Mostly we will want to know if at least one declaration exists, i.e. hasActiveByAnyContext(). We might also need to put a link to one of the declarations, but it could potentially be chosen arbitrarily??

Good. Our thinking aligns. Ironically we each thought the other was saying the opposite!

create a new declaration when rights are granted, i.e. the date is moved later, and bulk update when rights are removed.

Yes!

In terms of retraction, then I could see that as a special case to handle by a user with admin permissions.

A good enough solution for now to be sure.

In the case of declarations moving into a context (a online donor with existing old declarations gets associated with a CRM contact who has recently cancelled gift aid)

I wonder if this module needs to take responsibility for that case??

Agreed. I think it's good to have some awareness of context edge cases in case we can sometimes avoid them. But let's not create elaborate mechanisms to handle them. Nice if we can document them.

I think we have the theoretical clarity we need, we can move on to implement.

As I pointed out before, this function is currently not used, so I feel it's premature to keep spending time altering it.

Agreed. Can you leave a todo flagging the question and otherwise leave the code in whatever state you think best?

john.oltman → credited jonathanshaw → .

I don't think it's particularly important, but thought the idea might be worth sharing.

jonathanshaw → created an issue.

See also ✨ Record donor name & address Active

Nice!

2) Sure I can wait if you like. However in my view, the current code is unacceptable in terms of audit requirements as declarations change their end date in baffling ways without an audit trail. We could safely remove it even if we don't know what we'll use instead.

For right now, I don't mind which you do. But the MR is weird in that it reintroduces $later, but also removes $newer. That will definitely produe bad results I would think. Originally bother were present.

(3) getActiveByAnyContext

If 2 declarations had the same end date, we should prefer the newer of the 2.

(5) The dates in Declaration entity.

I'd have had 3 concerns with the MR:
- it used \Datetime to get the current time, not Drupal's time service. That's untestable and not best practice.
- it seemed to remove the edge case handling associated with this comment:
// The declaration ends at the end of the day, not beginning of the day.
- I thought that anytime we stored a date in Drupal we needed to update the timezone to the storage timezone before we format it using the storage format. Because when it gets loaded it will be assumed that the stored string is in the storage timezone. Seems dangerous to break that assumption for any custom code accessing these fields.

I like your idea that maybe assuming Europe/London is a good rule of thumb in this module. And your point that the tax year end is in BST might be important.

My current thought is that getStartDate() and getEndDate() should rebuild the timezone to Europe/London:
$formatted = $stored->format('Y-m-d H:i:s');
$new = new DateTime($formatted, new DateTimeZone('Europe/London'));

Additionally, getEndDate() should use "23:59:59" as the time.

The donor has changed their mind. We can then mark the old declaration as cancelled

mark the old entry outdated: this is an updated preference

I don't think we can do either of these things. The problem is that declarations will often be partially overlapping: the old declaration may have a start date earlier than the newer one, and in that earlier period it may be the necessary declaration for donations made in that period; it's only superseded in the later period.

The basic questions with cancellation is "where do we store the new end date"? On the declaration we currently think is the active one, or on all of the relevant stored declarations? I'm thinking it's safer to apply it to all of them, in case some shenanigans changes which one is active.

Changes of start date should be very rare, and are hard to reason about; I'm happy to leave that to admins to figure out manually editing all the relevant declarations.

I feel it will help us to phrase things in terms of the standard Drupal terminology of CRUD.

I've actually been skittish about this. As you say, donors shouldn't be exposed to the complicated concept of declaration instances. But for things like cancellation, I'm not sure that non-admin staff should be either. We might need some simple forms that take the necessary entity updates, but which ones are conventional entity edit forms is not yet clear to me.

Creating a duplicate fails

I don't think so. It's a new source of legitimacy to a Gift Aid claim, even if Inland Revenue decided at audit that your previous declaration was invalid for some reason, you could still be covered if this new supposed duplicate with a different evidence trail was valid.

Update is allowed within a short time period to correct a clerical error and after that, there are access restrictions.

Yes, I've wondered about whether we would end up with a mechanism like that. Not yet fully clear to me. I guess there's always scope to upload additional evidence, so maybe the access control is at the field level.

Can cancel (set end date to 'now') but cannot set end date in the past. Maybe can also shorten or lengthen end date to a new future date

I'm thinking that it may be better on GiftAidOverviewContextController to show the user's current Gift Aid declaration status, and then have buttons like "Add declaration" or "Cancel" that go to specific forms with a UI and staff guidelines appropriate for that.

Delete is not allowed except for admin

Except within short time period as you suggest above.

jonathanshaw → created an issue.

Yes, if we use this for the access it makes sense to have it.

Feel free to reopen if you think that's right, I agree we definitely should have an issue for adding UI for customers to add/manage payment methods and it would be great to see your code for this.

(1) You're right, the removal is good.

(2) This is related to 🌱 The problem of changes Active . I'm OK with you removing the code here if necessary to pass tests, or leave the todo to resolve in the context of that issue.

(3) Consider this scenario. A user makes a declaration saying we can claim gift aid indefinitely. Later a user makes a declaration seperately saying we can claim gift aid this year. Does the later more limited one win out over the earlier one here? I was thinking it shouldn't because it might be an artifact of a more narrowly focused form or something, but now I doubt that and think maybe the newer one should be taken as a partial cancellation per 🌱 The problem of changes Active . So I suggest we need to revisit this question in that context, but the removal may well be OK. However, even if the removal is technically OK because the circumstances should not arise, I'm not sure whether or not it's good to use this point-of-loading to enforce the cancellation.

(4) The only thing that's making me hesitate about the fields is the commerce store, which has some of the same information. Probably that's just harmless duplication and we should put the fields in. Yes, fine to use address module.

5)

I fixed the start/end dates on Declaration, however on reflection I believe it's still not right.

I'm not sure what you mean. Maybe what you did in
https://git.drupalcode.org/project/gift_aid/-/merge_requests/3/diffs
I overlooked that earlier, there's various things about it that don't seem right to me at first glance.

I created ✨ Record donor name & address Active for address handling and added it to 🌱 [META] Roadmap Active .

I like the discussion on "advised of liability", oral and bundles a lot. I'm still not sure what's right, but I think it's important and sensible discussion. Worth its own issue.

jonathanshaw → created an issue.

Yeah, I figured this was a notepad of sorts. See 🌱 [META] Roadmap Active for priorities.

Add name and address required fields to the Declaration entity.

My thinking was that these belonged with the context, not per declaration. For example, on an order, the order's billing profile. Also, if a user changed their address, then that shouldn't have to be done on each declaration of that user. And if a user changed their address in the system for reasons unrelated to Gift Aid, then that should probably also be their new address for Gift Aid.

More generally though, we need to think about address complicance carefully:

If the charity is notified of a change in the donor’s name or address, it must keep a record of the updated information — this can be kept as an electronic copy such as a scanned document. The declaration itself does not need to be amended, but a record of the change should be kept on the charity’s database.

Add method (oral/written/self etc) and evidence (file attachments) fields to the Declaration entity

I was thinking to use bundles here, with different configured fields in different bundles. We should provide config for the common circumstances, but allow sites to modify further for their use case. As the fields aren't used programatically, it seemed better to allow configuration to do the work here?

and remove written_record_required from DeclarationType.
Add advised_of_liability field to the Declaration entity. If not advised, then written record is required.

I'm not sure. Oral seems to require a written record unless you have an audio recording of it? That's why I had this on the DeclarationType.

And AFAICS "advised of liability" is a requirement of any valid declaration at the point of declaration.

Add a page for a user to view/edit/create their own declarations and donations.

Yes, that's ✨ Fix GiftAidOverviewController Active although see 🌱 The problem of changes Active .

Enhance the declarations listings to cover everything needed for audit. Add some filters.

Nice.

Turn on revisions, perhaps require a revision log or generate automatically, write submission guidelines.

Yes, this is a very important area, linked to 🌱 The problem of changes Active .

Add a link from the order/donation to the declaration.

Yes. Or at least a Yes/no.

If possible add a link from the declaration to all the orders/donations. (3.28).

Out of scope for now.

Remove the restriction that declaration end date cannot be in the past (3.9.3).

This can be a form of cancellation with an existing declaration. See 🌱 The problem of changes Active .

Move the declaration form settings/building from GiftAidDeclaration to Charity to allow using the form elsewhere (e.g. the self-declaration page).

Code-reuse is good, but why Charity rather than a parent class I'm not sure.

Add support for tokens that refer to charity fields. Ensure that the charity name is included (3.9.2)

Twig us fields for free and is in many ways superior, so I'm happy to only support that.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

I just noticed this charmer:

If a donor who’s made an oral declaration and been sent a written statement by the charity cancels their declaration within 30 days of being sent the written statement, the cancellation will have retrospective effect, so that it’ll be as if the declaration had never been made.

That retrospective effect needs careful consideration, I think it might be why I wanted written_record_sent_date despite it being technically redundant as you said. Because it would make it much easier to compute/display whether it was "safe" to claim a donation under a declaration.

Choose charity with drop-down rather than auto-complete
Shorten some very long field labels, instead putting information into the description (E.g. "Is this an enduring declaration that covers all donations within a period of time?")
Avoid using technical jargon such as entity in UI menu/ page title, e.g. "Declaration type entities" => "Declaration types"

All sound good, maybe a UI cleanups issue.

Put entity links on declaration listing

Very likely makes sense. Maybe we should have a listing issue, as you point out it should have more work to enhance auditability, but this is not the thing to tackle first.

Commerce pane improvements. Give information. "Thank you already covered". "Sorry not eligible". "£XX.XX of your order is eligible". Maybe these supersede the when options? Allow to configure the data parameters of the declaration?

Interesting. Would need more discussion in its own issue. I wonder if its necessary in the 95% use case.

Fix deprecation notices in test results

Great

In GiftAidDeclaration::submitPaneForm(), set the Declaration date to match order date

Maybe. I'm not sure quite when/if it matters. But it makes me think we should have an issue "Ensure compliance if an admin checkouts an order for a donor" as that scenario raises all kinds of hairy compliance questions.

Associate declaration with User rather than Order if available, then can drop second half of code in GiftAidRelatedContextsSubscriber. This should be much better performance and more intuitive.

We lose quite a lot of audit information about the context of the declaration. Maybe that doesn't matter. I have an attachment to the way we currently do it, but I think you are probably right.

Change some cases that are currently invalid declaration, into required fields?

Worth considering.

Possibly you want https://www.drupal.org/project/inline_entity_form_preview →

Nice, thanks Adam.

Shouldn't src/RegistrationValidationResult.php be implementing RefineableCacheableDependencyInterface not CacheableDependencyInterface?

Your change to the behavior of RegistrationValidationEventSubscriber::getCacheableMetadata looks like it break the behavior of registration_admin_overrides\EventSubscriber\RegistrationValidationEventSubscriber

I'm also not sure what that change does to the way the validator and constraint validators handle cacheability, but I imagine we have test coverage there.

Sorry to be slow in reviewing this. A few points:

1. In RegistrationType, shouldn't the workflow be able to affect cache context and expiry as well as tags? I'd be tempted to add the workflow as a cacheable dependency in the constructor.

2. In modules/registration_inline_entity_form/registration_inline_entity_form.module you didn't remove a now redundant addition of the entity as a CacheableDependency.

3. In HostEntity you've overridden getCacheMaxAge() in a way that stops us inheriting any lesser max age from a cacheable dependency.

4. The just-in-time updating we're doing in getCacheTags() feels odd. We have to repeat it across getCacheTags, getCacheContexts, and getCacheMaxAge. It's vulnerable to bugs like 2 above. We have to maintain logic that core will take care of for us. But I get your concern about settings not existing yet - I like what you're doing with registration_settings_list. How about if we add all the field, registration type and settings as cacheable dependencies in the HostEntity constructor, but then additionally add the registration_settings_list cache tag if the settings are unsaved? Should work and is neater.

5. addCacheableDependencies() feels like e legacy method that should maybe be deprecated. At the least, it's little confusingly named. But it does contain some interesting ideas ...

6. addCacheableDependencies() adds a registration_list cache tag. Which reminds me that isUserRegistrant(), and all the capacity methods, likely end up depending on registrations. Probably we should add that tag to the host_entity. If we don't, we need to add it to more validators. At the moment only HostHasRoomConstraintValidator has it.

7. addCacheableDependencies() adds session or user.permissions cache contexts. I've no idea why, but if they belong there then probably they belong on the host entity or some of the validators.

john.oltman → credited jonathanshaw → .

jonathanshaw → created an issue.

jonathanshaw → created an issue.

Charity has accessor methods for some fields that no longer exist so I removed them. The alternative of course would be to put the fields back.

New issue please. I'm not sure what's right here, that might get clearer. I suspect we need to display the charity number and address to users at some point.

Nice work! And challenging :)

I'm fine to leave commited the changes you've made, but some need further discussion in new more specific issues, ideally with the removed code in the IS for consideration, or a link to the commit in the MR here.

In GiftAidRelatedContextsSubscriber I removed the test against a draft order because for sure the order is draft during the checkout process when looking for an existing declaration.

New issue. Your point is valid, but I'm not sure a past declaration on a draft order should be considered to have been truly declared.

disabled the code in Declaration::postSave() for "If this declaration has had its end date edited, force the same end date on other ongoing declarations for the same context."

New issue please. Your points are good, but I had a good reason we should consider how to address.

I simplified the code in DeclarationStorage::getActiveByAnyContext() so it just returns the most recently declared, and removed the corresponding tests. It seems that it shouldn't really matter which one we return if multiple are relevant, and I couldn't conceive of any rule relating to start/end dates that actually works reliably.

New issue. That code looks ok to me.

The routes gift_aid.declaration_make and gift_aid.declaration_extend are apparently missing the corresponding form. I'm not sure if they are still needed - potentially we could reuse the add and edit forms??

Let's remove extend, I can't see an important case for extending a declaration rather than just making a new one.

For make, please open a new issue.

DeclarationFormTest has code that expects special button text changes and thank-you messages, however I can't see corresponding code to generate them. Either I can change the tests or add code to match.

I think this is what I was working on when I stalled 2 years ago. I suggest opening a new issue, and give some consideration to what the form should be like. Hopefully it's just changing it to match the tests, but might be more complex. Happy for you to also do simple changes here to get the tests passing if you prefer.

I'm not sure that the calculated field Declaration:status is going to work. The value depends on the date, so it would break any caching. Also I'm not sure we need it: the function getStatus() is fine for most purposes.

New issue. We could appropriate cache expiry to the entity. I guess the purpose of it was for display (a computed field being more elegant than hook_entity_extra_field), but I'm open to other approaches to displaying status.

I believe we can rewrite the various functions in DeclarationStorage to work without it - and at the same time be faster, and take an extra parameter of a timestamp. This seems important as we can't always assume the current date is the correct perspective: for example suppose someone adds an order in the past or edits the date or an order. (This one isn't breaking any tests - it's just something I spotted.)

New issue. Sounds like a good idea.

Undefined array key "wrapper_element"

That's an annotation on a commerce checkout pane plugin.

I don't see any sub-module.

It's in the latest version in the gift-aid branch, per my comment on 📌 Migrate basic code Active .

Ah, I caught you out here: the latest version of the code isn't that on the master branch at https://gitlab.com/awakened-heart-sangha/main-site, it's that in the gift-aid branch I gave in the IS: https://gitlab.com/awakened-heart-sangha/main-site/-/tree/gift-aid

Confusing!

#2 done

Sorry, my bad. This actually is fixed in the latest version of the module where there parameters are upcast if not timestamps.

In my use case, I have legacy values in old events where only the start time is known, not the end time. I use the core widget for these events, but the smart date formatter.

jonathanshaw → created an issue.

having a helper method to handle that is very useful, since I need to invoke that logic from both RACH and the RegisterForm. I considered removing it from RegisterForm but that class can be called outside of a context controlled by RACH, for example somebody editing a cart with registrations in it.

Makes sense.

$this->context->getCacheableMetadata()->addCacheableDependency($host_entity);

instead of having to add individual dependencies on the host entity's real entity, and the settings. And extenders of HostEntity can easily add more to it, and all the existing code would pick those additional dependencies up automatically.

Yes, exactly.

I don't think it helps get rid of the constraint dependency mechanism,

I'm not sure why we need the constraint dependency mechanism if we have this. The validator knows what it uses, and adds the appropriate cacheability. Maybe someone has a host entity class that has a custom implementation of hasRoom() that doesn't need the settings entity at all, so hardcoding the dependency into the constraint is actually unhelpful. And having every validator have to add the host entity as a cacheable dependency if the validator calls a host entity method, is repetitive but it's only one line and a nicely explicit. Maybe I'm missing something bigger.