#1391: SaveAssetsKeywordsInterface to delete obsolete keywords #29207

jmonteros422 · 2020-07-20T08:42:36Z

Description (*)

magento/adobe-stock-integration#1391

Related Pull Requests

Fixed Issues (if relevant)

Fixes SaveAssetsKeywordsInterface to delete obsolete keywords adobe-stock-integration#1391

Manual testing scenarios (*)

Edit Image details
Add / remove new keyword tags
Should delete old keyword asset links not specified on the parameters and insert the new ones.

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

…delete obsolete keywords - Add method to delete keywords which has no relation to assets

…delete obsolete keywords - Revised behaviour to delete obsolete asset keyword link based on updated arguments

…delete obsolete keywords - Move and improve logic for saving and deleting obsolete asset keyword links

sivaschenko

Thanks for the pull request @jmonteros422 ! Please see my comments

sivaschenko · 2020-07-20T09:00:27Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+    /**
+     * @var GetAssetsKeywordsInterface
+     */
+    private $getAssetsKeywordsInterface;


I propose to use a shorter and more precise property naming

Suggested change

private $getAssetsKeywordsInterface;

private $getAssetsKeywords;

sivaschenko · 2020-07-20T09:02:25Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+        if (!empty($currentKeywordsData)) {
+            $currentKeywords = $this->getKeywordIdsFromKeywordData(
+                $currentKeywordsData[$assetId]->getKeywords()
+            );
+
+            return $currentKeywords;
+        }
+
+        return [];


Suggested change

if (!empty($currentKeywordsData)) {

$currentKeywords = $this->getKeywordIdsFromKeywordData(

$currentKeywordsData[$assetId]->getKeywords()

);

return $currentKeywords;

}

return [];

if (empty($currentKeywordsData)) {

return [];

}

return $this->getKeywordIdsFromKeywordData(

$currentKeywordsData[$assetId]->getKeywords()

);

sivaschenko · 2020-07-20T09:04:35Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+    /**
+     * Get keyword ids from keyword data
+     *
+     * @param array $keywordsData


Suggested change

* @param array $keywordsData

* @param KeywordInterface[] $keywordsData

sivaschenko · 2020-07-20T09:04:51Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+     * Get current keyword data of an asset
+     *
+     * @param int $assetId
+     * @return array


Suggested change

* @return array

* @return int[]

sivaschenko · 2020-07-20T09:05:01Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+     * @param int $assetId
+     * @return array
+     */
+    private function getCurrentKeywords(int $assetId): array


Suggested change

private function getCurrentKeywords(int $assetId): array

private function getCurrentKeywordIds(int $assetId): array

