🇬🇧United Kingdom @adamps

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.

Answer: Don't support multiple charities - always use the default in the UI

New issue ✨ Enhancements to declaration entity Active

I split most of the "suggested changes" into the child issues, including a new one ✨ Enhancements to declaration entity Active

For discussion:

J: AFAICS "advised of liability" is a requirement of any valid declaration at the point of declaration.
A: I agree, but still two reasons.

• The audit process requires a record that this occurred. If we make a box for it, and the staff have to tick it, it makes them much more likely to remember, and should be more convincing to the auditor.
• We could receive an email saying "please add gift aid to my donations". We would naturally create a declaration at this point that is not yet valid because not advised. We could send a written record to remedy that.

J: Oral seems to require a written record unless you have an audio recording of it? That's why I had this on the DeclarationType.
A: True, but this brings a risk that we force the site owner to double the number of declaration types: each one has a variant for oral and another for non-oral. 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).

J: I was thinking to use bundles here [for method and evidence fields], 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?

adamps → created an issue.

For discussion: multiple charity support.

• Support multiple charities, and allow each to be configured separately (enable/disable and dates). Most complex / highest cost.
• Support multiple charities, but enforce they are all configured together: a single enable/disable, and the blurb lists all the names
• Variation: donor self-declaration configures them all together, staff can be separate (because they might receive a letter about just one charity)
• Don't support multiple charities

Sorry "option" was my poor choice of word, I edited my previous comment. I didn't mean that I was describing 2 possible options, I meant that the form allowed staff to do two things: "cancel all" (which was already covered in point 2) and "add".

I wonder about having a checkbox when staff create a new declaration with an end date "Enforce this end date on relevant previous declarations?"

That works well for the case of changing the end date to something still in the future. However if we are setting the end date to now, then there isn't any new declaration to create. So I guess we could have your checkbox on add, still keep the "cancel all" button, but remove the date by it.

Requirements for Donor entity

• Prevent it being deleted (except admin override)
• Have revisions with author and date
• Name and address field both mandatory
• Distinct from other non-Donor things (if using profiles, need to know which ones represent donors)
• Be clearly identified as a Donor, so anyone editing it is aware that it specifically represents name and address for Gift aid purposes
• Staff editing: requirement to provide evidence with a file/media attachment
• Staff viewing show: a list of declarations; button to add a declaration; cancellation button (with date)

Completely agree, reusing is good and we should gather all the information before deciding. My intuition at this point is towards the separate Donor entity because "Donor" and "profile" are not the same concept, and have different requirements. The Donor entity is governed by legal requirements and basically we don't want anyone else messing with it.

If we do create a new entity, then we need to look at the implications. Presumably we would copy the address from the profile, which means that the address now exists in 2 places; someone might change one and forget to change the other. However we might not always want to change both together: e.g. perhaps a customer orders a gift to send to someone else, and chooses to edit their own commerce profile rather than create a new one (slightly strange, but seems nevertheless valid). Obviously this shouldn't change the Gift Aid information.

I created a "Points for discussion" section.

Idea/suggestions for discussion. I'm looking forward to hearing your preferences.

1) Donor interface. I feel attracted to simplifying the UI for the donor's direct self-declaration. I propose we could have a form to edit a start/end date range. This allows not exposing the entity CrUD to donors at all. Behind the scenes, the code would create a new declaration and cancel the old one. We have always maximum one active declaration. We would still need to show multiple declarations for the history.

If, for example, the donor wished to declare for all the odd years 2025, 2027, 2029, but not the even ones, then they would have to wait until at least 2026 to declare for 2027. Maybe they would forget, and we would lose a little gift aid. However this case seems pretty unlikely.

2) Staff cancellation. When the donor makes a clear expression to cancel gift aid, then we need to cancel all declarations (set their end date earlier). So we need a cancel button, with a date that defaults to now.

3) Staff new declaration. Otherwise (they never mentioned the idea that they wished to cancel, we just happen to have two declarations with different dates) then I feel we can reasonably say we have permission to do either option. The second option in the staff interface is to upload a new declaration. If there is already an active declaration, then we could write code to compare the two, and automatically cancel the inferior one (generating a clear link between the two so that this cancellation can be reversed in case HMRC subsequently reject the active one).

Yes that could work.

Or another option, instead of using a profile we could make a Donor entity type. It could share code with Declaration using a trait or shared base type. This would allow both entity types to share the same set of whatever protection mechanisms we devise: revisions, author, date, log messages (the text field that describes the change, I forget the proper name), evidence fields, etc.

OK that makes sense, thanks. Probably you are right and perhaps I am worrying about nothing. However the audit process states that the auditor will ask to see the relevant Gift Aid declaration, plus various other things.

Interesting.

AFAICS your main reason for proposing using a profile is to share the information across multiple declarations. I don't think it is necessary to update the address on cancelled declarations, so this argument supposes an expectation of multiple ongoing declarations - and this is a case I would prefer to avoid😃.

Absolutely I agree it could work like this however I have some reservations. On the other hand, I see some costs/downsides. Splitting the information between 2 entities (the declaration and the profile) seems to make code more complex. We might need to create both, or just one; they have different life cycles, updated at different times by different means; the references between the entities could become complex especially if clerical errors were made at some point. The profile isn't entirely under the jurisdiction of this module and other modules might alter it. It won't necessarily have log messages, nor a trail of attached evidence.

