Support street name and house number

[Sperling-0]

Ticket: https://phabricator.wikimedia.org/T369158

[Abban]

You're getting there. I've left a couple of suggestions, and don't forget to add tests for every class you changed. We especially want to test that the data makes it through the use cases and converters intact.

[Abban]

You should make the constructor private, as it's not deprecated. Then look for everywhere it's used and replace it with PostalAddress::fromLegacyStreetName()

[Abban]

You should add a method like this to the DonorFieldMapper:

private function getStreetAddress( Address $address ): string {
    if ( $address->getStreetAddress() != '' ) {
       return $address->getStreetAddress();
    }
    return "{$address->getStreetName()} {$address->getHouseNumber()}";
}

Then your return array would be:

return [
	'strasse' => $this->getStreetAddress( $address ),
	'plz' => $address->getPostalCode(),
	'ort' => $address->getCity(),
	'country' => $address->getCountryCode(),
];

[gbirke]

This PR needs to be improved in several ways (most of them already
outlined in the ticket):

1. We could have a quick discussion on where to store the new fields:
   • in the "data blob", to be consistent, avoid the need for a migration and have an easy backwards compatibility?
   • In the donation table. This would distribute the address data across the table and the data blob. It would also make it query-able and could be seen as a good step towards getting rid of the data blob.
   • In a separate table. I've outlined extracting the donor data in a separate table in https://phabricator.wikimedia.org/T203679 But this would be a large refactoring that's probably out of scope for this ticket. It will remove a lot of code in the long run, but affects several code parts (donation confirmation, donor address update, analysis and reports in FOC). The perspective of this refactoring should inform our choice of going with the data blob or the columns - what will be easier to refactor in the future?
2. The changes need to be backwards-compatible: setting streetName and houseNumber should both update the streetAddress (strasse) field in the data blob, otherwise we'll run into trouble when exporting the data and showing it in the FOC.
3. Let's make it more explicit what type of address we have. I'd recommend having two classes, PostalAddress and LegacyPostalAddress (or, if you run into typing trouble
   with that, an abstract AbstractPostalAddress that has two children). The classes can then have their own implementation for getting the legacy streetAddress field and you don't need the two constructors. We also reduce the cyclomatic complexity of the code (fewer if statements), because the only place where we have to check is DonorFactory: If the Doctrine entity (or its data blob) has a non-empty streetName then create a PostalAddress, otherwise create a LegacyPostalAddress
4. If we're going with database columns, don't make them nullable (both in DB definition and property definition). A nullable value forces us to do null checks (more code, more potential complaints from PHPStan when the null check is missing) but for these fields an empty string check is fully sufficient.
5. If we choose to store the data in table columns (see 1), we must add code to LegacyToDomainConverter and DomainToLegacyConverter that fills the fields correctly (and also clears the field for scrubbed donors, please write a test case for that, e.g. testGivenDonationWithAddressData_updatingDonationAsScrubbedWillClearAddressColumn). DonorFieldMapper is only for the
   data blob.

[gbirke]

This will return a non-empty string for anonymous and scrubbed addresses. I know that there is an if-statement that guards against this, but if we ever unwittingly call getStreetAddress from another place, we're opening ourselves up for unexpected data.

[gbirke]

This will always produce a legacy postal address, even when the underlying data is non-legacy. See my review comment for recommendations.

[Sperling-0]

Note

This PR will be worked on later.
https://phabricator.wikimedia.org/T369158