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

Changed Validation to get store for block #11601

Closed
wants to merge 1 commit into from

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Oct 20, 2017

Changed validation to getIsUniqueBlockToStores

Description

Changed validation to get DEFAULT_STORE_ID when the app is setted with SingleStoreMode in the other cases get store from object

Fixed Issues (if relevant)

  1. Inconsistent cms block validation and save methods #4831: Inconsistent cms block validation and save methods
  2. ...

Manual testing scenarios

  1. Make sure that you have only one store view and SingleStoreMode option to no
  2. Create cms block example for All Store views
  3. Create another cms block example for first store view only

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)

@dmanners
Copy link
Contributor

So as far I a see this fixes the issue raised but there is another issue with the block validation. In my testing I setup a multi store system so there are two store views for a website. I then added 2 blocks with the id example for store 1 and store 2. I then went to add a 3rd block to store 2 with the id example and the system lets me do this. My thinking is that the validation should check to make sure you are not adding the same block twice to the same store.

blocks

@@ -183,7 +183,7 @@ public function getIsUniqueBlockToStores(AbstractModel $object)
$entityMetadata = $this->metadataPool->getMetadata(BlockInterface::class);
$linkField = $entityMetadata->getLinkField();

if ($this->_storeManager->hasSingleStore()) {
if ($this->_storeManager->isSingleStoreMode()) {
$stores = [Store::DEFAULT_STORE_ID];
} else {
$stores = (array)$object->getData('stores');
Copy link
Contributor

Choose a reason for hiding this comment

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

The store validation seems to be broken here as the data set on the object is under store_id and not stores.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe better still would be to use the getStores method that is against the Block model.

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

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

Store validation seems to not work anyway.

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

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

The validation issue I raised has been solved in #11802

@osrecio
Copy link
Member Author

osrecio commented Oct 27, 2017

Yes is my teamate @thiagolima-bm , What que do?

@dmanners
Copy link
Contributor

@osrecio nothing to do now with this pr. We will process both of them to improve/fix the cms block admin experience. Thank you for your code.

@osrecio
Copy link
Member Author

osrecio commented Oct 30, 2017

Solved this PR in /pull/11802

@osrecio osrecio closed this Oct 30, 2017
thiagolima-bm added a commit to thiagolima-bm/magento2 that referenced this pull request Oct 30, 2017
@osrecio osrecio reopened this Oct 30, 2017
@osrecio osrecio closed this Oct 30, 2017
dmanners pushed a commit to thiagolima-bm/magento2 that referenced this pull request Nov 1, 2017
dmanners pushed a commit to thiagolima-bm/magento2 that referenced this pull request Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants