Update Person::getLastAddress() based on feedback.

src/Bundle/ChillPersonBundle/Entity/AccompanyingPeriod.php

@@ -25,6 +25,7 @@ namespace Chill\PersonBundle\Entity;
 use Doctrine\ORM\Mapping as ORM;
 use Symfony\Component\Validator\Context\ExecutionContextInterface;
 use Chill\MainBundle\Entity\User;
+use DateTimeImmutable;
 
 /**
  * AccompanyingPeriod
@@ -34,11 +35,9 @@ class AccompanyingPeriod
     /** @var integer */
     private $id;
 
-    /** @var \DateTime */
+    private DateTimeImmutable $openingDate;
-    private $openingDate;
 
-    /** @var \DateTime */
+    private ?DateTimeImmutable $closingDate = null;
-    private $closingDate;
 
     /** @var string */
     private $remark = '';
@@ -58,10 +57,10 @@ class AccompanyingPeriod
 
     /**
      *
-     * @param \DateTime $dateOpening
+     * @param \DateTimeImmutable $dateOpening
      * @uses AccompanyingPeriod::setClosingDate()
      */
-    public function __construct(\DateTime $dateOpening) {
+    public function __construct(\DateTimeImmutable $dateOpening) {
         $this->setOpeningDate($dateOpening);
     }
 
@@ -75,25 +74,14 @@ class AccompanyingPeriod
         return $this->id;
     }
 
-    /**
+    public function setOpeningDate(DateTimeImmutable $openingDate): self
-     * Set openingDate
-     *
-     * @param \DateTime $dateOpening
-     * @return AccompanyingPeriod
-     */
-    public function setOpeningDate($openingDate)
     {
         $this->openingDate = $openingDate;
 
         return $this;
     }
 
-    /**
+    public function getOpeningDate(): DateTimeImmutable
-     * Get openingDate
-     *
-     * @return \DateTime 
-     */
-    public function getOpeningDate()
     {
         return $this->openingDate;
     }
@@ -102,24 +90,15 @@ class AccompanyingPeriod
      * Set closingDate
      *
      * For closing a Person file, you should use Person::setClosed instead.
-     *
-     * @param \DateTime $dateClosing
-     * @return AccompanyingPeriod
-     * 
      */
-    public function setClosingDate($closingDate)
+    public function setClosingDate(DateTimeImmutable $closingDate): self
     {
         $this->closingDate = $closingDate;
 
         return $this;
     }
 
-    /**
+    public function getClosingDate(): ?DateTimeImmutable
-     * Get closingDate
-     *
-     * @return \DateTime 
-     */
-    public function getClosingDate()
     {
         return $this->closingDate;
     }
