Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

magento/graphql-ce:#678 Send email to friend #752

Merged
merged 10 commits into from
Jun 25, 2019

Conversation

rleshchenko
Copy link
Contributor

@rleshchenko rleshchenko commented Jun 22, 2019

Description (*)

Previously there was using comparison with Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS ENABLED instead of
comparison with Magento\Catalog\Model\Product\Visibility::VISIBILITY*. Please look at the Magento\Catalog\Model\Product::isVisibleInCatalog()

Fixed Issues (if relevant)

  1. magento/magento2#<[Test coverage] Send email to friend #678>: 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)

pmclain and others added 4 commits June 7, 2019 22:22
Resolver throws `GraphQlAuthorizationException` when sending as guest if allow
send as guest is disabled.

Fixes magento#732
* Missing dep on magento/module-authorization
* Separate email send logic from resolver
@vpodorozh vpodorozh self-assigned this Jun 22, 2019
@vpodorozh vpodorozh self-requested a review June 22, 2019 12:48
@sivaschenko sivaschenko added the Event: cd19kharkiv Contribution Day 2019 Kharkiv label Jun 22, 2019
Copy link
Contributor

@vpodorozh vpodorozh left a comment

Choose a reason for hiding this comment

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

Please check my comments and fix static tests.
Thx!

@vpodorozh
Copy link
Contributor

@rleshchenko - please move \Magento\SendFriendGraphQl\Model\Resolver\SendEmailToFriend::getProduct into separate service. thx - this will eliminate two dependencies that break tests

@rleshchenko
Copy link
Contributor Author

@vpodorozh, I've refactored code according to your notes. Please look again

@vpodorozh
Copy link
Contributor

@novikor - please pick up this PR.
Everything seems to be ok from my point of view - so can you just approve it? (from now I do not have permissions any more)

Thx!

@vpodorozh vpodorozh removed their assignment Jun 24, 2019
…velop-678

# Conflicts:
#	app/code/Magento/SendFriendGraphQl/Model/Resolver/SendEmailToFriend.php
#	dev/tests/api-functional/testsuite/Magento/GraphQl/SendFriend/SendFriendTest.php
@lenaorobei lenaorobei changed the base branch from 2.3-develop to 1-graphql-develop-prs-fast June 24, 2019 20:08
@lenaorobei lenaorobei changed the base branch from 1-graphql-develop-prs-fast to 2.3-develop June 24, 2019 20:08
@lenaorobei
Copy link
Contributor

@rleshchenko thank you for the contribution. Few changes I made to your PR:

  • merged @pmclain pmclain:issue/732 branch to your PR since you both were fixing SendFriend module;
  • resolved conflicts;
  • fixed expected exception message in the api-functional test.

@magento magento deleted a comment from magento-cicd2 Jun 24, 2019
@ghost
Copy link

ghost commented Jun 25, 2019

Hi @rleshchenko, 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants