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

refs #10725 removing and creating url rewrites #10939

Closed
wants to merge 1 commit into from

Conversation

psiwik
Copy link

@psiwik psiwik commented Sep 17, 2017

Fix an issue #10725

Description

This problem concerns not only visibility. This bug appears when we changed product category, url_key and website. When we saving the product from admin (default store view) magento try to remove urls from product->storeId = 0. In the database there are no such entries. From the other side, when we want to change categories after save we should to remove and rebuild all urls (category is global attribute), but when admin is set to a specific store, magento rebuilds only links for this store. It's not a good idea to get storeId from product to rebuild url rewrites.

Fixed Issues (if relevant)

  1. Product URL rewrite is not removed when switching visibility #10725 after changing visibility from default store view all stores uses default settings will be refreshed (removed or rebuilded)
  2. Change category/website from specific store rebuilds product urls for all stores
  3. Changing visibility and url_key from specific store works like before

Manual testing scenarios

  1. Create website with at least two stores view.
  2. Create a product that is disabled and has visibilty set to 'not visible individually'
  3. Check the URL rewrite table. It should not have a URL rewrite (Magento creates those only for visible products).
  4. Edit the product, setting visibility to 'Category & Search'.
  5. Check the URL Rewrite table. A record should be added.
  6. Edit the product, setting visibility back to 'Not visible individually'
  7. Check the URL Rewrite table. All records should be removed.
  8. Change store view to a specific store.
  9. Disable "use default value checkbox"
  10. Change visibility to "Category & Search"
  11. Check URL Rewrite table. Should be only records from that store
  12. Change visibility to "Not visible individually"
  13. Change store to "All Store Views"
  14. Change visibility to "Category & Search".
  15. Check URL Rewrite table. Should be all records expect that disabled store
  16. Change category (only adding works - it should be new issue)
  17. Check URL Rewrite table, all entries should be changed.
  18. Change store to "disabled" store
  19. Change category.
  20. Check URL Rewrite table, all entries should be changed (like on default store)

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 17, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Przemek Siwik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@vrann vrann self-assigned this Sep 18, 2017
@vrann vrann added this to the September 2017 milestone Sep 18, 2017
@vrann vrann added the develop label Sep 18, 2017
@vrann
Copy link
Contributor

vrann commented Sep 19, 2017

@psiwik please address this issue from CLA tool (seems like you did some commits with the wrong email or something)

Przemek Siwik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@vrann
Copy link
Contributor

vrann commented Sep 26, 2017

@psiwik can you please accept CLA? We would not be able to accept the PR before you do that.

Also, please fix issues in the static tests. Other than that, the contribution passed QA and looks great.

* @return \Magento\UrlRewrite\Service\V1\Data\UrlRewrite[]
*/
public function generate(Product $product, $rootCategoryId = null)
public function generate(Product $product, $rootCategoryId = null,$storeId = null)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

please add space after comma

foreach ($stores as $storeId) {
$visible = $productResource->getAttributeRawValue($productId,'visibility',$storeId);
if (in_array($visible,$siteVisibilities)) {
$this->urlPersist->replace($this->productUrlRewriteGenerator->generate($product,null,$storeId));
Copy link
Contributor

Choose a reason for hiding this comment

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

please add spaces after commas

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@vkublytskyi
Copy link

@psiwik Please let us know if you have time to resolve static tests failures and finish this PR.

Apply Magento coding standards as described in

1) Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 2 violation(s): 
FILE: ...ento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php
--------------------------------------------------------------------------------
FOUND 11 ERRORS AFFECTING 8 LINES
--------------------------------------------------------------------------------
 53 | ERROR | [x] Whitespace found at end of line
 60 | ERROR | [x] Whitespace found at end of line
 62 | ERROR | [x] Whitespace found at end of line
 70 | ERROR | [ ] Line exceeds maximum limit of 120 characters; contains 134
    |       |     characters
 70 | ERROR | [x] Whitespace found at end of line
 77 | ERROR | [x] Whitespace found at end of line
 78 | ERROR | [x] No space found after comma in function call
 78 | ERROR | [x] No space found after comma in function call
 79 | ERROR | [x] No space found after comma in function call
 80 | ERROR | [x] No space found after comma in function call
 80 | ERROR | [x] No space found after comma in function call
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: ...lRewrite/Test/Unit/Observer/ProductProcessUrlRewriteSavingObserverTest.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 230 | ERROR | Line exceeds maximum limit of 120 characters; contains 131
     |       | characters
 231 | ERROR | Code must not contain multiple empty lines in a row; found 2
     |       | empty lines
--------------------------------------------------------------------------------
FILE: ...o2/app/code/Magento/CatalogUrlRewrite/Model/ProductUrlRewriteGenerator.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 129 | ERROR | [x] Expected 1 space between comma and argument "$storeId"; 0
     |       |     found
 131 | ERROR | [ ] is_null must be avoided. Use strict comparison instead.
 137 | ERROR | [ ] Code must not contain multiple empty lines in a row; found 2
     |       |     empty lines
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Make reafcroting of ProductProcessUrlRewriteSavingObserver:: execute and split logic by private methods to resolve

2) Magento\Test\Php\LiveCodeTest::testCodeMess
PHP Code Mess has found error(s):
/home/travis/build/magento/magento2/app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php:43	The method execute() has a Cyclomatic Complexity of 11. The configured cyclomatic complexity threshold is 10.

@okorshenko okorshenko assigned okorshenko and unassigned vrann Nov 28, 2017
@okorshenko
Copy link
Contributor

Hi @psiwik thank you for your contribution. Unfortunately we can not accept this PR without signed CLA and with broken tests.
Feel free to reopen this PR once ready

@okorshenko okorshenko closed this Nov 28, 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.

7 participants