🇬🇧United Kingdom @adamps

For the cancellation, I would expect the following fields to meet the same standard of flexibility as the Declaration (i.e. some are for future-proofing and we don't create a UI now)

1. donor
2. charity
3. evidence
4. declared_date
5. date_based (later on we could cancel based on something other than dates)
6. start_date
7. end_date (someone could ask not to claim Gift Aid on their £25000 donation from savings as they didn't pay enough tax, but keep claiming on the £50 direct debit; one way we could handle this is a cancellation to exclude a date range)
8. reference to declaration (cancel that one specifically)

I don't see that it can really be a revision. It shares about 70% of the fields of a declaration, so we can make a new entity type, and give them a common base class.

On the other hand, change of address, and misc correspondence are simple things: they carry evidence and perhaps a date. We could fairly easily use a Donor revision: we'd need to enable revisions and add the evidence field. In return we would save a little compared with my previous idea because instead of the record entity with multiple bundles we can have a non-bundled cancellation entity.

In this way we avoid doing either of the strange things (one of which we have each been uncomfortable about for a valid reason). It adds a little work developing entities, but likely saves as much by avoiding forcing strangeness such as services with API functions that create entities under the covers.

What do you think?

I have some doubts about usability. With inline-entity-form it could be good - staff ideally won't even notice there are 2 entities.

Without that it seems pretty bad. The evidence entity it hard/impossible to refer to or select. My current implementation has no fields for donor, no declared date, no "record type" because I realise that they would all be duplication of the declaration. Nor does it have a label, and I don't see any sensible way to create one.

Even with inline-entity-form, we are adding complexity. It could be quite a lot simpler if we just let staff upload any file that they like (in other words use a file field). I agree it's a bit of a hack. And it doesn't work so well with the self-declarations, but I guess we could add a different field for those (which doesn't need a form UI because they are generated in code).

So I don't have a clear suggestion, I'm just putting it out for discussion if you feel. I'll have a quick try with IEF.

Don't worry - it was fixed by a webserver restart.

I'm seeing some very strange errors

For example admin/gift-aid/evidence-type/add gives

The "default" form handler of the "gift_aid_evidence_type" entity type specifies a non-existent class "Drupal\gift_aid\Form\Evidence\EvidenceTypeForm"

If I call class_exists from drush eval it works. If I call it from a web-page it fails. If I randomly move the file to a different directory then it works.

Similar for admin/gift-aid/evidence/add/

@jonathanshaw Any chance you could try it on your own server when you get a chance please?

Great thanks, the code looks good.

It now needs a few simple test fixes for the changed wording. Plus it would be great to have one new test to check the new message appears. There should likely be an existing test where we could just add a single line.

because they need to consider audit and provide an explanation of why they made the change they made

Yes I agree - whether it's a revision or a record, then custom code needs to be audit-compliant and provide an explanation and evidence.

I agree cancellation is strange as a donor revision.

And I agree that change change of address is strange not as a revision.

But I'm still left wondering: isn't revision log the API Drupal already gives us for this ...?

Yes you could be right, I will consider it again.

We have the change of address, cancellation and misc record all sharing various concepts including storage of evidence, access checking, various routes/forms etc. So we'd like to share code and UI. Whether we choose revisions or records, either way somewhere there is a part that's a bit strange.

There is information to store for these changes, especially for the cancellation. If we have record entities, then we can use fields. With revisions it's less obvious. We could have fields on the evidence, only used for cancellation. Or we could make more fields on the donor that are like revision log - they get reset for each new revision. Anyway, the evidence is specific to the revision, so it could be clearer to treat it the same way - it makes the correlation of evidence to revision more obvious.

At the moment I'm still not really all that attracted to revisions as there seem to be more compromises. It's potentially stretching revision API beyond its intended scope. Declarations and the other records have similarities: both require evidence, dates, have similar access restrictions, etc. With records, the 2 entities can potentially have a common base type whereas with revisions, one is handled as an entity and the other as a revision.

Anyway when I reach the decision point I will do an evaluation.

@jonathanshaw

I updated the "Default bundles" list in the IS to add info about fields. Please can you review? At the moment there are two pairs of identical ones, so we could potentially combine them.

The idea of 'contexts' was an attempt 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 entities are added/merged because that gets too complex and fragile. But nonetheless the system is able to pull these declarations together for a single donor when needed.

Thanks for that. I have my doubts though, because it seems to bypass many mechanisms that we are putting (or could put) in this module to meet the HMRC guidelines:

1. Explicit record of all changes with evidence
2. Preserve a copy of the evidence within this module in case the original data gets deleted
3. Provide access control and warnings for changing data that may have already been used for a declaration
4. Maintain a list of any such changes to allow retroactively updating claims with HMRC.

With the dynamic contexts, I feel that the charity could face major problems during audit because the required evidence is no longer present. So actually we do have to do the dynamic updates, which is exactly the solution you reached anyway in #4. Perhaps it could be "complex and fragile", but I am not convinced that it is any more so than the related contexts option.

I think you probably still want donor revisions anyway as they're the most natural way of handling donor change of address. blockquote>Donor revisions would definitely work. I agree they are the most obvious solution and I would be happy to do that. However with the approach I have outlined, the address history is provided by the declaration and change of address entities - so the Donor revisions are unnecessary, which saves time and complexity. The historical addresses are accessible without having to dive into revisions, which could be an advantage. If the Donor made one Commerce order with address A then another with B, then we can't necessarily be sure which is the "best" address, so it could be useful to have both available. Anyway I can always add the revisions in later if we want them.

Thanks for explaining I agree that is useful for me to know. I had a look and I feel that the suggestions work better for smaller changes.

Good idea. I didn't see the suggest changes button, so I've just posted my code here. Please let me know what you think.

    // Show newsletter sending options.
    $form['send'] = [
      '#type' => 'details',
      '#open' => TRUE,
      '#title' => $this->t('Send'),
    ];

    // Add some text to describe the send situation.
    $form['send']['count'] = [
      '#type' => 'item',
      '#markup' => $summary['description'],
    ];

    if (($status == SIMPLENEWS_STATUS_SEND_NOT) || ($status == SIMPLENEWS_STATUS_SEND_READY)) {
      if ($status == SIMPLENEWS_STATUS_SEND_READY) {
        $send_text = $this->t('Newsletters can only be sent once.');
      }
      elseif (!$node->isPublished()) {
        $send_text = $this->t('Mails will be sent when the issue is published.');
        $button_text = $this->t('Send on publish');
      }
      elseif (!$config->get('mail.use_cron')) {
        $send_text = $this->t('Mails will be sent immediately.');
      }
      else {
        $send_text = $this->t('Mails will be sent when cron runs.');
      }

      $form['send']['method'] = [
        '#type' => 'item',
        '#markup' => $send_text,
      ];

      $form['send']['send'] = [
        '#type' => 'submit',
        '#button_type' => 'primary',
        '#disabled' => $status == SIMPLENEWS_STATUS_SEND_READY,
        '#value' => $button_text ?? $this->t('Send now'),
      ];
    }
    else {
      $form['send']['stop'] = [
        '#type' => 'submit',
        '#submit' => ['::submitStop'],
        '#value' => $this->t('Stop sending'),
      ];
    }

    return $form;

Thanks

Great I like it. Yes your initial MR had some good ideas.

Please can we take one more step to simplify the code (and it also solves a bug)? I made some detailed comments in the MR.

Sorry I realise anyway that the fix is incomplete, my mistake. We need to

1. Remove reference to __disable_customize__ in Mailer::doSend() and LegacyMailer::send()
2. Add a 3rd parameter to ImportHelper::parseAddress() which is ?AccountInterface $account = NULL and pass the value to new Address() in 2 places.
3. In LegacyMailer, after the code that we already added, pass a 3rd parameter which is the current account. Add an extra constructor parameter protected readonly AccountInterface $account to fetch this.

Also please make sure that the current account that you are logged in with has a different langcode from the one on the new user.

adamps → changed the visibility of the branch 3413081-set-langcode-with to hidden.

Actually I believe you are looking for this #1140606: Permissions Per-Newsletter →

Thanks

adamps → made their first commit to this issue’s fork.

I feel it is a specialised case, so it doesn't belong in this project. You are welcome to create a contrib module for it. It would be a moderately complex task.

I updated the title as it's different from #3266989 - the SubscriberHistory entity currently doesn't even have a uuid field.

I didn't put a UUID for the reason that the entity doesn't seem to identify anything concrete. Equally a revision doesn't have a UUID. I am open to changing it, but we need to make sure it doesn't have a cost for the majority of sites that don't need it.

You have 3 plugins classes with the same ID. Drupal will use one of them and ignore the others.

Sure we could do that. All patches go into 2.x now please.

Yes I agree. I have already fixed it in 2.x, see OverrideManager::createInstanceFromMessage() which never returns NULL.

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.

That sounds like a good idea I didn't even know it existed. Please can you explain more?

I found this https://www.drupal.org/node/3191609 → . Or perhaps a capability of https://www.drupal.org/project/entity → ?

Thanks. The MR still seems mixed up - please see #53. This MR should only contain the new adjuster, schema changes and tests.

New fixed need to go into 2.x please. We can backport to 1.x after.

Thanks @debrup

I have one idea - make sure that receiver langcode is not the default.

Can this be broken up into smaller chunks at all

This issue is a deprecation issue that depends on about 5 other open issues in ✨ [META] Expose Title and other base fields in Manage Display Active so the question is somewhat theoretical at the moment.

I guess the patch will be quite large, but basically just be a load of deletions of deprecated code. I see no reason why it couldn't be split into multiple issues if desired.

@divyansh.gupta thanks for trying to test. It seems almost random whether a site is affected so maybe we will never know the answer.

Thanks it looks good.

This needs someone to do a manual test. I added "steps to reproduce" in the issue summary. First run them without this patch and check you see the error. Then apply the patch and confirm the problem is fixed.

Thanks

I would prefer to get something a bit more complete then get you to review and test the whole module thoroughly (rather than you reviewing each issue one-by-one in a slightly arbitrary stopping point).

OK for me to commit and continue working please? Probably ✨ Create evidence entity Active next.

Thanks.

Good idea for the edit page. There is already code in simplenews_form_node_form_alter() for SIMPLENEWS_STATUS_SEND_PENDING. I suggest we expand that to include other status types.

Thanks for the screenshot. I would like to move "Newsletter issue sent to 1 subscribers, 0 errors." to be above the disabled "sent" button? This would match the message prior to sending "Send newsletter issue to 1 subscribers. " Currently I find it hard to notice at the bottom. We could use the same code $form['send']['count'] = , just the actual message differs based on the status. We could even put "Newsletters can only be sent once." into $form['send']['method']. I like the idea that the form becomes much more consistent across the different states.

However there is a question of back-compatibility in case anyone customised the form. Perhaps if we make a new minor release and document it?

All fixes now go in 2.x please. The file is now ContactMailer.php and the code to change is line 100

    if ($email->getTag(2) == 'mail') {

This is much harder than I initially realised, so thank you for pushing me to consider it more carefully. I have 3 new suggestions. Apologies this might seem like I'm banging on again with ideas you already indicated that you don't favour, however I still see very strong reasons for them so I feel we need to revisit please. Maybe I've missed some important points, or maybe the new evidence I present will change the balance for you too.

1. 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.
2. 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.
3. 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 this 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.

Yes, yes, yes and yes😃.

Absolutely we are talking about what was originally declaration type, and will become evidence type in ✨ Create evidence entity Active (this issue is complex enough already so I just split that out). Sorry this issue has been a bit slow to adapt to the introduction of the evidence entity. I believe that every reference to declaration types will become evidence types, and probably we will no longer even have declaration types.

You are right, the differentiation mostly works better coming from the different behaviours of the staff vs donor forms. Probably the only time the code will explicitly check for "web" is to hide some bundles from staff. We can also usefully display the web aspect in the UI.

adamps → created an issue.

Thanks, good points.

I have raised 📌 Figure out contexts and donors Active .

I'm doubtful we need a record entity type in addition to the evidence entity type.

Yes, that would work. I put your comment on 🌱 The problem of changes Active .

I'm less certain whether that will involve an explicit need to flag web vs written

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. It won't significantly affect the development time whether 2 states or 3.

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

Yes that's a good way to describe it. The stored validity is part of this issue. I agree that the computed one can wait until we use it. Potentially it should output translated UI text. The current getStatus() function is a reasonable starting point.

I have found IEF to be one of the most fragile, buggiest, ...

Thanks for the warning!

From ✨ Enhancements to declaration entity Active

J: I'm doubtful we need a record entity type in addition to the evidence entity type. I think revision of the donor and declaration 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.

adamps → created an issue.

My proposals

1. I will try inline entity form for creating the evidence entity. At first glance, it might seem like more work as we increase the number of entities, however I feel it could actually go the other way: 2 simple entities with a specific purpose could be easier than one huge complex one.
2. Keep 3 states oral, written, web. I like that you try to simplify, but I don't see how to make it work without all 3.
3. Go with your suggestion of "written confirmation". I predict that the same code can be used for both cases, however one of them is out-of-scope anyway so it isn't a problem for now if that isn't 100% true.
4. Add your effective_date field, which I like.
5. 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 (solving the caching problem of 🐛 Declaration:status problem Active - the other option of adding custom caching could be expensive and difficult to debug).
6. 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. Instead we can cover this at the report stage (as per requirement 4.1), adding a check on effective date being in the past to the report view.

My thinking on this hasn't fully adapted from the old plan

Nor has mine😃. First there was just one declaration entity. Then we added #2, the donor entity, for good reason to share storage of the address, but I've not fully processed the consequences. 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?? Anyway I like that the donor entity makes things more concrete and clear - the ContextOverviewController now becomes simply the view page for a donor.

Our newest idea is to add #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. There's a good reason to separate the types of record (date-based declaration, cancellation, change of address etc.) from the types of evidence (phone, email, web, etc.). But it means another entity: more code, and more complex UI. Declarations would likely always need evidence. Do we force a 2-stage process to create one then reference it from the other? Or do we try and create both together e.g. with inline entity form?

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?

Or we could go back to combining the evidence into the declaration. It's simpler, with the downside of some duplication - we need to recreate the evidence types as bundles also for entity #4. However it's a less important case, and perhaps we don't need so many of them?

1)

The guidelines distinguish between oral and everything else.

In many places yes. 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.

This page specifies 3 types

You must keep records of all Gift Aid declarations, whether they are written, online or verbal.

"Written confirmation" could be argued to be good because it covers both situations without being over-identifed with either.

You make an interesting argument for their being 2 separate cases, but I'm still not convinced.

Firstly I feel that 3 the separate terms aren't as rigorously separated as you suggest, and in fact are potentially used interchangeably. 3.8.2 talks about a written statement for an oral declaration. This page says

You must send or give donors a written confirmation of a verbal declaration.

This implies irrespective of whether you have a recording (which was the assumption I made, but I agree 3.10.1 suggests only if lack of evidence). If oral needs a confirmation only if the evidence is missing, then it seems identical to written??

Secondly, I feel that the behaviour in the 2 cases (3.8.1 or 3.10.1) is effectively identical. The staff members evaluates if the declaration is inherently valid based on the evidence (the criteria might vary slightly for oral, but that's all). If not then it needs needs written confirmation. Any cancellation within 30 days of sending the confirmation is backdated. A valid declaration could later be reclassified e.g. based on an audit.

Validity is an interesting one.

I agree that "valid" and "written statement" could be combined into a single field. Possibly there's an extra state: fatally invalid, and we don't even intend to try and validate it with a written record (this state is currently possible with the 2 separate fields). Possibly we don't need the pending state, if we make the assumption (as per the requirements 3.3) that any declaration less than 30 days ago can be edited to correct a mistake => we wait until May 5th before putting together the annual gift aid claim.

I do wonder if the created date is so important for so many audit etc purposes that it would be useful to keep it as a field (so it s easily available in views, data exports, etc.).

Really? Perhaps you mean the declared date, which I agree is important and we do have a field for. Created date is just the moment that staff found time to process that pile of forms that came in last week. I propose to remove it because people will get it mixed up with declared date😃.

Thanks the code looks good. I'm confused about the test - it looks like there was a commit that added tests then they were later removed. Please can you check?

MailerHelper is now called ImportHelper in 2.x. The constructor needs only one argument.

It's a balance - we don't want to clutter the UI for experienced users with stuff they already know. I feel that help text should be separate from essential UI text.

Here's a suggestion. Instead of the changes in the current MR, we make just one change to NodeTabForm in the case that the newsletter has been sent (this is the point at which the "send now" button is 'missing').

There is already some indication of what's going on:

Newsletter issue sent to 4 subscribers, 0 errors.

We can make it more obvious. We could make the '#type' => 'details', wrapper be present always (move it outside the if test). Then in the case where there is no button, we can if you like make the button appear but disabled, and add a sentence "Newsletters can only be sent once.".

It seems to be fixed in 4.x. If you still see a problem on 4.x then please reopen the issue.

  /**
   * User ID used for account switcher.
   *
   * @var int
   */
  protected $uid;

Thanks

adamps → made their first commit to this issue’s fork.

Thanks @lus

Why did you change the category to "support request", are we doing something wrong?

There are no steps to reproduce, and this module is used by 20k users, so it's not likely it has such a critical bug😃.

You can do $email->getHeaders()->addHeader()

You are welcome to add it to the documentation. I'd rather not have it filling up the issue queue.

All fixes in 2.x please - that's why it's the default branch😃

Thanks for the information. I'm not sure there is anything we can do in this module - this seems more related to the upstream symfony mailer library.

Thanks - one comment please.

Thanks

adamps → made their first commit to this issue’s fork.

Thanks

Great thanks

adamps → made their first commit to this issue’s fork.

https://en.wikipedia.org/wiki/Open_mail_relay states

An open mail relay is a Simple Mail Transfer Protocol (SMTP) server configured in such a way that it allows anyone on the Internet to send e-mail through it, not just mail destined to or originating from known users.

This module doesn't have any mechanism to receive emails. I don't understand how it could possibly be an open relay. Please provide steps to reproduce.

The HTML setting applies only the newsletter emails, not to the subscription emails.

Fix is to use Symfony Mailer module, and enable override of simplenews emails. Then you can configure an HTML message.

https://www.drupal.org/docs/contributed-modules/drupal-symfony-mailer/fe... →

Great that the patch is working for some people. It's not something that I would commit. We don't want to add an extra phase that exposes the protected inner symfony email to everyone - it's protected for a reason😃.

See the link in the IS and perhaps also #8. We could provide an API function that allows registering of a signer function. That function would get called with the symfony email, however it's still protected for all other users.

It should be fixed in either 1.6 or 2.0. The MR here are not necessarily the full solution.

I am willing to accept this simple change without writing an automatic test script. We do at minimum need someone to run a manual test to check that it's working.

Thanks @catch. Let's hope that someone needs this issue enough to be willing to fund it.

The sequence of events as I understand it is described in ✨ [META] Expose Title and other base fields in Manage Display Active .

It proposes we next work on phase B stage 1 which is covered by issue ✨ Add formatters and other mechanisms as alternative to base fields directly in entity templates Active . That will involve ✨ Add a tag select list in string Formatter Needs review as the solution for 'title', and it seems quite close to completion. The equivalent for 'submitted' is more complex, however working code does exists in "Manage Display" contrib; it doesn't have its own Core issue yet as far as I know.

It came as a surprise to me that validation isn't run by default. My entity list builder crashes when a required field is missing.

For my custom entity type, I would like validation always to be required. I don't want the caller of the save() to have a choice. I would like to be able to rely on the fact that all entities in the database are valid.

We could have flag on the entity type to enable validation before saving - defaulting to false for now, but modules can start to opt in. Currently it is complex to enable the validation. Even the trait from #7 potentially doesn't work if the entity already has a preSave() (we would need to use it as a different name then add a function call).

Some are relevant to other issues I'm already working on or completed, and I've added the issue number to the start of the bullet.

I added some text in italics to highlight areas of flexibility or uncertainty.

Please can you review the IS?

Great thanks

I'd like to check this in now if possible. Donor entity is basically working on its own. It doesn't do much of any use without some other issues. The interesting decisions will come in the next stage, and we might have to adjust some details here at that point. It's hard to predict now, so I've settled for something simple that works.

I have tentatively removed the edit form for the Donor. This is based on the idea that everything for a donor will be done by adding new records/evidence - which can have knock-on effects in their postSave() hooks. These records then form the audit trail, and like a VCS nothing is ever lost. All the old addresses are present in the trail, so if you have paperwork based on an old address it's still easy to find the donor.

If you put the wrong address in at first, you can add a new record/evidence of type "data-entry error" and set the new value. Maybe I'm being a bit too strict, and we should allow an edit for this case (and perhaps for an admin to change the context??) - but on the other hand the strictness gives simplicity and clarity.

I feel inclined to commit this simplest case for now and extend later if needed (rather than spend more time now on something uncertain). Also probably the higher priority for your testing and reviewing is after the next step when there's something more interesting to look at.

Fixed in ✨ Record donor name & address Active

Great, now it needs someone to test. A manual test would be OK. Set up a multi-language site, configure a user with a non-default language, send an email with template text that depends on language. Test without the fix the language is wrong, with the fix it is right.

Thanks, it looks good to me.

We need someone to test that it works? Either run the code in a debugger, or add a debug print message, then check that the headers are as expected. If you can do that great, else we can wait for one of the other followers.

Thanks. The code in Drupal Core has recently changed to support PHP 8.4. Please can we do the same here?

$value = str_getcsv($value, escape: '\\');

All of this logic is in desperate need of updating but several of the issues around it are stalled.

Unfortunately yes😃. ✨ Add a tag select list in string Formatter Needs review is the likely next step, and it was moving well. But now the next step is a quite complex update hook.

@catch is there any chance of any funding being available? It probably wouldn't take a huge number of hours to make real progress on the whole META area. Unfortunately it's often too much to do for free.

Symfony mailer doesn't have any implementation Drupal\Core\Mail\MailInterface, and has no reason to create one. What would such an implementation even do? I feel this is a won't fix, or perhaps set to Webform.

Either you need to create system.mail configuration on your site, or persuade the Webform maintainers to change their module to work without this configuration. It's not really related to this module, which neither requires nor creates the legacy config.

Very useful, thank you. Potentially also out of scope:

• multiple charities
• declarations that aren't date based (I guess they would need a separate bundle with non-date fields to specify what donations are included)
• non-user donors (the design allows it, but we won't necessarily create the full UI)
• high-volume staff declarations (we currently assume that 95% are self-declarations and optimise for this case)

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

You make excellent points, I agree.

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.

Yes, I agree. I would phrase it like this:
- Some definitely need validation as per the regs (oral)
- Some definitely don't because we know that we showed the info (web self-declaration)
- Some need it conditionally, depending on 'Advised of liability' (and we can write clear advice to staff for filing out 'Advised of liability')

can we prove that on such and such a date the user actually saw a liability message and what exact message did they see

This might point back towards storing the message on the charity, and making that entity have revisions?

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

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

So maybe 2 different fields?

Yes I agree

they must cancel the declaration and make a fresh one.

Interesting, well spotted. More evidence to suggest that we are heading in the right direction😃

Requirement

In section 3.28 "Records to be maintained" it says that the auditor will ask to see: the declaration; notification of change of address / cancellation; copy of written statement; and "all correspondence".

3.9.1: 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.

3.8.2: The charity should keep a record of the cancellation of a declaration, including the date of the donor’s notification.

3.8.1: The charity must maintain an auditable record of all written statements and cancellation notices.... HMRC will accept that a written confirmation has been issued where the charity can show it has issued a standard letter template in an acceptable format and provides a list of the donors to whom the letter was sent.

So we definitely have to keep a record. To what degree the we need to have actual concrete evidence (scanned document / recording / etc) is debatable in some of the cases.

Solution

So far, we have suggested putting the cancellation/change of address on the declarations, which could mean duplicating it to multiple places, perhaps adding a free-form log message to explain. This doesn't necessarily allow saving evidence.

These records all have some similarities: need to record the correspondence date, probably the method (oral/web/etc.), maybe some other dates, and the evidence (a file attachment dependent on the method). I had some ideas around this...

We could generalise the Declaration class to become instead a Gift Aid Record. It would mean adding one more field to say the type of record. This class can cover all of the above records, possibly excepting the written statement where the current code could stay.

In this new model, the records are mostly immutable; any change is handled by adding new events with the new information. We would allow editing only to correct a data-entry error. This could allow simplification to remove entity revisions.

This is interesting looking back to the IS. Regarding the 2 solutions, we do now have a "negative declaration" stored, and now we call it a cancellation, it is in the "mental model". We could even do option 1 and allow the cancellation to dynamically override declarations, however I have my doubts, because I feel when looking at a declaration I would like to know that it has been cancelled.

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

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.

Here is my alternative suggestion to the one in the IS (the original one absolutely can work, but I feel that the balance is in favour of this one)

1. Create a new Donor class
2. Move the charity and context fields into it (from declaration)
3. Add an address field
4. Give it owner, created, revisions, log messages etc as per Declaration
5. Create a create/edit form which is simple, taking bits from Declaration
6. Create a listing page (I know it's not in the requirements, but I'll be really quick, and it helps e.g. if the user was deleted).

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

Yes I like it.

Or possibly, we should just assume this is what happens as long as the name is unchanged

I prefer the first version. The name can change and still be the same person.

I feel that the context field should potentially move from the declaration into this address.

Profiles don't seem to have an owner or created field. We likely won't be able to distinguish a self-edit from a staff one.

I have my doubts about item 6 from IS

6. If a donor or staff edit a profile referenced from a declaration, then we should create a new revision of the declaration recording who made the change.

We would need to do this for every declaration right? If so, then I have a doubt over the need for this issue😃. We could just store the address on the declaration and perform the same process of creating a new revision of each declaration.

Profiles are tied into users: I can't find any UI to create a profile independently of a user. The global profile listing page includes all profile types and there are no filters.

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.

If we create a new profile type "gift aid" then we could solve many of these, but then we have the address existing in 2 places anyway. So the choice between "Donor entity" and "gift aid profile type" is likely just a matter of ease of implementation. 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.