-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579
base: 2.5-develop
Are you sure you want to change the base?
Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579
Changes from 4 commits
1829a71
28ca7a4
a25bc9e
ac37fdc
8b6bba9
8f02ec5
27ecac0
52a3160
53b75d6
256ea57
8c39c05
49614f4
e90bfa5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,23 @@ public function setErrorCode($errorCode) | |
return $this->setData(self::ERROR_CODE, $errorCode); | ||
} | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
public function getOperationKey() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing return type on prototype |
||
{ | ||
return $this->getData(self::OPERATION_KEY); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK when data returning from DB - it might be a string. Could you convert it to int here? |
||
} | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
public function setOperationKey(int $operationKey) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing return type on prototype |
||
{ | ||
$this->setData(self::OPERATION_KEY, $operationKey); | ||
return $this; | ||
} | ||
|
||
/** | ||
* Retrieve existing extension attributes object. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,7 @@ public function createByTopic($topicName, $entityParams, $groupId, $operationId) | |
]; | ||
$data = [ | ||
'data' => [ | ||
OperationInterface::ID => $operationId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't forget this class exists as well in the webapi async module, and needs to be changed there as well. Magento/WebApiAsync/Model/OperationRepository.php:92 |
||
OperationInterface::OPERATION_KEY => $operationId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should not work correctly. if you switch OPERATION_KEY to "id", it will stop working, cause $operationId here is a key of array, and in database "id" is an auto_increment value. And in case of multiple operations, will be duplicated for different bulk operations. Implementing of OPERATION_KEY was done in purpose, for decouple operation keys from database increments and improve performance on saving big operations. Another issue, it that not all Magento API functionality have a tests coverage, so we overslept broken another part of API |
||
OperationInterface::BULK_ID => $groupId, | ||
OperationInterface::TOPIC_NAME => $topicName, | ||
OperationInterface::SERIALIZED_DATA => $this->jsonSerializer->serialize($serializedData), | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -61,7 +61,7 @@ public function testGetBulkStatus() | |||||
} | ||||||
/** @var OperationInterface $operation */ | ||||||
$operation = array_shift($operations); | ||||||
$operationId = $operation->getId(); | ||||||
$operationId = $operation->getOperationKey(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
$this->assertTrue($this->model->changeOperationStatus( | ||||||
'bulk-uuid-5', | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not oberation ids anymore. Pls rename it in all places