Fix checking of user ids when building requests

lmc-eu · Jan 8, 2018 · 2c41ec4 · 2c41ec4
1 parent ffb7420
commit 2c41ec4
Show file tree

Hide file tree

Showing 10 changed files with 172 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,9 @@
 ### Changed
 - Unify `self`/`static`/`$this` return typehints.
 
+### Fixed
+- Checking of consistent user ids on `recommendation()` and `sorting()` requests was not working in accordance with API behavior.
+
 ## 1.1.0 - 2017-12-20
 ### Added
 - Endpoint to get all defined item properties in the Matej database (`$matej->request()->getItemProperties()`).

diff --git a/src/Exception/LogicException.php b/src/Exception/LogicException.php
@@ -23,6 +23,17 @@ public static function forInconsistentUserId(UserAwareInterface $mainCommand, Us
         return new static($message);
     }
 
+    public static function forInconsistentUserMergeAndInteractionCommand($userMergeId, $interactionUserId)
+    {
+        $message = sprintf(
+            'Source user in UserMerge command ("%s") must be the same as user in Interaction command ("%s")',
+            $userMergeId,
+            $interactionUserId
+        );
+
+        return new self($message);
+    }
+
     public static function forClassNotExtendingOtherClass($class, $wantedClass)
     {
         return new self(sprintf('Class %s has to be instance or subclass of %s.', $class, $wantedClass));

diff --git a/src/Model/Command/UserMerge.php b/src/Model/Command/UserMerge.php
@@ -46,6 +46,11 @@ public function getUserId(): string
         return $this->targetUserId;
     }
 
