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

#8810 - REST API - Attribute option creation -> no ID returned #12920

Merged
merged 7 commits into from
Jul 3, 2018
Merged

#8810 - REST API - Attribute option creation -> no ID returned #12920

merged 7 commits into from
Jul 3, 2018

Conversation

sanjay-wagento
Copy link
Contributor

@sanjay-wagento sanjay-wagento commented Dec 29, 2017

Set option_id in value field to attributeoptioninterface return type

Description

I have changed return type of add function and set value to option. So when we do REST or SOAP api then it will return option_id in 'value' field to attributeoptioninterface return type.

Fixed Issues (if relevant)

  1. REST API - Attribute option creation -> no ID returned #8810: REST API - Attribute option creation -> no ID returned

Manual testing scenarios

  1. Fresh install
  2. Make following REST request:
    POST /rest/default/V1/products/attributes/manufacturer/options
    with following Body:
    { "option": { "label": "Manufacturer 1" } }

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-engcom-team magento-engcom-team added bugfix 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 SQUASHTOBERFEST Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog labels Dec 29, 2017
@mzeis mzeis self-assigned this Dec 29, 2017
@mzeis mzeis added this to the December 2017 milestone Dec 29, 2017
@mzeis
Copy link
Contributor

mzeis commented Jan 7, 2018

Hi @sanjay-wagento,

a few functional tests are failing and have to be adjusted because the result is not true anymore but a data array:

It would be awesome if you are able to fix this. If not, please let us know.

Thanks!

@okorshenko okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018
@mzeis
Copy link
Contributor

mzeis commented Jan 28, 2018

@sanjay-wagento Short update: results of the performance test suite indicate this change might cause a performance degradation. We are investigating at the moment.

@okorshenko okorshenko modified the milestones: January 2018, February 2018 Feb 7, 2018
@okorshenko okorshenko modified the milestones: February 2018, March 2018 Mar 1, 2018
@@ -29,7 +29,7 @@ public function getItems($attributeCode);
* @param \Magento\Eav\Api\Data\AttributeOptionInterface $option
* @throws \Magento\Framework\Exception\StateException
* @throws \Magento\Framework\Exception\InputException
* @return bool
* @return \Magento\Eav\Api\Data\AttributeOptionInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

please return the option ID instead of object

@@ -20,7 +20,7 @@
* @param \Magento\Eav\Api\Data\AttributeOptionInterface $option
* @throws \Magento\Framework\Exception\StateException
* @throws \Magento\Framework\Exception\InputException
* @return bool
* @return \Magento\Eav\Api\Data\AttributeOptionInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

please return the option ID instead of object

@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018
Stanislav Idolov added 2 commits April 25, 2018 15:51
@sidolov sidolov changed the base branch from 2.3-develop to 2.2-develop April 25, 2018 14:31
@sidolov sidolov changed the base branch from 2.2-develop to 2.3-develop April 25, 2018 14:31
@magento-engcom-team magento-engcom-team added Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner labels Apr 25, 2018
@magento magento deleted a comment from magento-cicd2 Apr 26, 2018
@okorshenko okorshenko modified the milestones: April 2018, May 2018 May 18, 2018
@okorshenko okorshenko removed this from the May 2018 milestone May 31, 2018
@magento-engcom-team magento-engcom-team merged commit 5eef1d9 into magento:2.3-develop Jul 3, 2018
@magento-engcom-team
Copy link
Contributor

Hi @sanjay-wagento. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

@hostep
Copy link
Contributor

hostep commented Sep 25, 2018

Hi all involved

Something seems to have gone wrong here.
This was discovered totally by accident btw.

In Magento 2.2-develop, when you create a new attribute option using the REST API, it returns either:

  • true
  • false

In Magento 2.3-develop, when you create a new attribute option using the REST API, it returns either:

  • "new_option"
  • false

The purpose of this PR (correct me if I'm wrong) was to return either:

  • {"label":"some label","value":"123"}
  • false

Reading @okorshenko remarks, I think the following needed to be returned (but might be wrong about that):

  • 123
  • false

After git bisecting, it looks like 5eef1d9 caused the problem.

So what was ultimately decided what needed to be returned? I assume "new_option" isn't really correct?

Would be great if either @sanjay-wagento or @sidolov could review and make sure that the correct thing happens here.

If I need to turn this into a new issue, please let me know.

Thanks!

@milindsingh
Copy link
Member

Hi all involved

Something seems to have gone wrong here.
This was discovered totally by accident btw.

In Magento 2.2-develop, when you create a new attribute option using the REST API, it returns either:

  • true
  • false

In Magento 2.3-develop, when you create a new attribute option using the REST API, it returns either:

  • "new_option"
  • false

The purpose of this PR (correct me if I'm wrong) was to return either:

  • {"label":"some label","value":"123"}
  • false

Reading @okorshenko remarks, I think the following needed to be returned (but might be wrong about that):

  • 123
  • false

After git bisecting, it looks like 5eef1d9 caused the problem.

So what was ultimately decided what needed to be returned? I assume "new_option" isn't really correct?

Would be great if either @sanjay-wagento or @sidolov could review and make sure that the correct thing happens here.

If I need to turn this into a new issue, please let me know.

Thanks!

@hostep You are correct as original discussion was not to just return the id but the complete option data.
But as suggested by @mbrinton01 in #6300 (comment) I have backported this PR.

I still believe that it should return the complete option data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3 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.

8 participants