Conversation
| </item> | ||
| </argument> | ||
| <field name="source_id"> | ||
| <argument name="data" xsi:type="array"> |
There was a problem hiding this comment.
Use also settings, instead of data -> config.
ishakhsuvarov
suggested changes
Jun 2, 2017
Contributor
ishakhsuvarov
left a comment
There was a problem hiding this comment.
- Term
Sourceshould be used instead ofWarehouse - UI Component declarations should stick to the up-to-date format (please see integration test failures)
| /** | ||
| * Class AbstractButton | ||
| */ | ||
| abstract class AbstractButton |
Contributor
There was a problem hiding this comment.
Try to avoid usage of abstract classes in such cases.
We do not encourage usage and introduction of the new inheritance based APIs
| * | ||
| * @return bool | ||
| */ | ||
| protected function isEditMode() |
Contributor
There was a problem hiding this comment.
Please avoid using protected methods
| * | ||
| * @return null | int | ||
| */ | ||
| protected function getSourceId() |
Contributor
There was a problem hiding this comment.
Please avoid using protected methods
| try { | ||
| $sourceId = $this->context->getRequest()->getParam('id'); | ||
| return $this->sourceRepository->getById($sourceId)->getId(); | ||
| } catch (NoSuchEntityException $exception) { |
Contributor
There was a problem hiding this comment.
Exception must be processed
| /** | ||
| * @return string | ||
| */ | ||
| private function getDeleteUrl() |
Contributor
There was a problem hiding this comment.
This method is not really needed, as it is used only once. You may inline this code in this case.
| /** | ||
| * @var \Magento\Backend\Model\View\Result\ForwardFactory | ||
| */ | ||
| protected $resultForwardFactory; |
Contributor
There was a problem hiding this comment.
Magento does not encourage inheritance-based API. This method should be private.
| /** | ||
| * @var \Magento\Framework\View\Result\PageFactory | ||
| */ | ||
| protected $resultPageFactory; |
Contributor
There was a problem hiding this comment.
Magento does not encourage inheritance-based API. This property should be private.
| public function getButtonData() | ||
| { | ||
| $data = []; | ||
| // This part should be refactored. Implemented for test purposes. |
Contributor
There was a problem hiding this comment.
- There is no reason to get Source by Id from SourceRepository, since delete controller will validate it anyways.
- We should research building delete url on frontend, to avoid getting Request in block, which is very bad.
…d for shipping origin
# Conflicts: # dev/tests/functional/tests/app/Magento/CatalogInventoryConfigurableProduct/Test/TestCase/CreateConfigurableProductEntityTest.xml
-- fix static tests
…ement-ui # Conflicts: # composer.lock
Fixed issues: - MAGETWO-64249 Issue with integration test Magento/Setup/Console/Command/I18nCollectPhrasesCommandTest.php on Travis CI - MAGETWO-62271 Inconsistent saving of Stock Data
…ribute to a product
-- index action
…AGETWO-69690-PR-9690
…d for shipping origin
…cts with fixed price
…d for shipping origin
…ess on Travis #9872 - Merge Pull Request magento/magento2#9872 from davidalger/magento2:feature/fix-travis-tests
Fixed an issue: - MAGETWO-62227 Unable to sort attributes table when trying to Add Attribute to a product
…-upload-Favicon-Icon' into Okapis-develop-pr
…s' into Okapis-develop-pr
Public Pull Requests magento/magento2#9943 magento/magento2#9941
Fixed issue: - MAGETWO-69515: Static files are deployed too slow for multiple locales
MAGETWO-67431 Functional test \Magento\ConfigurableProduct\Test\TestCase\CreateConfigurableProductEntityTest fails randomly
… into Okapis-develop-pr
…Stock' into Okapis-develop-pr
…heck' into Okapis-develop-pr
…Okapis-develop-pr
[Okapis] P1 P2
-- add carriers links assignment
-- add carriers links assignment
-- add carriers links assignment
-- update composer.lock
|
Internal build |
This was referenced Dec 11, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist