Page MenuHomePhabricator

Write Donor entities when saving donation
Open, Needs TriagePublic

Description

NOTE: This is a technical subticket of T390611: Extract Donor data into tables

This ticket is about changing the donation bounded context to store donor data in normalized tables, additionally to storing it in the "data blob".
The changes are also about splitting streetAddress into streetName and houseNumber (see T369158: Extend donation bounded countext to support building numbers). Those changes will be done last.

Context

We distinguish between Domain Objects and Doctrine Entities. Both are called Donation, but are in different namespace (Domain vs. DataAccess). We have a "Translation Layer" that converts one into the other. In the course of this ticket, we will store the different subclasses of the Donor Domain object as relations of the Donation Doctrine entity. This will add code to the translation layer. We can only remove code from the Translation Layer when all applications use the new tables. These changed are decribed in different tickets.

Entity Relationship Diagram

Entity Relationship Diagram

Note: The relationship between donation and donor is 1:1 except for recurring Paypal donations, where we are creating un-exported "followup donations" that reuse the donor objects. This is done automatically by the existing code (see Donation::createFollowupDonationForPayment)

Integration details

This ticket creates a backwards-breaking change in the use case dependencies (DonorIdRepository added to AddDonationUseCase, UpdateDonorUseCase, BookDonationUseCase) and needs a major release and a manual update PR for the Fundraising Application after merging. This is described in T369160: Change Fundraising Application to support individual street name and house number fields

DO NOT MERGE the changes for this ticket right away, it needs T390843: Create database migration for donor data tables before it can be integrated in the Fundraising application.

Implementation Notes

Ideally, these sub-tasks should be done in order

1. Adding and generating IDs

  • In the next steps we'll need an ID when creating donors. Therefore, donors cannot be instantiated inside the Donation class any more. Change Donation::scrubPersonalData to receive a ScrubbedDonor instead of creating it inside the donation. You may move the donor creation to a static constructor in ScrubbedDonor. Change DatabaseDonationAnonymizer accordingly.
  • Create a DonorIdRepository interface (and a DoctrineDonorIdRepository) to generate new donor IDs. See the to Donation ID generation ( DonationIdRepository and DoctrineDonationIdRepository) as an example.

2. Changing the use cases to use the ID repository

This step has two stages

  1. adding the DonorIdRepository as a dependency to use cases that create donors, AddDonationUseCase and UpdateDonorUseCase
  2. Add an id property to all donor, address and name classes and require it in their constructors. Provide the ID to the constructors in the use case methods, AddDonationUseCase::getPersonalInfoFromRequest and UpdateDonorUseCase::updateDonor

When creating address and name classes, reuse the ID of the donor - it's a 1:1 relationship

2. Turning Donor interfaces into classes

  • Turn the Donor interface into the abstract Donor base class (which is currently called AbstractDonor), copying the existing accessors from the interface into the abstract class.
  • We want to continue to use the Null Object Pattern instead of doing null checks for addresses and names. Therefore, the Donor classes that don't use PostalAddress and CompanyName/PersonName must now intantiate the null object (e.g. ScrubbedAddress, AnonymousAddress, NoName, etc) in their getters. NOT in the constructor as they do now, because Doctrine does not call the constructor and the getter of instances that get read by Doctrine would not have the property instantiated.
  • To keep CompanyContactName but to avoid storing it as a separate table, add the companyName and contactPerson (of type PersonName) as properties to the CompanyDonor class and instantiate a CompanyContactName class in the getName getter.
  • Don't introduce street name and house number yet, this will be done as the last step of this ticket.

3. Map Domain objects as Doctrine entities

  • Create Doctrine mapping XML files for the donor classes, postal address class and two name classes. You may keep them in the config/DoctrineClassMapping directory. See https://www.doctrine-project.org/projects/doctrine-orm/en/3.3/reference/basic-mapping.html for documentation
    • Don't use autogenerated IDs, we'll generate the IDs in code (reusing the donor ID for address and name). Use <generator strategy="NONE"/> on all tables.
    • Pay attention to the naming, it must match the namespace, the files should start with WMDE.Fundraising.DonationContext.Domain.Model.
    • Define the name and address entities as one-to-one unidirectional associations.
    • Add persist and remove to the name and address associations (see Transitive persistence cascade)
  • Add a donor property to the existing Donation entity (in the DoctrineEntities namespace and as an one-to-one association mapping in the XML mapping file for Donation). For maximum backwards compatibility (until we have migrated all the data, which is another ticket), make it nullable for now

