Skip to content

Conversation

arnoudhgz
Copy link
Contributor

In some cases the 'removed' tag on the image is already set to an empty
string in the data which will be loaded. Therefore the isset check fails
which clears the image roles.

On all other cases where this 'removed' tag is being validated with an
empty or !empty. Therefor it seems safe to apply this here also.

Manual testing scenarios (*)

  1. Upload an image for a product
  2. Set the image roles
  3. See that the image roles still work
  4. Try step 2 and 3 with different combination of image roles.

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented May 16, 2019

Hi @arnoudhgz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

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.

@arnoudhgz did you manage to figure out stable steps to reproduce?

@ghost ghost assigned orlangur May 16, 2019
@arnoudhgz
Copy link
Contributor Author

arnoudhgz commented May 16, 2019

@orlangur No I could not, but every other check with removed is done with empty. Changing this worked in a Vanilla installation and the installation where it was broken.

It seems similar to this issue what is hard to reproduce: #22863

In some cases the 'removed' tag on the image is already set to an empty
string in the data which will be loaded. Therefore the isset check fails
which clears the image roles.

On all other cases where this 'removed' tag is being validated with an
empty or !empty. Therefor it seems safe to apply this here also.
@arnoudhgz arnoudhgz force-pushed the feature/improve-image-roles-saving branch from 4bbeccd to b65c12e Compare June 1, 2019 09:44
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5235 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Jun 26, 2019

Hi @arnoudhgz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sidolov sidolov added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Catalog Partner: MediaCT Pull Request is created by partner MediaCT partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants