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

[Backport] REST API - Attribute option creation -> no ID returned #20201

Closed
wants to merge 5 commits into from
Closed

[Backport] REST API - Attribute option creation -> no ID returned #20201

wants to merge 5 commits into from

Conversation

milindsingh
Copy link
Member

@milindsingh milindsingh commented Jan 11, 2019

Original Pull Request

#12920

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
  2. Successful attribute option POST returns 'true', not the option hash or value #6300: Successful attribute option POST returns 'true', not the option hash or value

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
Copy link
Contributor

Hi @milindsingh. 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.2-develop instance - deploy vanilla Magento instance

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

@hostep
Copy link
Contributor

hostep commented Jan 11, 2019

Isn't this considered a backwards incompatible change? The API response changes in a patch version, existing API implementations might not expect this to happen.
I might be wrong since I don't know if Magento has a policy for this?

And won't this PR cause the API to return "id_new_option" instead of the actual ID? (see #18392 & #19108)

@milindsingh
Copy link
Member Author

@hostep Kindly check #6300 (comment)

@hostep
Copy link
Contributor

hostep commented Jan 12, 2019

Thanks @milindsingh, I don't really agree with @mbrinton01, so let's include him in here.

@mbrinton01: can you check out #18392 (full history can be found in #12920) and also my question above regarding backwards compatibility? Thanks!

@milindsingh
Copy link
Member Author

@hostep Can you please confirm about 2 things ?

  1. The option create should return the complete data rather than just the id ?
  2. API behavior changes should not be ported?

@hostep
Copy link
Contributor

hostep commented Jan 12, 2019

@milindsingh: I'm not a Magento employee, so I can't confirm any of these things, sorry! :)

Might be best to wait until someone from the maintainers or Magento has replied here to see where this needs to go.

@Nazar65
Copy link
Member

Nazar65 commented Jan 17, 2019

@milindsingh did you check my PR ? #18392
returned type must be option_id and this already done by my commit.

@mbrinton01
Copy link

Adding @paliarush for help on backwards compatibility API policy. Alex, do we have strict guidelines on backwards compatibility for API responses?

@aleron75 aleron75 self-assigned this Mar 6, 2019
@magento-engcom-team
Copy link
Contributor

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

@hostep
Copy link
Contributor

hostep commented Mar 6, 2019

@aleron75: why is this approved, it makes no sense? Have you read through #12920 (comment) or #18392 ?

I think we need to wait until #19108 is approved before any of this can be backported to 2.2 (if that is even allowed by backwards compatible rules for API responses, which we don't know about yet...)

@sivaschenko
Copy link
Member

Hi @milindsingh @aleron75 unfortunately we cannot allow backporting such API changes to 2.2.

I have to close this pull request according to above. Feel free to contanct me in case of any comments or questions

See Code Contributions Guide for more information.

@ghost
Copy link

ghost commented Mar 12, 2019

Hi @milindsingh, 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.

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.

9 participants