@@ -130,15 +109,15 @@ class AccompanyingPeriod
      */
     public function isOpen(): bool
     {
-        if ($this->getOpeningDate() > new \DateTime('now')) {
+        if ($this->getOpeningDate() > new \DateTimeImmutable('now')) {
             return false;
         }
 
         if ($this->getClosingDate() === null) {
             return true;
-        } else {
-            return false;
         }
+
+        return false;
     }
 
     /**

src/Bundle/ChillPersonBundle/Entity/Person.php

@@ -28,7 +28,10 @@ use Chill\PersonBundle\Entity\MaritalStatus;
 use Doctrine\Common\Collections\ArrayCollection;
 use Chill\MainBundle\Entity\HasCenterInterface;
 use Chill\MainBundle\Entity\Address;
+use DateTime;
+use DateTimeImmutable;
 use Doctrine\Common\Collections\Criteria;
+use Symfony\Component\VarDumper\VarDumper;
 
 /**
  * Person
@@ -125,14 +128,14 @@ class Person implements HasCenterInterface {
      */
     private $fullnameCanonical;
 
-    public function __construct(\DateTime $opening = null) {
+    public function __construct(\DateTimeImmutable $opening = null) {
         $this->accompanyingPeriods = new ArrayCollection();
         $this->spokenLanguages = new ArrayCollection();
         $this->addresses = new ArrayCollection();
         $this->altNames = new ArrayCollection();
 
         if ($opening === null) {
-            $opening = new \DateTime();
+            $opening = new \DateTimeImmutable();
         }
 
         $this->open(new AccompanyingPeriod($opening));
@@ -745,27 +748,28 @@ class Person implements HasCenterInterface {
      * By default, the addresses are ordered by date, descending (the most
      * recent first)
      *
-     * @return \Chill\MainBundle\Entity\Address[]
+     * @return ArrayCollection
      */
-    public function getAddresses()
+    public function getAddresses(): ArrayCollection
     {
         return $this->addresses;
     }
 
-    public function getLastAddress(\DateTime $date = null)
+    public function getLastAddress(?DateTimeImmutable $from = null): ?Address
     {
-        if ($date === null) {
+        $from ??= new DateTimeImmutable('now');
-            $date = new \DateTime('now');
-        }
 
-        $addresses = $this->getAddresses();
+        $addressesIterator = $this->getAddresses()
+            ->filter(static fn (Address $address): bool => $address->getValidFrom() <= $from)
+            ->getIterator();
 
-        if ($addresses == null) {
+        $addressesIterator->uasort(
+            static fn (Address $left, Address $right): int => $right->getValidFrom() <=> $left->getValidFrom()
+        );
 
-            return null;
+        return [] === ($addresses = iterator_to_array($addressesIterator)) ?
-        }
+            null :
-
+            current($addresses);
-        return $addresses->first();
     }
 
     /**

src/Bundle/ChillPersonBundle/Tests/Entity/PersonTest.php

@@ -25,6 +25,10 @@ namespace Chill\PersonBundle\Tests\Entity;
 use Chill\PersonBundle\Entity\Person;
 use Chill\PersonBundle\Entity\AccompanyingPeriod;
 use Chill\MainBundle\Entity\Address;
+use DateInterval;
+use DateTime;
+use DateTimeImmutable;
+use Generator;
 
 /**
  * Unit tests for the person Entity
@@ -38,7 +42,7 @@ class PersonTest extends \PHPUnit\Framework\TestCase
      */
     public function testGetCurrentAccompanyingPeriod()
     {
-        $d = new \DateTime('yesterday'); 
+        $d = new \DateTimeImmutable('yesterday');
         $p = new Person($d);
 
         $period = $p->getCurrentAccompanyingPeriod();
@@ -48,7 +52,7 @@ class PersonTest extends \PHPUnit\Framework\TestCase
         $this->assertEquals($d, $period->getOpeningDate());
 
         //close and test
-        $period->setClosingDate(new \DateTime('tomorrow'));
+        $period->setClosingDate(new \DateTimeImmutable('tomorrow'));
 
         $shouldBeNull = $p->getCurrentAccompanyingPeriod();
         $this->assertNull($shouldBeNull);
@@ -60,17 +64,17 @@ class PersonTest extends \PHPUnit\Framework\TestCase
      */
     public function testAccompanyingPeriodOrderWithUnorderedAccompanyingPeriod()
     {
-        $d = new \DateTime("2013/2/1");
+        $d = new \DateTimeImmutable("2013/2/1");
         $p = new Person($d);
 
-        $e = new \DateTime("2013/3/1");
+        $e = new \DateTimeImmutable("2013/3/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($e);
         $p->close($period);
 
-        $f = new \DateTime("2013/1/1");
+        $f = new \DateTimeImmutable("2013/1/1");
         $p->open(new AccompanyingPeriod($f));
 
-        $g = new \DateTime("2013/4/1"); 
+        $g = new \DateTimeImmutable("2013/4/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($g);
         $p->close($period);
 
@@ -86,17 +90,17 @@ class PersonTest extends \PHPUnit\Framework\TestCase
      * starting at the same time order regarding to the closing date.
      */
     public function testAccompanyingPeriodOrderSameDateOpening() {
-        $d = new \DateTime("2013/2/1");
+        $d = new \DateTimeImmutable("2013/2/1");
         $p = new Person($d);
 
-        $g = new \DateTime("2013/4/1"); 
+        $g = new \DateTimeImmutable("2013/4/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($g);
         $p->close($period);
 
-        $f = new \DateTime("2013/2/1");
+        $f = new \DateTimeImmutable("2013/2/1");
         $p->open(new AccompanyingPeriod($f));
 
-        $e = new \DateTime("2013/3/1");
+        $e = new \DateTimeImmutable("2013/3/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($e);
         $p->close($period);
 
@@ -113,17 +117,17 @@ class PersonTest extends \PHPUnit\Framework\TestCase
      * is covering another one : start_1 < start_2 & end_2 < end_1
      */
     public function testDateCoveringWithCoveringAccompanyingPeriod() {
-        $d = new \DateTime("2013/2/1");
+        $d = new \DateTimeImmutable("2013/2/1");
         $p = new Person($d);
 
-        $e = new \DateTime("2013/3/1");
+        $e = new \DateTimeImmutable("2013/3/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($e);
         $p->close($period);
 
-        $f = new \DateTime("2013/1/1");
+        $f = new \DateTimeImmutable("2013/1/1");
         $p->open(new AccompanyingPeriod($f));
 
-        $g = new \DateTime("2013/4/1"); 
+        $g = new \DateTimeImmutable("2013/4/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($g);
         $p->close($period);
 
@@ -138,14 +142,14 @@ class PersonTest extends \PHPUnit\Framework\TestCase
      * before an existing period
      */
     public function testNotOpenAFileReOpenedLater() {
-        $d = new \DateTime("2013/2/1");
+        $d = new \DateTimeImmutable("2013/2/1");
         $p = new Person($d);
 
-        $e = new \DateTime("2013/3/1");
+        $e = new \DateTimeImmutable("2013/3/1");
         $period = $p->getCurrentAccompanyingPeriod()->setClosingDate($e);
         $p->close($period);
 
-        $f = new \DateTime("2013/1/1");
+        $f = new \DateTimeImmutable("2013/1/1");
         $p->open(new AccompanyingPeriod($f));
 
         $r = $p->checkAccompanyingPeriodsAreNotCollapsing();
@@ -153,46 +157,45 @@ class PersonTest extends \PHPUnit\Framework\TestCase
         $this->assertEquals($r['result'], Person::ERROR_ADDIND_PERIOD_AFTER_AN_OPEN_PERIOD);
     }
 
-    public function testGetLastAddress()
+    public function dateProvider(): Generator
     {
-        $date0 = (\DateTime::createFromFormat('Y-m-d', '2021-02-05'))->settime(0,0);
+        yield [(DateTimeImmutable::createFromFormat('Y-m-d', '2021-01-05'))->settime(0, 0)];
-        $date1 = (\DateTime::createFromFormat('Y-m-d', '2021-03-05'))->settime(0,0);
+        yield [(DateTimeImmutable::createFromFormat('Y-m-d', '2021-02-05'))->settime(0, 0)];
-        $date2 = (\DateTime::createFromFormat('Y-m-d', '2021-04-05'))->settime(0,0);
+        yield [(DateTimeImmutable::createFromFormat('Y-m-d', '2021-03-05'))->settime(0, 0)];
-        $date3 = (\DateTime::createFromFormat('Y-m-d', '2021-05-05'))->settime(0,0);
+    }
-        $date4 = (\DateTime::createFromFormat('Y-m-d', '2021-06-05'))->settime(0,0);
-        $date5 = (\DateTime::createFromFormat('Y-m-d', '2021-07-05'))->settime(0,0);
-        $date6 = (\DateTime::createFromFormat('Y-m-d', '2021-08-05'))->settime(0,0);
-        $p = new Person($date1);
 
-        $this->assertNull($p->getLastAddress($date1));
+    /**
+     * @dataProvider dateProvider
+     */
+    public function testGetLastAddress(DateTimeImmutable $date)
+    {
+        $p = new Person($date);
 
-        // add some address
+        // Make sure that there is no last address.
-        $add1 = (new Address())->setValidFrom($date1);
+        $this->assertNull($p->getLastAddress());
-        // no address with date 2
-        $add3 = (new Address())->setValidFrom($date3);
-        // no address with date 4
-        $add5 = (new Address())->setValidFrom($date5);
 
-        $p->addAddress($add1);
+        // Take an arbitrary date before the $date in parameter.
+        $addressDate = $date->sub(new DateInterval('PT180M'));
 
-        // test that, if only one address, that does work:
+        // 1. Smoke test: Test that the first address added is the last one.
-        $this->assertSame($add1, $p->getLastAddress($date1));
+        $address1 = (new Address())->setValidFrom($addressDate);
-        $this->assertSame($add1, $p->getLastAddress($date2));
+        $p->addAddress($address1);
-        $this->assertSame($add1, $p->getLastAddress());
 
-        // adress before the date should not work
+        $this->assertCount(1, $p->getAddresses());
-        $this->assertNull($p->getLastAddress($date0));
+        $this->assertSame($address1, $p->getLastAddress());
 
-        // add addresses
+        // 2. Add an older address, which should not be the last address.
-        $p->addAddress($add3);
+        $address2 = (new Address())->setValidFrom($addressDate->sub(new DateInterval('PT30M')));
-        $p->addAddress($add5);
+        $p->addAddress($address2);
 
-        // test retrieval of address
+        $this->assertCount(2, $p->getAddresses());
-        $this->assertSame($add1, $p->getLastAddress($date2));
+        $this->assertSame($address1, $p->getLastAddress());
-        //$this->assertSame($add3, $p->getLastAddress($date3));
+
-        dump($p->getLastAddress($date4), $add3);
+        // 3. Add a newer address, which should be the last address.
-        $this->assertSame($add3, $p->getLastAddress($date4));
+        $address3 = (new Address())->setValidFrom($addressDate->add(new DateInterval('PT30M')));
-        //$this->assertSame($add5, $p->getLastAddress($date5));
+        $p->addAddress($address3);
-        $this->assertSame($add5, $p->getLastAddress($date6));
+
+        $this->assertCount(3, $p->getAddresses());
+        $this->assertSame($address3, $p->getLastAddress());
     }
 }