it must keep a record of the updated information — this can be kept as an electronic copy such as a scanned document.

So we need evidence and presumably it will be on the declaration entities. But each declaration might end up have evidence that entirely fails to match it's current state, because it points to a profile that was initially matching but has since been edited. Possibly there was evidence for that edit, however it might be difficult to find that declaration that holds it or it might even have been deleted (for example because a certain number of years have elapsed from when it expired).

I feel that the concept of a declaration has a legal status in a very clearly defined way. You have some ideas, often clever, potentially a bit mind-boggling, that amount to doing things slightly differently. Whereas I feel we should do exactly what they say😃. The declaration is like a contract that was made with specific statements at a specific time in the past. I believe they want to see the contracts exactly as they were made. A particular declaration might now be outdated, but still can be needed to cover some past donations. So I would like to just mark it as outdated, and otherwise leave it exactly as it was made.

In June 2020, Miss Helen Jones of Bedford granted Gift aid indefinitely. In March 2024 Mrs Helen Smith of Edgeware (the same lady now married with a young child) grants Gift aid for the current year. Let's say she never sent us a change of address, but it's the same user account, so we have some common context. We can show HMRC either declaration and they can match it to their records. However there was no Mrs Smith living in Bedford in 2020.

Let's put this on the list of matters to discuss😃.

Yes it's good, we are roughly aligned, however I am not yet satisfied we have the best option.

I agree with your point, it's good to keep some reserve declarations in case the first ones are rejected. And I agree that if someone writes a clear expression to cancel their gift aid, and we have multiple declarations then we have to cancel them all.

But still I would like to have a single ongoing declaration (subtly but importantly not the same as a single active declaration), because otherwise I feel it will become too complex and I wonder if we could achieve this by immediately cancelling the reserve ones (in a way that could be reversed if the active is rejected). Also in most cases, I wonder if we won't have a clear expression of cancellation, and instead the donor will just have happened to grant a slightly different scope on two different occasions and we can reasonably follow whichever we prefer.

Finally I am concerned that we are designing everything "bottom up" - you have identified some complex subtle cases, and I totally agree we shouldn't neglect them. However I don't yet feel I understand how the simplest possible mainline cases will look/feel - I'd like us to look into that together then afterwards come back here and I hope the answer might be more clear.

I won't write more - it seems like the right time for a discussion as per your email.

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

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?? True, custom or contrib code could set up complex context rules. However for this module we can keep them very simple. Possibly merging of 2 contexts would need human examination of the consequences??

I feel that the main need we have from this issue is a "bulk cancel" operation which decreases the end date on all declarations in scope.

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.

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

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.

I meant to use that terminology in our discussion as it that's what we'll eventually have to write code in. I agree we don't necessarily want to expose it to users.

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.

Fair enough. But I feel there is a balance - having too many duplicates is unlikely to add value and just makes it harder to see what's going on. So we could still benefit from a feature that flags up any matching declarations.

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.

I don't feel intuitively connected to your idea that there is a special active declaration, according to some rules of your devising. It seems that at best it could only apply from the perspective of a specific donation (i.e. it varies with the donation time) and at a certain moment in time (it varies over time as declarations are altered). 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??

In terms of your first question, if we interpret the donor's instruction to be "please don't claim any more gift aid after XXX" then absolutely we have to decrease in bulk the end date of all declarations that currently extend after that time. This isn't too serious for audit, by your own theorem: they will trust us when we are reducing what we claim. However I would not wish to increase in bulk, because there's no need and it would create an audit nightmare. Firstly we'd need to duplicate the evidence onto every declaration and even worse if HMRC declined to accept the evidence that triggered the bulk update, and we'd like to restore the original ends. So this suggests to create a new declaration when rights are granted, i.e. the date is moved later, and bulk update when rights are removed.

Thanks.

Can you leave a todo flagging the question

Done

otherwise leave the code in whatever state you think best?

That's exactly the description of this issue😃. Not necessarily a final resting point, but at least a bit more consistent.

From 🐛 Fix problems arising from "Consolidate tests" Active :

Existing comment/API:

* If there are multiple ongoing date-based declarations for this charity in this context,
* then get the one with no end date or the end date furthest in the future.

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

A: Sorting by declared date we can do via SQL. Sorting by end date with special handling of NULL, probably we can't, see https://stackoverflow.com/a/12767777 comment by user330315.

it used \Datetime to get the current time, not Drupal's time service. That's untestable and not best practice.

Good point, fixed

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

This handled by the less-than, whereas start had a less-than-or-equals

    return ($end < $today);
    return ($start <= $today);

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.

My current thought is that getStartDate() and getEndDate() should rebuild the timezone to Europe/London:

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

OK, done. My reasoning was that setting the timezone has no effect when we are dealing with a date without a time. However I can see that it wasn't always going to work as I expected, so I've made everything explicit again.

Yes, fine to use address module.

I raised 📌 Use drupal address module Active

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

I believe the complexity of your preferred interface will prevent optimising to DB queries in 📌 Improvements to DeclarationStorage Active . Sorting by declared date we can do. Sorting by end date with special handling of NULL, probably we can't, see https://stackoverflow.com/a/12767777 comment by user330315. As I pointed out before, this function is currently not used, so I feel it's premature to keep spending time altering it.

adamps → created an issue.