sivaschenko · 2020-07-20T09:07:33Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+        try {
+            if (!empty($keywordIds)) {
+                $values = [];
+                $keywordsToInsert = array_diff($keywordIds, $this->getCurrentKeywords($assetId));


What do you think about moving current keywords retrieving to the execute method to retrieve them only once.
insertAssetKeywords and deleteAssetKeywords methods can accept only the needed keywordIds (the result of array_diff)

sivaschenko · 2020-07-20T09:08:33Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+     * Save new asset keyword links
+     *
+     * @param int $assetId
+     * @param array $keywordIds


Suggested change

* @param array $keywordIds

* @param int[] $keywordIds

…delete obsolete keywords - Code enhancement for SavedAssetLinks logic and updated AssetKeywordsTest

jmonteros422 · 2020-07-20T14:00:09Z

@magento run all tests

sivaschenko

Thanks for the updates @jmonteros422 ! Looks good. See my comments regarding the integration test

sivaschenko · 2020-07-20T14:16:38Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

+        $this->assertEquals(count($updatedKeywords), count($updatedLoadedKeywords));
+
+        $updatedLoadedKeywordStrings = [];
+        foreach ($updatedLoadedKeywords as $updatedLoadedKeywordObject) {


Please extract the reusable code to a private method

sivaschenko · 2020-07-20T14:17:04Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

@@ -72,6 +72,7 @@ protected function setUp(): void
    public function testSaveAndGetKeywords(array $keywords): void
    {
        $keywords = ['pear', 'plum'];


Please remove this variable for the dataprovider items to be used

…delete obsolete keywords - integration test refactor

jmonteros422 · 2020-07-21T11:17:57Z

@magento run all tests

sivaschenko

Thanks for updates @jmonteros422 ! Please see my comments

sivaschenko · 2020-07-21T13:20:51Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+    private function insertAssetKeywords(int $assetId, array $keywordIds): void
+    {
+        try {
+            if (!empty($keywordIds)) {


I'd revert the conditions to decrease the nesting level (this can be applied to other such conditions in this PR as well). Also this check can be moved out of try-catch block

if (empty($keywordIds)) {
return;
}

sivaschenko · 2020-07-21T13:21:31Z

app/code/Magento/MediaGallery/Model/ResourceModel/Keyword/SaveAssetLinks.php

+                    $values[] = [$assetId, $keywordId];
+                }
+
+                if (!empty($values)) {


I think this is an unnecessary check. the previous check should be enough in my opinion

sivaschenko · 2020-07-21T13:23:23Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

@@ -66,45 +66,57 @@ protected function setUp(): void
     *
     * @magentoDataFixture Magento/MediaGallery/_files/media_asset.php
     * @dataProvider keywordsProvider
-     * @param array $keywords
+     * @param array|null $keywords
+     * @param array|null $updatedKeywords


These parameters cannot be null, also detailed array type declaration is welcome)

Suggested change

* @param array|null $updatedKeywords

* @param string[] $updatedKeywords

sivaschenko · 2020-07-21T13:27:40Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php


-        $this->assertCount(1, $loadedAssetKeywords);
+        if (!empty($keywords)) {


I'd still perform a verification of empty keywords, can we remove this condition?

…delete obsolete keywords - Enhancements, Revision on integration test and fix unit test failure

jmonteros422 · 2020-07-21T16:43:03Z

@magento run all tests

…delete obsolete keywords - Revised try catch to resolve failure in unit tests

jmonteros422 · 2020-07-22T05:10:16Z

@magento run all tests

sivaschenko

Hi @jmonteros422 , added a couple of comments regarding the integration test

sivaschenko · 2020-07-22T11:15:34Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php


        $loadedKeywordStrings = [];
-        foreach ($loadedKeywords as $loadedKeywordObject) {
-            $loadedKeywordStrings[] = $loadedKeywordObject->getKeyword();
+        if (!empty($loadedKeywords)) {


foreach loop will not be executed if $loadedKeywords is an empty array, the condition is not necessary

sivaschenko · 2020-07-22T11:15:39Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

     * @throws \Magento\Framework\Exception\LocalizedException
     */
-    public function testSaveAndGetKeywords(array $keywords): void
+    public function testSaveAndGetKeywords(array $keywords = [], array $updatedKeywords = []): void


I don't think these arguments should be optional

sivaschenko · 2020-07-22T11:32:30Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

                'keywords' => $this->getKeywords($keywords)
            ]
        );

        $this->saveAssetsKeywords->execute([$assetKeywords]);
-        $loadedAssetKeywords = $this->getAssetsKeywords->execute([$loadedAsset->getId()]);
+        $loadedAssetKeywords = $this->getAssetsKeywords->execute([$assetId]);


For empty keywords - let's ensure empty result is returned and quit. To avoid the following code complication

if (empty($keywords)) { $this->assertEmpty($loadedAssetKeywords); return; }

sivaschenko · 2020-07-22T11:33:19Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php


+        $this->assertCount($expectedCount, $loadedAssetKeywords);


Suggested change

$this->assertCount($expectedCount, $loadedAssetKeywords);

$this->assertCount(1, $loadedAssetKeywords);

sivaschenko · 2020-07-22T11:35:21Z

dev/tests/integration/testsuite/Magento/MediaGallery/Model/ResourceModel/AssetKeywordsTest.php

+     * @param string[] $keywords
+     * @param string[] $currentKeywords
+     */
+    private function updateAssetKeywords(int $assetId, array $keywords = [], array $currentKeywords = []): void


I don't think we need the $currentKeywords parameter for this function at all, only the actual (new) keywords should be used for update and verification

…delete obsolete keywords - Allow empty keywords to execute saveAssetLinks, added condition to integration test for empty keywords

jmonteros422 · 2020-07-22T14:30:31Z

@magento run all tests

sivaschenko · 2020-07-23T12:08:14Z

Does not require manual/functional testing - covered by integration tests

…ywords #29207

m2-assistant · 2020-07-29T08:27:37Z

Hi @jmonteros422, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

jmonteros422 added 3 commits July 16, 2020 17:07

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

a3e9fea

…delete obsolete keywords - Add method to delete keywords which has no relation to assets

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

7c2511a

…delete obsolete keywords - Revised behaviour to delete obsolete asset keyword link based on updated arguments

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

d81d262

…delete obsolete keywords - Move and improve logic for saving and deleting obsolete asset keyword links

jmonteros422 requested a review from sivaschenko July 20, 2020 08:42

ghost added the Progress: pending review label Jul 20, 2020

magento-engcom-team added Component: MediaGallery Release Line: 2.4 labels Jul 20, 2020

sivaschenko requested changes Jul 20, 2020

View reviewed changes

ghost assigned sivaschenko Jul 20, 2020

ghost added Progress: needs update and removed Progress: pending review labels Jul 20, 2020

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

fe0be1d

…delete obsolete keywords - Code enhancement for SavedAssetLinks logic and updated AssetKeywordsTest

sivaschenko requested changes Jul 20, 2020

View reviewed changes

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

659071c

…delete obsolete keywords - integration test refactor

sivaschenko requested changes Jul 21, 2020

View reviewed changes

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

b166cf5

…delete obsolete keywords - Enhancements, Revision on integration test and fix unit test failure

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

cc4c5d0

…delete obsolete keywords - Revised try catch to resolve failure in unit tests

sivaschenko requested changes Jul 22, 2020

View reviewed changes

magento/adobe-stock-integration#1391: SaveAssetsKeywordsInterface to …

958fee3

…delete obsolete keywords - Allow empty keywords to execute saveAssetLinks, added condition to integration test for empty keywords

sivaschenko approved these changes Jul 23, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Jul 23, 2020

sivaschenko changed the title ~~[WIP] #1391: SaveAssetsKeywordsInterface to delete obsolete keywords~~ #1391: SaveAssetsKeywordsInterface to delete obsolete keywords Jul 23, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Jul 23, 2020

ghost added Progress: accept and removed Progress: testing in progress labels Jul 23, 2020

slavvka mentioned this pull request Jul 29, 2020

Error generating styles-m.css from Less library #1391

Closed

magento-engcom-team pushed a commit that referenced this pull request Jul 29, 2020

ENGCOM-7873: #1391: SaveAssetsKeywordsInterface to delete obsolete ke…

eca1a9b

…ywords #29207

magento-engcom-team merged commit a1fa1c6 into magento:2.4-develop Jul 29, 2020

lenaorobei added this to the 2.4.1 milestone Jul 29, 2020

	private $getAssetsKeywordsInterface;
	private $getAssetsKeywords;

	* @param array $keywordsData
	* @param KeywordInterface[] $keywordsData

	private function getCurrentKeywords(int $assetId): array
	private function getCurrentKeywordIds(int $assetId): array

	* @param array\|null $updatedKeywords
	* @param string[] $updatedKeywords


		$this->assertCount(1, $loadedAssetKeywords);
		if (!empty($keywords)) {

	$this->assertCount($expectedCount, $loadedAssetKeywords);
	$this->assertCount(1, $loadedAssetKeywords);

#1391: SaveAssetsKeywordsInterface to delete obsolete keywords #29207

#1391: SaveAssetsKeywordsInterface to delete obsolete keywords #29207

Uh oh!

Conversation

jmonteros422 commented Jul 20, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmonteros422 commented Jul 20, 2020

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmonteros422 commented Jul 21, 2020

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmonteros422 commented Jul 21, 2020

Uh oh!

jmonteros422 commented Jul 22, 2020

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmonteros422 commented Jul 22, 2020

Uh oh!

sivaschenko commented Jul 23, 2020

Uh oh!

m2-assistant bot commented Jul 29, 2020

Uh oh!

Uh oh!