Skip to content

magento/magento2#27337: GraphQl. Add a mutation for subscribe feature #27586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 5, 2020
Merged

magento/magento2#27337: GraphQl. Add a mutation for subscribe feature #27586

merged 7 commits into from
Jun 5, 2020

Conversation

atwixfirster
Copy link
Contributor

@atwixfirster atwixfirster commented Apr 4, 2020 •
edited by lenaorobei
Loading

Description (*)

PR fixes #27337.

This PR was mentioned for fixing failed static test related with composer.json file
#258

Related Pull Requests

https://github.com/magento/partners-magento2ee/pull/258

Fixed Issues (if relevant)

  1. Fixes GraphQl. Add a mutation for subscribe feature #27337: GraphQl. Add a mutation for subscribe feature

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 4, 2020

Hi @atwixfirster. 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.

@atwixfirster atwixfirster mentioned this pull request Apr 4, 2020
Merged
@rogyar rogyar self-assigned this Apr 4, 2020
@rogyar rogyar self-requested a review April 4, 2020 17:15
rogyar
rogyar requested changes Apr 4, 2020
View reviewed changes
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @atwixfirster. Thank you for the great PR. Please, check my comments below for minor changes

app/code/Magento/NewsletterGraphQl/Model/Resolver/SubscribeEmailToNewsletter.php Outdated
use Magento\Framework\GraphQl\Query\Resolver\ContextInterface;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
use Magento\Framework\Validator\EmailAddress as EmailValidator;
use Magento\Framework\App\Config\ScopeConfigInterface;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, sort imports alphabetically (there's "Optimize imports" feature in PHPStorm, I recommend to enable the automated sorting by default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

I've missed that...

...-functional/testsuite/Magento/GraphQl/Newsletter/Customer/SubscribeEmailToNewsletterTest.php Outdated
*/
declare(strict_types=1);

namespace Magento\GraphQl\Newsletter\Guest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, correct the namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

fixed

...-functional/testsuite/Magento/GraphQl/Newsletter/Customer/SubscribeEmailToNewsletterTest.php Outdated
private function getHeaderMap(string $username = '[email protected]', string $password = 'password'): array
{
$customerToken = $this->customerTokenService->createCustomerAccessToken($username, $password);
$headerMap = ['Authorization' => 'Bearer ' . $customerToken];
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the temp $headerMap variable here. You may return the array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep!

THank you, @rogyar

@rogyar rogyar added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Apr 4, 2020
@atwixfirster atwixfirster force-pushed the graphql-mutation-for-subscription branch 4 times, most recently from e974f6a to 6b7a81f Compare April 4, 2020 18:50
@rogyar
Copy link
Contributor

rogyar commented Apr 8, 2020

@magento run all tests

@atwixfirster atwixfirster force-pushed the graphql-mutation-for-subscription branch from 6b7a81f to 319aaba Compare April 9, 2020 00:19
@atwixfirster atwixfirster force-pushed the graphql-mutation-for-subscription branch 3 times, most recently from 4717ec7 to 5f1330b Compare April 22, 2020 19:21
keharper
keharper reviewed Apr 23, 2020
View reviewed changes
Copy link
Contributor

@keharper keharper left a comment

Choose a reason for hiding this comment

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

All comments are suggestions.

@ghost ghost assigned keharper Apr 23, 2020
@rogyar
Copy link
Contributor

rogyar commented Apr 26, 2020

Now it is the most favorite part. To deal with failing tests. I believe we may decouple the resolver slightly in order to decrease the number of dependencies.

@engcom-Kilo engcom-Kilo self-assigned this Jun 1, 2020
@engcom-Kilo
Copy link
Contributor

@magento run Magento Health Index

@engcom-Kilo
Copy link
Contributor

@magento run Magento Health Index

@magento-engcom-team
Copy link
Contributor

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

@slavvka slavvka mentioned this pull request Jun 5, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Jun 5, 2020
@lenaorobei
ENGCOM-7599: #27337: GraphQl. Add a mutation for subscribe feature #2…
b5e1dda 
…7586
@magento-engcom-team magento-engcom-team merged commit 5927ec8 into magento:2.4-develop Jun 5, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jun 5, 2020

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

@atwixfirster atwixfirster mentioned this pull request Jun 7, 2020
Closed
4 tasks
@atwixfirster atwixfirster mentioned this pull request Jun 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keharper keharper keharper left review comments

@rogyar rogyar rogyar approved these changes

@lenaorobei lenaorobei lenaorobei approved these changes

Assignees

@rogyar rogyar

@keharper keharper

@lenaorobei lenaorobei

@engcom-Bravo engcom-Bravo

@engcom-Kilo engcom-Kilo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: category of expertise Award: complex Award: test coverage Component: Newsletter Component: NewsletterGraphQl Event: Global-Contribution-Day Partner: Atwix Pull Request is created by partner Atwix 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 Project: GraphQL 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
Pull Requests Dashboard
Archived in project
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

GraphQl. Add a mutation for subscribe feature