+    public function getSourceUserId(): string
+    {
+        return $this->sourceUserId;
+    }
+
     protected function setSourceUserId(string $sourceUserId): void
     {
         Assertion::typeIdentifier($sourceUserId);

diff --git a/src/RequestBuilder/RecommendationRequestBuilder.php b/src/RequestBuilder/RecommendationRequestBuilder.php
@@ -47,7 +47,8 @@ public function setInteraction(Interaction $interaction): self
 
     public function build(): Request
     {
-        $this->assertConsistentUsersInCommands();
+        $this->assertInteractionUserId();
+        $this->assertUserMergeUserId();
 
         return new Request(
             static::ENDPOINT_PATH,
@@ -58,14 +59,41 @@ public function build(): Request
         );
     }
 
-    private function assertConsistentUsersInCommands(): void
+    /**
+     * Assert that interaction user ids are ok:
+     * - (A,  null,  A)
+     * - (A, A -> ?, ?)
+     */
+    private function assertInteractionUserId(): void
     {
-        $mainCommandUser = $this->userRecommendationCommand->getUserId();
-        if ($this->interactionCommand !== null && $mainCommandUser !== $this->interactionCommand->getUserId()) {
+        if ($this->interactionCommand === null) {
+            return;
+        }
+
+        $interactionUserId = $this->interactionCommand->getUserId();
+
+        // (A, null, A)
+        if ($this->userMergeCommand === null && $interactionUserId !== $this->userRecommendationCommand->getUserId()) {
             throw LogicException::forInconsistentUserId($this->userRecommendationCommand, $this->interactionCommand);
         }
 
-        if ($this->userMergeCommand !== null && $mainCommandUser !== $this->userMergeCommand->getUserId()) {
+        // (A, A -> ?, ?)
+        if ($this->userMergeCommand !== null && $interactionUserId !== $this->userMergeCommand->getSourceUserId()) {
+            throw LogicException::forInconsistentUserMergeAndInteractionCommand(
+                $this->userMergeCommand->getSourceUserId(),
+                $interactionUserId
+            );
+        }
+    }
+
+    /**
+     * Assert user merge id is ok:
+     * (?, ? -> A, A)
+     */
+    private function assertUserMergeUserId(): void
+    {
+        if ($this->userMergeCommand !== null
+            && $this->userMergeCommand->getUserId() !== $this->userRecommendationCommand->getUserId()) {
             throw LogicException::forInconsistentUserId($this->userRecommendationCommand, $this->userMergeCommand);
         }
     }

diff --git a/src/RequestBuilder/SortingRequestBuilder.php b/src/RequestBuilder/SortingRequestBuilder.php
@@ -47,8 +47,10 @@ public function setInteraction(Interaction $interaction): self
 
     public function build(): Request
     {
-        $this->assertConsistentUsersInCommands();
+        $this->assertInteractionUserId();
+        $this->assertUserMergeUserId();
 
+        // Build request
         return new Request(
             static::ENDPOINT_PATH,
             RequestMethodInterface::METHOD_POST,
@@ -58,14 +60,41 @@ public function build(): Request
         );
     }
 
-    private function assertConsistentUsersInCommands(): void
+    /**
+     * Assert that interaction user ids are ok:
+     * - (A,  null,  A)
+     * - (A, A -> ?, ?)
+     */
+    private function assertInteractionUserId(): void
     {
-        $mainCommandUser = $this->sortingCommand->getUserId();
-        if ($this->interactionCommand !== null && $mainCommandUser !== $this->interactionCommand->getUserId()) {
+        if ($this->interactionCommand === null) {
+            return;
+        }
+
+        $interactionUserId = $this->interactionCommand->getUserId();
+
+        // (A, null, A)
+        if ($this->userMergeCommand === null && $interactionUserId !== $this->sortingCommand->getUserId()) {
             throw LogicException::forInconsistentUserId($this->sortingCommand, $this->interactionCommand);
         }
 
-        if ($this->userMergeCommand !== null && $mainCommandUser !== $this->userMergeCommand->getUserId()) {
+        // (A, A -> ?, ?)
+        if ($this->userMergeCommand !== null && $interactionUserId !== $this->userMergeCommand->getSourceUserId()) {
+            throw LogicException::forInconsistentUserMergeAndInteractionCommand(
+                $this->userMergeCommand->getSourceUserId(),
+                $interactionUserId
+            );
+        }
+    }
+
+    /**
+     * Assert user merge id is ok:
+     * (?, ? -> A, A)
+     */
+    private function assertUserMergeUserId(): void
+    {
+        if ($this->userMergeCommand !== null
+            && $this->userMergeCommand->getUserId() !== $this->sortingCommand->getUserId()) {
             throw LogicException::forInconsistentUserId($this->sortingCommand, $this->userMergeCommand);
         }
     }

diff --git a/tests/integration/RequestBuilder/RecommendationRequestBuilderTest.php b/tests/integration/RequestBuilder/RecommendationRequestBuilderTest.php
@@ -20,7 +20,7 @@ public function shouldExecuteRecommendationRequestOnly(): void
     {
         $response = $this->createMatejInstance()
             ->request()
-            ->recommendation($this->createRecommendationCommand())
+            ->recommendation($this->createRecommendationCommand('user-a'))
             ->send();
 
         $this->assertInstanceOf(RecommendationsResponse::class, $response);
@@ -33,8 +33,8 @@ public function shouldExecuteRecommendationRequestWithUserMergeAndInteraction():
     {
         $response = $this->createMatejInstance()
             ->request()
-            ->recommendation($this->createRecommendationCommand())
-            ->setUserMerge(UserMerge::mergeInto('user-a', 'user-b'))
+            ->recommendation($this->createRecommendationCommand('user-b'))
+            ->setUserMerge(UserMerge::mergeInto('user-b', 'user-a'))
             ->setInteraction(Interaction::bookmark('user-a', 'item-a'))
             ->send();
 
@@ -43,10 +43,10 @@ public function shouldExecuteRecommendationRequestWithUserMergeAndInteraction():
         $this->assertShorthandResponse($response, 'OK', 'OK', 'OK');
     }
 
-    private function createRecommendationCommand(): UserRecommendation
+    private function createRecommendationCommand(string $username): UserRecommendation
     {
         return UserRecommendation::create(
-            'user-a',
+            $username,
             5,
             'integration-test-scenario',
             0.50,

diff --git a/tests/integration/RequestBuilder/SortingRequestTest.php b/tests/integration/RequestBuilder/SortingRequestTest.php
@@ -33,8 +33,8 @@ public function shouldExecuteSortingRequestWithUserMergeAndInteraction(): void
     {
         $response = $this->createMatejInstance()
             ->request()
-            ->sorting(Sorting::create('user-a', ['item-a', 'item-b', 'itemC-c']))
-            ->setUserMerge(UserMerge::mergeInto('user-a', 'user-b'))
+            ->sorting(Sorting::create('user-b', ['item-a', 'item-b', 'itemC-c']))
+            ->setUserMerge(UserMerge::mergeInto('user-b', 'user-a'))
             ->setInteraction(Interaction::bookmark('user-a', 'item-a'))
             ->send();
 

diff --git a/tests/unit/Model/Command/UserMergeTest.php b/tests/unit/Model/Command/UserMergeTest.php
@@ -37,5 +37,6 @@ private function assertUserMergeCommand($command, string $sourceUserId, string $
             $command->jsonSerialize()
         );
         $this->assertSame($targetUserId, $command->getUserId());
+        $this->assertSame($sourceUserId, $command->getSourceUserId());
     }
 }
diff --git a/tests/unit/RequestBuilder/RecommendationRequestBuilderTest.php b/tests/unit/RequestBuilder/RecommendationRequestBuilderTest.php
@@ -26,7 +26,7 @@ public function shouldBuildRequestWithCommands(): void
         $recommendationsCommand = UserRecommendation::create('userId1', 5, 'test-scenario', 0.5, 3600);
         $builder = new RecommendationRequestBuilder($recommendationsCommand);
 
-        $interactionCommand = Interaction::detailView('userId1', 'itemId1');
+        $interactionCommand = Interaction::detailView('sourceId1', 'itemId1');
         $builder->setInteraction($interactionCommand);
 
         $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('sourceId1', 'userId1');
@@ -76,7 +76,7 @@ public function shouldSendRequestViaRequestManager(): void
     }
 
     /** @test */
-    public function shouldThrowExceptionWhenUserOfInteractionDiffersFromSorting(): void
+    public function shouldThrowExceptionWhenInteractionIsForUnrelatedUser(): void
     {
         $builder = new RecommendationRequestBuilder(
             $recommendationsCommand = UserRecommendation::create('userId1', 5, 'scenario', 0.5, 3600)
@@ -93,7 +93,7 @@ public function shouldThrowExceptionWhenUserOfInteractionDiffersFromSorting(): v
     }
 
     /** @test */
-    public function shouldThrowExceptionWhenUserOfUserMergeDiffersFromSorting(): void
+    public function shouldThrowExceptionWhenMergeIsForUnrelatedUser(): void
     {
         $builder = new RecommendationRequestBuilder(
             $recommendationsCommand = UserRecommendation::create('userId1', 5, 'scenario', 0.5, 3600)
@@ -108,4 +108,41 @@ public function shouldThrowExceptionWhenUserOfUserMergeDiffersFromSorting(): voi
         );
         $builder->build();
     }
+
+    /**
+     * ([interaction], [user merge], [recommendation]): (A, A -> B, B)
+     * @test
+     */
+    public function shouldPassOnCorrectSequenceOfUsersWhenMerging(): void
+    {
+        $interactionCommand = Interaction::purchase('test-user-a', 'test-item-id');
+        $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('test-user-a', 'test-user-b');
+        $recommendationsCommand = UserRecommendation::create('test-user-b', 5, 'scenario', 0.5, 3600);
+
+        $builder = new RecommendationRequestBuilder($recommendationsCommand);
+        $builder->setUserMerge($userMergeCommand);
+        $builder->setInteraction($interactionCommand);
+        $this->assertInstanceOf(Request::class, $builder->build());
+    }
+
+    /**
+     * ([interaction], [user merge], [recommendation]): (A, B -> A, A)
+     * @test
+     */
+    public function shouldFailOnIncorrectSequenceOfUsersWhenMerging(): void
+    {
+        $interactionCommand = Interaction::purchase('test-user-a', 'test-item-id');
+        $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('test-user-b', 'test-user-a');
+        $recommendationsCommand = UserRecommendation::create('test-user-a', 5, 'scenario', 0.5, 3600);
+
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage(
+            'Source user in UserMerge command ("test-user-b") must be the same as user in Interaction command ("test-user-a")'
+        );
+
+        $builder = new RecommendationRequestBuilder($recommendationsCommand);
+        $builder->setUserMerge($userMergeCommand);
+        $builder->setInteraction($interactionCommand);
+        $builder->build();
+    }
 }
diff --git a/tests/unit/RequestBuilder/SortingRequestBuilderTest.php b/tests/unit/RequestBuilder/SortingRequestBuilderTest.php
@@ -26,7 +26,7 @@ public function shouldBuildRequestWithCommands(): void
         $sortingCommand = Sorting::create('userId1', ['itemId1', 'itemId2']);
         $builder = new SortingRequestBuilder($sortingCommand);
 
-        $interactionCommand = Interaction::detailView('userId1', 'itemId1');
+        $interactionCommand = Interaction::detailView('sourceId1', 'itemId1');
         $builder->setInteraction($interactionCommand);
 
         $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('sourceId1', 'userId1');
@@ -101,4 +101,41 @@ public function shouldThrowExceptionWhenUserOfUserMergeDiffersFromSorting(): voi
         );
         $builder->build();
     }
+
+    /**
+     * ([interaction], [user merge], [sorting]): (A, A -> B, B)
+     * @test
+     */
+    public function shouldPassOnCorrectSequenceOfUsersWhenMerging(): void
+    {
+        $interactionCommand = Interaction::purchase('test-user-a', 'test-item-id');
+        $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('test-user-a', 'test-user-b');
+        $sortingCommand = Sorting::create('test-user-b', ['itemId1', 'itemId2']);
+
+        $builder = new SortingRequestBuilder($sortingCommand);
+        $builder->setUserMerge($userMergeCommand);
+        $builder->setInteraction($interactionCommand);
+        $this->assertInstanceOf(Request::class, $builder->build());
+    }
+
+    /**
+     * ([interaction], [user merge], [sorting]): (A, B -> A, A)
+     * @test
+     */
+    public function shouldFailOnIncorrectSequenceOfUsersWhenMerging(): void
+    {
+        $interactionCommand = Interaction::purchase('test-user-a', 'test-item-id');
+        $userMergeCommand = UserMerge::mergeFromSourceToTargetUser('test-user-b', 'test-user-a');
+        $sortingCommand = Sorting::create('test-user-a', ['itemId1', 'itemId2']);
+
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage(
+            'Source user in UserMerge command ("test-user-b") must be the same as user in Interaction command ("test-user-a")'
+        );
+
+        $builder = new SortingRequestBuilder($sortingCommand);
+        $builder->setUserMerge($userMergeCommand);
+        $builder->setInteraction($interactionCommand);
+        $builder->build();
+    }
 }