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

Fix unable to remove parameter override for a webapi route #33844

Merged
merged 5 commits into from
Oct 21, 2021
Merged

Fix unable to remove parameter override for a webapi route #33844

merged 5 commits into from
Oct 21, 2021

Conversation

gowrizrh
Copy link
Contributor

Description (*)

This PR contains the changes to fix #33843 - the force attribute in a webapi.xml is ignored in certain cases.

Fixed Issues (if relevant)

  1. Fixes Unable to remove parameter override for a webapi route #33843

Manual testing scenarios (*)

  1. Redefine a route parameter override in a new webapi.xml
  2. Verify the webapi config with something like the n98 dev:console

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Aug 18, 2021

Hi @gowrizrh. Thank you for your contribution
Here are 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@magento-engcom-team magento-engcom-team added Component: Webapi Use with concrete module component label E.g. "Component: Webapi" + "Catalog" Release Line: 2.4 labels Aug 18, 2021
@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Aug 18, 2021
@magento-engcom-team magento-engcom-team added Partner: Aligent Consulting partners-contribution Pull Request is created by Magento Partner labels Aug 18, 2021
@gowrizrh gowrizrh closed this Aug 18, 2021
@m2-assistant
Copy link

m2-assistant bot commented Aug 18, 2021

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

@gowrizrh
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@gowrizrh
Copy link
Contributor Author

@magento run Unit Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@gowrizrh
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

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

@gowrizrh Thanks for contribution.
Please look at suggested changes

app/code/Magento/Webapi/Model/Config/Converter.php Outdated Show resolved Hide resolved
Co-authored-by: Denis Kopylov <dkopylov@magenius.team>
@Den4ik
Copy link
Contributor

Den4ik commented Aug 19, 2021

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@gowrizrh
Copy link
Contributor Author

it looks like the phpcs change wasn't pushed to GH in my last comment. however i have no idea what the Functional EE test failure is.

i'll be just rerunning all tests.

@gowrizrh
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@gowrizrh
Copy link
Contributor Author

gowrizrh commented Oct 1, 2021

@engcom-Charlie

this time it says it has failed Functional Tests EE however i cannot see any errors in the build report. these automated builds are not very stable at all.

image

@engcom-Charlie
Copy link
Contributor

engcom-Charlie commented Oct 1, 2021

Hi @gowrizrh,

Yeah, Perhaps there is some issue for now.

Functional Tests EE are showing failed but when I am checking the detail report, there is no single test failure. All the Functional EE tests are successfully passed.

All the rest of the tests are also green in build so moving this issue further.

image

Thank you!

@engcom-Alfa
Copy link
Contributor

@magento run Functional Tests EE

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Preconditions:

  1. Should have Magento 2.4-develop installed.
  2. Install n98-magerun2

Manual testing scenario:

  1. Create a new simple custom module in the Magento.

  2. Create a new simple webapi.xml along with its relevant simple interfaces and classes under same created custom module.

  3. Make sure your module and API working fine.

  4. Update the webapi.xml with the <route method="POST" ; if it is already not a POST method; Since it is new one, it will having you own URI, and that should be fine. Update the data in the same file as like below
    <data> <parameter name="customer.extension_attributes.company_attributes.company_id" force="false">%company_id%</parameter> </data>
    Note: Here, the force is set to false (It should be set to false only!)

  5. Using n98-magerun2 fire up the dev console using command php bin/n98-magerun2.phar dev:console or bin/n98-magerun2 dev:console (It opens the SHELL script console to run further commands)

  6. Run the command 1: $config = $di->create(\Magento\Webapi\Model\ConfigInterface::class);

  7. Run the command 2: $customer_route_config = $config->getServices()['routes']['<API URI>']
    API URI ex is: /V1/customers ; It will be as same as defined in the webapi.xml file

  8. Check the response of the second command.

Before: ✖️ Was unable to remove the paramater override for webapi route and was getting response always "force" => false

image

After: ✔️ Able to remove the paramater override for webapi route and getting response according to the set force either to false/true

image

Tested by setting the force to false and also for true; We are getting the response accordingly. There is no other additional testing is required since it is related to API response specific to a case and functionally all works fine. Hence no additional regression is required too!

@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented Oct 8, 2021

As per the last comment - QA Passed; Also the Functional tests are getting passed as per the above - comment.
Hence moving it to merge in progress.

@m2-assistant
Copy link

m2-assistant bot commented Oct 21, 2021

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

@ihor-sviziev ihor-sviziev moved this from Merge in Progress to Recently Merged in High Priority Pull Requests Dashboard Oct 22, 2021
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 Award: bug fix Component: Webapi Use with concrete module component label E.g. "Component: Webapi" + "Catalog" Partner: Aligent Consulting partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept Release Line: 2.4 Risk: low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to remove parameter override for a webapi route
6 participants