Skip to content
This repository was archived by the owner on May 20, 2019. It is now read-only.

The user passed for async api referenced correct WebAPI user #22

Merged
merged 12 commits into from
Oct 10, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
*/
interface BulkSummaryInterface extends \Magento\Framework\Bulk\BulkSummaryInterface
{
const USER_TYPE = 'user_type';

/**
* Retrieve existing extension attributes object.
*
Expand All @@ -31,4 +33,18 @@ public function getExtensionAttributes();
public function setExtensionAttributes(
\Magento\AsynchronousOperations\Api\Data\BulkSummaryExtensionInterface $extensionAttributes
);

/**
* Get user type
*
* @return int
*/
public function getUserType();

/**
* Set user type
* @param int $userType
* @return $this
*/
public function setUserType($userType);
}
13 changes: 13 additions & 0 deletions app/code/Magento/AsynchronousOperations/Model/BulkManagement.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Magento\Framework\EntityManager\EntityManager;
use Magento\Framework\EntityManager\MetadataPool;
use Magento\AsynchronousOperations\Model\ResourceModel\Operation\CollectionFactory;
use Magento\Authorization\Model\UserContextInterface;

/**
* Class BulkManagement
Expand Down Expand Up @@ -51,6 +52,11 @@ class BulkManagement implements \Magento\Framework\Bulk\BulkManagementInterface
*/
private $resourceConnection;

/**
* @var \Magento\Authorization\Model\UserContextInterface
*/
private $userContext;

