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

Attribute category_ids issue #11389

Merged
merged 1 commit into from
Nov 28, 2017
Merged

Conversation

manuelson
Copy link
Contributor

Description

Attribute category_ids issue

Fixed Issues

  1. Attribute category_ids issue #11341: Attribute category_ids issue

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)

@orlangur orlangur self-assigned this Oct 12, 2017
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

After all changes are made and all builds but functional tests are green on Travis, please squash changes into a single commit.

} elseif ($attribute->getFrontendInput() == 'price' && is_string($value)) {
$value = $this->priceCurrency->convertAndFormat($value);
}
if(!is_array($value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not increase indentation, change to

if (is_array($value)) {
    continue;
}

if(!is_array($value)) {
if (!$product->hasData($attribute->getAttributeCode())) {
$value = __('N/A');
} elseif (is_array($value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be always false condition, please remove.

if (!$product->hasData($attribute->getAttributeCode())) {
$value = __('N/A');
} elseif (is_array($value)) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

No-no, your initial idea with skipping everything in case of array was fine, continue here is a bit confusing.

Please move it upper:

if (is_array($value = $attribute->getFrontend()->getValue($product))) {
    continue;
}

and the rest will remain the same. Please squash changes into single commit when builds are green (you can ignore functional tests on Travis, they are failing sometimes due to timeouts).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

Copy link
Contributor Author

@manuelson manuelson left a comment

Choose a reason for hiding this comment

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

Fix it

if (!$product->hasData($attribute->getAttributeCode())) {
$value = __('N/A');
} elseif (is_array($value)) {
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

@orlangur
Copy link
Contributor

@manuelson thanks, changes look good to me now and builds on Travis are passing.

Could you please squash all changes into a single commit? I may assist with this if needed.

@orlangur orlangur added this to the October 2017 milestone Oct 13, 2017
@manuelson
Copy link
Contributor Author

@orlangur did You mean that I have to close this PR and open a new one with a single commit?

@orlangur orlangur added 2.2.x Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 13, 2017
@orlangur
Copy link
Contributor

No, just force push into the same branch.

Fixed issue:
MAGETWO-77777 [2.2.x] - [Magento Cloud] Using search synonyms from the same group gives different results

array bug fix

Category_ids display no

Refactoring

continue condition

Condition rewrite
@manuelson
Copy link
Contributor Author

Done

@manuelson
Copy link
Contributor Author

manuelson commented Oct 27, 2017

@orlangur @okobchenko @vkublytskyi can be SQUASHTOBERFEST? :)

@orlangur
Copy link
Contributor

@manuelson not sure who defines it, not me :(

@manuelson
Copy link
Contributor Author

Hello @orlangur for close it , you need any update?

Thanks!

@orlangur
Copy link
Contributor

orlangur commented Nov 8, 2017

Hi @manuelson, no update needed, I believe it's still waiting for testing

magento-team moved this from Review In Progress to Testing In Progress in Community Pull Requests 22 days ago

@okorshenko okorshenko merged commit c33d9d7 into magento:2.2-develop Nov 28, 2017
okorshenko pushed a commit that referenced this pull request Nov 28, 2017
magento-team pushed a commit that referenced this pull request Nov 30, 2017


 - Merge Pull Request #11807 from manuelson/magento2:feature/11341_backport
 - Merged commits:
   1. 4e24f73
magento-team pushed a commit that referenced this pull request Nov 30, 2017
Public Pull Requests

#11807 [backport 2.1] Attribute category_ids issue #11389 by @manuelson

Fixed Public Issues

#11341 Attribute category_ids issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants