Skip to content

Added The return of Post Value when an Integration Exception is thrown while Save Action #26660

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

Merged
merged 8 commits into from
Jul 16, 2020

Conversation

realadityayadav
Copy link
Member

@realadityayadav realadityayadav commented Feb 2, 2020

This keeps the form data persistent when the an Integration Exception is throw as a case when name of Integration being added is same.

Description (*)

Magento 2.x - This issue reproduce in all version.

Go to System-> Integrations
Add a new Integration , Choose a name that already exist and fill all the form and then hit Save.

Expected - The Save Actions Fails with a message The integration with name "%1" exists. but the form entries are intact.
Actual - The Save Actions Fails with a message The integration with name "%1" exists. but the form entries are reset.

Related Pull Requests

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

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)

Resolved issues:

  1. resolves [Issue] Added The return of Post Value when an Integration Exception is thrown while Save Action #28143: Added The return of Post Value when an Integration Exception is thrown while Save Action

This keeps the form data persistent when the an Integration Exception is throw as a case when name of Integration being added is same.
@m2-assistant
Copy link

m2-assistant bot commented Feb 2, 2020

Hi @realadityayadav. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

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

@ghost
Copy link

ghost commented Feb 2, 2020

@realadityayadav unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@realadityayadav realadityayadav changed the title Update Save.php Added The return of Post Value when an Integration Exception is thrown while Save Action Feb 2, 2020
@realadityayadav
Copy link
Member Author

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @realadityayadav. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @realadityayadav, here is your new Magento instance.
Admin access: https://pr-26660.instances.magento-community.engineering/admin_60db
Login: bd4fb776 Password: 7d8c102c5bff
Instance will be terminated in up to 3 hours.

@rogyar rogyar self-assigned this Feb 2, 2020
@rogyar
Copy link
Contributor

rogyar commented Feb 2, 2020

Hi @realadityayadav. Thank you for your collaboration.
According to the new Definition of Done all changes should be covered by automated tests. Could I kindly ask you to cover your changes with an MFTF test which will attempt to create another integration with the same name and assert that the form data persists?

Thank you!

@rogyar rogyar added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Feb 2, 2020
@realadityayadav
Copy link
Member Author

Ya why not. Could you guide me to a reference of adding an MFTF please?

@sivaschenko
Copy link
Member

sivaschenko commented Feb 24, 2020

@realadityayadav
Copy link
Member Author

@rogyar The Test are failing for a different module in the EE & B2B files which are not yet available in this release?
Can you take a look once?

@@ -46,5 +46,6 @@
<argument name="message" value="The integration with name &quot;Integration1&quot; exists."/>
<argument value="error" name="messageType"/>
</actionGroup>
<seeInField stepKey="checkEnteredValueIsPreserved" selector="{{AdminNewIntegrationSection.name}}" userInput="Integration1"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @realadityayadav. According to the best practices it's highly recommended to build a test using action groups without direct operations in the test body. As you can see from the test you have edited, it consists of the action groups.

Please, move this check into a new action group and name the action group as AssertAdminIntegrationNameInFormActionGroup. Then you can use the action group in your test.

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@rogyar
Copy link
Contributor

rogyar commented Mar 18, 2020

Hi @realadityayadav. Looks like you forgot to create the mentioned action group. According to my previous comment you need to perform the following steps.

  1. Create a new action group (AssertAdminIntegrationNameInFormActionGroup)
  2. Move seeInField check into the newly created action group
  3. Use the newly created action group in the test body.

As far as I see, steps 1-2 are missing so far.

@realadityayadav
Copy link
Member Author

I think it is already there. Please check the latest version.

@rogyar
Copy link
Contributor

rogyar commented Mar 28, 2020

Hi @realadityayadav. You have 2 files modified in your PR: the actual fix and the test. No action group so far.

@engcom-Charlie engcom-Charlie self-assigned this May 4, 2020
@engcom-Charlie engcom-Charlie requested a review from rogyar May 4, 2020 10:45
@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented May 7, 2020

✔️ QA Passed

Manual testing scenario:

  1. Go to Admin->System->Integrations;
  2. Add a new Integration;
  3. Choose a name that already exists and fill all the form ;
  4. Save;

Before: ✖️ The form entries are reset.

Peek 2020-05-07 09-46

After: ✔️ The form entries are intact.

Peek 2020-05-07 09-43

@slavvka
Copy link
Member

slavvka commented May 7, 2020

@magento create issue

@slavvka slavvka added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels May 7, 2020
@slavvka slavvka added this to the 2.4.1 milestone May 7, 2020
@VladimirZaets VladimirZaets added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label May 19, 2020
@engcom-Delta engcom-Delta added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Jun 30, 2020
@magento-engcom-team magento-engcom-team added Partner: Cedcommerce partners-contribution Pull Request is created by Magento Partner labels Jun 30, 2020
@slavvka
Copy link
Member

slavvka commented Jul 7, 2020

@magento run WebAPI tests

@slavvka
Copy link
Member

slavvka commented Jul 7, 2020

@magento run WebAPI Tests

@slavvka
Copy link
Member

slavvka commented Jul 13, 2020

@magento run Integration Tests

magento-engcom-team pushed a commit that referenced this pull request Jul 16, 2020
@magento-engcom-team magento-engcom-team merged commit 7cabedf into magento:2.4-develop Jul 16, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jul 16, 2020

Hi @realadityayadav, 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
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: MFTF test coverage Community Insider: Cedcommerce community-insider-contribution Component: Integration Event: mm20in Partner: Cedcommerce partners-contribution Pull Request is created by Magento Partner Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Issue] Added The return of Post Value when an Integration Exception is thrown while Save Action
9 participants