/**
* @var \Psr\Log\LoggerInterface
*/
Expand All @@ -73,6 +79,7 @@ public function __construct(
BulkPublisherInterface $publisher,
MetadataPool $metadataPool,
ResourceConnection $resourceConnection,
UserContextInterface $userContext,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This constructor exists in Magento Commerce 2.2 so new parameters should be added corresponding to backward compatibility policies (at the end as optional)

\Psr\Log\LoggerInterface $logger
) {
$this->entityManager = $entityManager;
Expand All @@ -81,6 +88,7 @@ public function __construct(
$this->metadataPool = $metadataPool;
$this->resourceConnection = $resourceConnection;
$this->publisher = $publisher;
$this->userContext = $userContext;
$this->logger = $logger;
}

Expand All @@ -93,13 +101,18 @@ public function scheduleBulk($bulkUuid, array $operations, $description, $userId
$connection = $this->resourceConnection->getConnectionByName($metadata->getEntityConnectionName());
// save bulk summary and related operations
$connection->beginTransaction();
$userType = $this->userContext->getUserType();
if ($userType === null) {
$userType = UserContextInterface::USER_TYPE_ADMIN;
}
try {
/** @var \Magento\AsynchronousOperations\Api\Data\BulkSummaryInterface $bulkSummary */
$bulkSummary = $this->bulkSummaryFactory->create();
$this->entityManager->load($bulkSummary, $bulkUuid);
$bulkSummary->setBulkId($bulkUuid);
$bulkSummary->setDescription($description);
$bulkSummary->setUserId($userId);
$bulkSummary->setUserType($userType);
$bulkSummary->setOperationCount((int)$bulkSummary->getOperationCount() + count($operations));

$this->entityManager->save($bulkSummary);
Expand Down
16 changes: 16 additions & 0 deletions app/code/Magento/AsynchronousOperations/Model/BulkSummary.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ public function setUserId($userId)
return $this->setData(self::USER_ID, $userId);
}

/**
* @inheritDoc
*/
public function getUserType()
{
return $this->getData(self::USER_TYPE);
}

/**
* @inheritDoc
*/
public function setUserType($userType)
{
return $this->setData(self::USER_TYPE, $userType);
}

/**
* @inheritDoc
*/
Expand Down
14 changes: 13 additions & 1 deletion app/code/Magento/AsynchronousOperations/Model/MassSchedule.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Magento\Framework\Exception\BulkException;
use Psr\Log\LoggerInterface;
use Magento\AsynchronousOperations\Model\ResourceModel\Operation\OperationRepository;
use Magento\Authorization\Model\UserContextInterface;

/**
* Class MassSchedule used for adding multiple entities as Operations to Bulk Management with the status tracking
Expand Down Expand Up @@ -54,6 +55,11 @@ class MassSchedule
*/
private $operationRepository;

/**
* @var \Magento\Authorization\Model\UserContextInterface
*/
private $userContext;

/**
* Initialize dependencies.
*
Expand All @@ -70,14 +76,16 @@ public function __construct(
AsyncResponseInterfaceFactory $asyncResponseFactory,
BulkManagementInterface $bulkManagement,
LoggerInterface $logger,
OperationRepository $operationRepository
OperationRepository $operationRepository,
UserContextInterface $userContext
Copy link
Collaborator

Choose a reason for hiding this comment

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

This constructor exists in Magento Commerce 2.2 so new parameters should be added corresponding to backward compatibility policies (as optional)

) {
$this->identityService = $identityService;
$this->itemStatusInterfaceFactory = $itemStatusInterfaceFactory;
$this->asyncResponseFactory = $asyncResponseFactory;
$this->bulkManagement = $bulkManagement;
$this->logger = $logger;
$this->operationRepository = $operationRepository;
$this->userContext = $userContext;
}

/**
Expand All @@ -95,6 +103,10 @@ public function publishMass($topicName, array $entitiesArray, $groupId = null, $
{
$bulkDescription = __('Topic %1', $topicName);

if ($userId == null) {
$userId = $this->userContext->getUserId();

Choose a reason for hiding this comment

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

User ID is not unique identifier and should be used in conjunction with user type.

There could be integration, admin and customer with the same ID. I believe bulk operation could at least be scheduled by the admin or integration.

Choose a reason for hiding this comment

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

you are right the user ID is not unique.

@anvasiliev @nuzil How do you think if we save the user type?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @paliarush , @vrann
We though about 2 possible solutions, maybe we have to discuss it separately:

  1. We will add prefixes to users, like "admin_ID, customer_ID" .....
  2. We will add new column to this table with user_type.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@munkhulzii @nuzil it would be a cleaner solution if we add another column, with the default value set to the admin. Otherwise it will be backward incompatible with the rest of the system

}

if ($groupId == null) {
$groupId = $this->identityService->generateId();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public function testScheduleBulk()
$bulkUuid = 'bulk-001';
$description = 'Bulk summary description...';
$userId = 1;
$userType = \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN;
$connectionName = 'default';
$topicNames = ['topic.name.0', 'topic.name.1'];
$operation = $this->getMockBuilder(\Magento\AsynchronousOperations\Api\Data\OperationInterface::class)
Expand All @@ -131,6 +132,7 @@ public function testScheduleBulk()
$bulkSummary->expects($this->once())->method('setBulkId')->with($bulkUuid)->willReturnSelf();
$bulkSummary->expects($this->once())->method('setDescription')->with($description)->willReturnSelf();
$bulkSummary->expects($this->once())->method('setUserId')->with($userId)->willReturnSelf();
$bulkSummary->expects($this->once())->method('setUserType')->with($userType)->willReturnSelf();
$bulkSummary->expects($this->once())->method('getOperationCount')->willReturn(1);
$bulkSummary->expects($this->once())->method('setOperationCount')->with(3)->willReturnSelf();
$this->entityManager->expects($this->once())->method('save')->with($bulkSummary)->willReturn($bulkSummary);
Expand Down
1 change: 0 additions & 1 deletion app/code/Magento/AsynchronousOperations/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"magento/module-authorization": "*",
"magento/module-backend": "*",
"magento/module-ui": "*",
"magento/module-user": "*",
"php": "~7.1.3||~7.2.0"
},
"suggest": {
Expand Down
5 changes: 2 additions & 3 deletions app/code/Magento/AsynchronousOperations/etc/db_schema.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
<column xsi:type="varbinary" name="uuid" nullable="true" length="39"
comment="Bulk UUID (can be exposed to reference bulk entity)"/>
<column xsi:type="int" name="user_id" padding="10" unsigned="true" nullable="true" identity="false"
comment="ID of the user that performed an action"/>
comment="ID of the WebAPI user that performed an action"/>
<column xsi:type="int" name="user_type" nullable="true" comment="Which type of user"/>
<column xsi:type="varchar" name="description" nullable="true" length="255" comment="Bulk Description"/>
<column xsi:type="int" name="operation_count" padding="10" unsigned="true" nullable="false" identity="false"
comment="Total number of operations scheduled within this bulk"/>
Expand All @@ -23,8 +24,6 @@
<constraint xsi:type="primary" name="PRIMARY">
<column name="id"/>
</constraint>
<constraint xsi:type="foreign" name="MAGENTO_BULK_USER_ID_ADMIN_USER_USER_ID" table="magento_bulk"
column="user_id" referenceTable="admin_user" referenceColumn="user_id" onDelete="CASCADE"/>
<constraint xsi:type="unique" name="MAGENTO_BULK_UUID">
<column name="uuid"/>
</constraint>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,47 +1,47 @@
{
"magento_bulk": {
Copy link

Choose a reason for hiding this comment

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

Please keep 4 spaces intendation for whitelists, changes are very hard to read.

"column": {
"id": true,
"uuid": true,
"user_id": true,
"description": true,
"operation_count": true,
"start_time": true
},
"constraint": {
"PRIMARY": true,
"MAGENTO_BULK_USER_ID_ADMIN_USER_USER_ID": true,
"MAGENTO_BULK_UUID": true
}
"magento_bulk": {
"column": {
"id": true,
"uuid": true,
"user_id": true,
"user_type": true,
"description": true,
"operation_count": true,
"start_time": true
},
"magento_operation": {
"column": {
"id": true,
"bulk_uuid": true,
"topic_name": true,
"serialized_data": true,
"result_serialized_data": true,
"status": true,
"error_code": true,
"result_message": true
},
"index": {
"MAGENTO_OPERATION_BULK_UUID_ERROR_CODE": true
},
"constraint": {
"PRIMARY": true,
"MAGENTO_OPERATION_BULK_UUID_MAGENTO_BULK_UUID": true
}
"constraint": {
"PRIMARY": true,
"MAGENTO_BULK_UUID": true
}
},
"magento_operation": {
"column": {
"id": true,
"bulk_uuid": true,
"topic_name": true,
"serialized_data": true,
"result_serialized_data": true,
"status": true,
"error_code": true,
"result_message": true
},
"index": {
"MAGENTO_OPERATION_BULK_UUID_ERROR_CODE": true
},
"constraint": {
"PRIMARY": true,
"MAGENTO_OPERATION_BULK_UUID_MAGENTO_BULK_UUID": true
}
},
"magento_acknowledged_bulk": {
"column": {
"id": true,
"bulk_uuid": true
},
"magento_acknowledged_bulk": {
"column": {
"id": true,
"bulk_uuid": true
},
"constraint": {
"PRIMARY": true,
"MAGENTO_ACKNOWLEDGED_BULK_BULK_UUID_MAGENTO_BULK_UUID": true,
"MAGENTO_ACKNOWLEDGED_BULK_BULK_UUID": true
}
"constraint": {
"PRIMARY": true,
"MAGENTO_ACKNOWLEDGED_BULK_BULK_UUID_MAGENTO_BULK_UUID": true,
"MAGENTO_ACKNOWLEDGED_BULK_BULK_UUID": true
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,30 +18,35 @@
'not_started' => [
'uuid' => 'bulk-uuid-1',
'user_id' => 1,
'user_type' => \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN,
'description' => 'Bulk Description',
'operation_count' => 1,
],
'in_progress_success' => [
'uuid' => 'bulk-uuid-2',
'user_id' => 1,
'user_type' => \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN,
'description' => 'Bulk Description',
'operation_count' => 3,
],
'in_progress_failed' => [
'uuid' => 'bulk-uuid-3',
'user_id' => 1,
'user_type' => \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN,
'description' => 'Bulk Description',
'operation_count' => 2,
],
'finish_success' => [
'uuid' => 'bulk-uuid-4',
'user_id' => 1,
'user_type' => \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN,
'description' => 'Bulk Description',
'operation_count' => 1,
],
'finish_failed' => [
'uuid' => 'bulk-uuid-5',
'user_id' => 1,
'user_type' => \Magento\Authorization\Model\UserContextInterface::USER_TYPE_ADMIN,
'description' => 'Bulk Description',
'operation_count' => 2,
],
Expand Down Expand Up @@ -90,8 +95,8 @@
],
];

$bulkQuery = "INSERT INTO {$bulkTable} (`uuid`, `user_id`, `description`, `operation_count`)"
. " VALUES (:uuid, :user_id, :description, :operation_count);";
$bulkQuery = "INSERT INTO {$bulkTable} (`uuid`, `user_id`, `user_type`, `description`, `operation_count`)"
. " VALUES (:uuid, :user_id, :user_type, :description, :operation_count);";
foreach ($bulks as $bulk) {
$connection->query($bulkQuery, $bulk);
}
Expand Down