Skip to content

{ASI} :- Image size is not passed to image-uploader when inserting an image from new media gallery #27388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 28, 2020
Merged

{ASI} :- Image size is not passed to image-uploader when inserting an image from new media gallery #27388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b870638
Partial Implementaion of ASI issue
konarshankar07 Mar 22, 2020
6156274
Added missing changes from previous commit
konarshankar07 Mar 22, 2020
e31dafd
Updated PHPDOC block
konarshankar07 Mar 22, 2020
57db694
Code review changes
konarshankar07 Mar 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/code/Magento/MediaGallery/Model/Asset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class Asset extends AbstractExtensibleModel implements AssetInterface
private const CONTENT_TYPE = 'content_type';
private const WIDTH = 'width';
private const HEIGHT = 'height';
private const SIZE = 'size';
private const CREATED_AT = 'created_at';
private const UPDATED_AT = 'updated_at';

Expand Down Expand Up @@ -89,6 +90,14 @@ public function getHeight(): int
return (int) $this->getData(self::HEIGHT);
}

/**
* @inheritdoc
*/
public function getSize(): int
{
return (int) $this->getData(self::SIZE);
}

/**
* @inheritdoc
*/
Expand Down
42 changes: 32 additions & 10 deletions app/code/Magento/MediaGallery/Test/Unit/Model/DataExtractorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@

use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use Magento\MediaGallery\Model\Asset;
use Magento\MediaGallery\Model\DataExtractor;
use Magento\MediaGallery\Model\Keyword;
use Magento\MediaGalleryApi\Api\Data\AssetInterface;
use Magento\MediaGalleryApi\Api\Data\KeywordInterface;
use Magento\MediaGallery\Model\DataExtractor;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

Expand Down Expand Up @@ -54,22 +53,41 @@ public function testExtractData(string $class, $interfaceClass, array $expectedD
'data' => $data,
]
);
$receivedData = $this->dataExtractor->extract($model, $interfaceClass);
$this->checkValues($expectedData, $receivedData, $model);
if ($interfaceClass) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the reason to change the test logic after adding a size field. Only data provider should be changed. What is the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to improve the Unit test for this line

magento2/app/code/Magento/MediaGallery/Test/Unit/Model/DataExtractorTest.php

Line 72 in 1a77b70

$this->assertEquals(array_keys($expectedData), array_keys($expectedData));

$receivedData = $this->dataExtractor->extract($model, $interfaceClass);
$this->checkAssetValues($expectedData, $receivedData, $model);
} else {
$receivedData = $this->dataExtractor->extract($model);
$this->checkKeyWordValues($expectedData, $receivedData, $model);
}
}

/**
* @param array $expectedData
* @param array $data
* @param object $model
*/
private function checkAssetValues(array $expectedData, array $data, $model)
{
foreach ($expectedData as $expectedDataKey => $expectedDataItem) {
$this->assertEquals($data[$expectedDataKey] ?? null, $model->{$expectedDataItem['method']}());
$this->assertEquals($data[$expectedDataKey] ?? null, $expectedDataItem['value']);
}
$this->assertEquals(array_keys($expectedData), array_keys($data));
}

/**
* @param array $expectedData
* @param array $data
* @param object $model
*/
protected function checkValues(array $expectedData, array $data, $model)
private function checkKeyWordValues(array $expectedData, array $data, $model)
{
foreach ($expectedData as $expectedDataKey => $expectedDataItem) {
$this->assertEquals($data[$expectedDataKey] ?? null, $model->{$expectedDataItem['method']}());
$this->assertEquals($data[$expectedDataKey] ?? null, $expectedDataItem['value']);
}
$this->assertEquals(array_keys($expectedData), array_keys($expectedData));
$this->assertEquals(array_keys($expectedData), array_keys(array_slice($data, 0, 2)));
}

/**
Expand Down Expand Up @@ -102,13 +120,17 @@ public function assetProvider()
'value' => 'content_type',
'method' => 'getContentType',
],
'height' => [
'value' => 4,
'method' => 'getHeight',
],
'width' => [
'value' => 3,
'method' => 'getWidth',
],
'height' => [
'value' => 4,
'method' => 'getHeight',
'size' => [
'value' => 300,
'method' => 'getSize',
],
'created_at' => [
'value' => '2019-11-28 10:40:09',
Expand All @@ -122,7 +144,7 @@ public function assetProvider()
],
'Keyword conversion without interface' => [
Keyword::class,
null,
'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this value should be either a valid interface name or null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per function argument type declarations for the method is string so instead of null I just replaced with the empty string

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it logically correct: it should be either interface name as string or null

[
'id' => [
'value' => 2,
Expand Down
1 change: 1 addition & 0 deletions app/code/Magento/MediaGallery/etc/db_schema.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<column xsi:type="varchar" name="content_type" length="255" nullable="true" comment="Content Type"/>
<column xsi:type="int" name="width" padding="10" unsigned="true" nullable="false" identity="false" default="0" comment="Width"/>
<column xsi:type="int" name="height" padding="10" unsigned="true" nullable="false" identity="false" default="0" comment="Height"/>
<column xsi:type="int" name="size" padding="10" unsigned="true" nullable="false" identity="false" comment="Asset file size in bytes"/>
<column xsi:type="timestamp" name="created_at" on_update="false" nullable="false" default="CURRENT_TIMESTAMP" comment="Created At"/>
<column xsi:type="timestamp" name="updated_at" on_update="true" nullable="false" default="CURRENT_TIMESTAMP" comment="Updated At"/>
<constraint xsi:type="primary" referenceId="PRIMARY">
Expand Down
3 changes: 2 additions & 1 deletion app/code/Magento/MediaGallery/etc/db_schema_whitelist.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"content_type": true,
"width": true,
"height": true,
"size": true,
"created_at": true,
"updated_at": true
},
Expand Down Expand Up @@ -53,4 +54,4 @@
"MEDIA_GALLERY_ASSET_KEYWORD_ASSET_ID_MEDIA_GALLERY_ASSET_ID": true
}
}
}
}
7 changes: 7 additions & 0 deletions app/code/Magento/MediaGalleryApi/Api/Data/AssetInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ public function getHeight(): int;
*/
public function getWidth(): int;

/**
* Retrieve asset file size in bytes
*
* @return int
*/
public function getSize(): int;

/**
* Get created at
*
Expand Down