Skip to content
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

Msi/source management ui #28

Merged
merged 557 commits into from
Jun 19, 2017
Merged

Msi/source management ui #28

merged 557 commits into from
Jun 19, 2017

Conversation

larsroettig
Copy link
Member

Description

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title
  2. ...

Manual testing scenarios

  1. ...
  2. ...

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 on Travis CI are green)

</item>
</argument>
<field name="source_id">
<argument name="data" xsi:type="array">

Choose a reason for hiding this comment

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

Use also settings, instead of data -> config.

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

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

  • Term Source should be used instead of Warehouse
  • UI Component declarations should stick to the up-to-date format (please see integration test failures)

/**
* Class AbstractButton
*/
abstract class AbstractButton
Copy link
Contributor

Choose a reason for hiding this comment

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

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid using protected methods

*
* @return null | int
*/
protected function getSourceId()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid using protected methods

try {
$sourceId = $this->context->getRequest()->getParam('id');
return $this->sourceRepository->getById($sourceId)->getId();
} catch (NoSuchEntityException $exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Exception must be processed

/**
* @return string
*/
private function getDeleteUrl()
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magento does not encourage inheritance-based API. This method should be private.

/**
* @var \Magento\Framework\View\Result\PageFactory
*/
protected $resultPageFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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.

@ishakhsuvarov ishakhsuvarov changed the base branch from mni/source-management to develop June 7, 2017 12:41
slamking and others added 26 commits June 8, 2017 13:07
# Conflicts:
#	dev/tests/functional/tests/app/Magento/CatalogInventoryConfigurableProduct/Test/TestCase/CreateConfigurableProductEntityTest.xml
Fixed issues:
- MAGETWO-64249 Issue with integration test Magento/Setup/Console/Command/I18nCollectPhrasesCommandTest.php on Travis CI
- MAGETWO-62271 Inconsistent saving of Stock Data
…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
Oleksii Korshenko and others added 21 commits June 15, 2017 19:33
Fixed issue:
- MAGETWO-69515: Static files are deployed too slow for multiple locales
MAGETWO-67431 Functional test \Magento\ConfigurableProduct\Test\TestCase\CreateConfigurableProductEntityTest fails randomly
-- add carriers links assignment
-- add carriers links assignment
-- add carriers links assignment
@naydav naydav changed the title Mni/source management ui Msi/source management ui Jun 19, 2017
@naydav
Copy link

naydav commented Jun 19, 2017

Internal build
https://bamboo.corp.magento.com/browse/M2-AT2096-13
Acceptance Tests plan is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.