4. Changing the persistence layer

  • Change DomainToLegacyConverter to add the donor from the Donation (domain object) to the (Doctrine entity) Donation.
  • Change LegacyToDomainConverter to read the Donor from the Donation entity and assign it to the Domain object Donation. You can drop the DonorFactory. Make sure the DoctrineDonationTest still works. If the donor is null, return an ScrubbedDonor with original_donor_type as anonymous. Mark this default for null values from the database as deprecated and to be removed when the data has been migrated. At the end of this point the tests for the UpdateDonorUseCase (and maybe the DatabaseDonationAnonymizer tests) might fail. They should pass again when adapt the DoctrineDonationRepository (see next paragraph). TODO: Link to data migration ticket here.
  • Change the DoctrineDonationRepository: When updating donations, check the ID of the donors of the entity vs the domain object. if they don't match, it means that code has changed the donor type (e.g. through scrubbing or in the UpdateDonor use case). When you encounter this, you must delete the old entity. Ideally, the following sample code should work:
$oldDonor = $donationEntity->getDonor();
$donationEntity->setDonor($donation->getDonor());
$entityManager->persist($donationEntity);
$entityManager->remove($oldDonor);
$entityManager->flush();

The persistence cascade should remove all the lingering dependent entities (address and name).

  • Thoroughly test the deletion of data when one donor is replaced with a different one! The best way to do this is to implement the feature using TDD. This is important for data protection compliance (see next step, it will use this override functionality). Put the tests in DoctrineDonationRepositoryTest.

Allow for scrubbing personal data

  • Change the PersonalDataBackup class to emit 2 addional backup requests for the all tables that contain personal data (i.e. not the ScrubbedDonors and the AnonymousDonors table from the ER diagram), using the BackupTableConfiguration class. You can leave the condition property empty, except for the Donor table where the condition should be donor_type NOT IN ('scrubbed_donor', 'anonymous_donor)
  • Change the condition in PersonalDataBackup for the spenden (donations) table to dump only the donations where the donor is not scrubbed/anonymous. You will have to do a subselect, since mysqldump does not do JOINs. The condition need additional need more filtering SQL which might look like this AND donor_id IN (SELECT id FROM donor WHERE donor_type NOT IN ('scrubbed_donor', 'anonymous_donor))
  • Change the DatabaseDonationAnonymizer to replace the Donor of the Doctrine Donation entity with a ScrubbedDonor instance, preserving the previous donor type and salutation. Adapt the existing tests for the anonymizer to checkl both the data blob and the donor database (the donor in the donor database should be of type scrubbed (or whatever the iednetifier for ScrubbedDonor is)).

Optimize the performance when loading donations

Use a throwaway script or the debugger to observe database behavior (executed SQL) of DoctrineDonationRepository::getDoctrineDonationById and the query behavior when you access the donor property of the entity. If the behavior shows that Doctrine lazy-loads the donor (multiple SQL statements), yopu can do one of the following things

Change BookDonationUseCase to create ScrubbedDonors when creating followup donations

With the code so far, we could get into a situation where a non-scrubbed donor is reused (duplicated) and then the scrubbing fails because the donor can't be deleted because it's used in 2 places.

  • Add DonorIdRepository as a dependency to BookDonationUseCase
  • When result from payment is a FollowUpSuccessResponse, create a new ScrubbedDonor (with data from the original donor) and create a new donation with the data from the original one. Move the code from Donation::createFollowupDonationForPayment into BookDonationUseCase. Remove Donation::createFollowupDonationForPayment method. Adapt the tests accordingly
  • Test the new behavior in BookDonationUseCasetest ("WhenCreatingAFollowUpDonation_DonorOfFollowUpDonationIsAScrubbedDonorWithDifferentIdFromOriginalDonation")

We'll keep the concept of "Followup Donations", changing this concept and improving the domain are out of scope and tracked in T392471: Don't do "Followup Donations"

NOTE: We got to this point in the story time, will discuss more in the next story time

Split StreetAddress

  • Add house number and street name properties to the PostalAddress class.
  • Add house number and street name getters to the Address interface. Return empty values for the null address and the properties for PostalAddress
  • change getStreetAddress (of PostalAddress to return a concatenated value) and mark it as deprecated

For the PostalAddress, you may already add house number and street name getters and change getStreetAddress to return a concatenated value. Tip: To avoid breaking all tests in AddDonationUseCase and UpdateDonorUseCase you may leave use the street name as a combined field and leave the house number empty. This is a temporary measure until you implement the other steps below

Event Timeline

TBD: add most recent screenshot / SVG image of the new Entity Relationship Diagram for these changes

For maximum backwards compatibility (until we have migrated all the data, which is another ticket), make it nullable for now

TBD: create a ticket for undoing the "nullable" in the future