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

Fixed issue #20828 Newsletter Problem Grid Filter issue #20829

Merged
merged 3 commits into from
Feb 12, 2019
Merged

Fixed issue #20828 Newsletter Problem Grid Filter issue #20829

merged 3 commits into from
Feb 12, 2019

Conversation

GovindaSharma
Copy link
Contributor

@GovindaSharma GovindaSharma commented Jan 31, 2019

Description (*)

Fixed issue #20828 Newsletter Problem Grid Filter issue

Fixed Issues (if relevant)

  1. magento/magento2 Newsletter Problem Grid Filter issue #20828 Newsletter Problem Grid Filter issue

Manual testing scenarios (*)

1.Go To Magento Backend
2.Navigate to Reports->Newsletter Problem Reports
3.Grid Will Open
4.Click on Search or Reset Filter Button

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 @GovindaSharma. 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.3-develop instance - deploy vanilla Magento instance

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

use Magento\Framework\App\Action\HttpGetActionInterface as HttpGetActionInterface;

class Index extends \Magento\Newsletter\Controller\Adminhtml\Problem implements HttpGetActionInterface
class Index extends \Magento\Newsletter\Controller\Adminhtml\Problem implements HttpGetActionInterface, HttpPostActionInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @GovindaSharma. Thanks for collaboration. Can you please explain how your changes related to the bug in the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @VladimirZaets ,as when grid is loading then it is get request but when you click on Search or Rest Filter button it becomes post request,but in controller only HttpGetActionInterface is implemented because of this filters not working,so i added the HttpPostActionInterface ,so whenever filters is called it is handled by HttpPostActionInterface.

You can check in Newsletter queue Grid,there it is working fine because in the controller(app/code/Magento/Newsletter/Controller/Adminhtml/Queue/Index.php) both HttpPostActionInterface and HttpGetActionInterface is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GovindaSharma Hi, I understand the case. So, the code that currently exists in Magento will be refactored (or you can do it also in another PR if you want).

The right way to resolve this issue is creating additional POST controller and use it for filters operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VladimirZaets ok then i will change accordingly,since controller is already created we just need to pass that controller url.I will update you on this.
Thank You

@GovindaSharma
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @GovindaSharma, here is your new Magento instance.
Admin access: https://pr-20829.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@GovindaSharma
Copy link
Contributor Author

@VladimirZaets i have updated the changes according to your request.Reverted the previous changes and just added the grid url in xml,now all filters request will directly go to grid url. Please check and update on this.

@magento-engcom-team
Copy link
Contributor

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

@ghost
Copy link

ghost commented Feb 12, 2019

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

3 participants