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

Add possibility to include multiple non primitive types in an array #433

Merged
merged 5 commits into from
Oct 21, 2019

Conversation

ochnygosch
Copy link
Contributor

@ochnygosch ochnygosch commented Aug 28, 2019

Description

If an array is defined for an entity it is currently not possible to
include different types of non primitive types in the same array.

E.g. it is not possible to add a product, whose "custom_attributes" field
should contain attributes that have a primitive type (e.g. url_key) and an
array type (e.g. category_ids)

This PR adds the ability to define multiple non primitive types for an array
type.

The definition of the CreateProduct request in
magento-catalog/Test/Mftf/Metadata/product-meta.xml could be changed to

<array key="custom_attributes">
  <value>custom_attribute_array</value>
  <value>custom_attribute</value>
</array>

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/verification tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
  • Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

If an array is defined for an entity it is currently not possible to
include different types of non primitive types in the same array.
@magento-cicd2
Copy link

magento-cicd2 commented Aug 28, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Aug 28, 2019

Coverage Status

Coverage increased (+1.4%) to 53.639% when pulling 491802c on ochnygosch:fDiverseArray into 3c7ed4b on magento:develop.

@KevinBKozan
Copy link
Contributor

Hey @ochnygosch, thank you for your submission! We created a ticket internally for reviewing your PR, and will be updating you with feedback and progress on this PR.

@tomreece tomreece self-assigned this Sep 23, 2019
@tomreece
Copy link
Contributor

Hi @ochnygosch! This change looks good to me after some manual testing.
But will you please add a unit test for this change?
See OperationDataArrayResolverTest.php for examples.

@tomreece
Copy link
Contributor

tomreece commented Oct 2, 2019

Hi @ochnygosch, thank you very much for adding unit tests to your change.

We have one last thing to resolve,
The Travis builds are failing due to a Cyclomatic Complexity error introduced in the function that you modified resolveOperationDataArray().

Do you see this error in the Travis results?

Can you fix this Cyclomatic Complexity error without simply suppressing it? Give it a shot and if you can't resolve it then we will discuss whether to allow a suppression of it or not.

Thank you for your time and contribution to MFTF.

@tomreece
Copy link
Contributor

tomreece commented Oct 9, 2019

Hi @ochnygosch -- Just checking in,

You don't have to fix the cyclomatic complexity warning. I will do this for you.

But we do need you to sign the Adobe CLA. Thanks!

tomreece added a commit that referenced this pull request Oct 18, 2019
@tomreece
Copy link
Contributor

tomreece commented Oct 18, 2019

@KevinBKozan After code freeze, merge PR #490
instead of
#433

KevinBKozan added a commit that referenced this pull request Oct 21, 2019
@KevinBKozan KevinBKozan merged commit 491802c into magento:develop Oct 21, 2019
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.

5 participants