-
Notifications
You must be signed in to change notification settings - Fork 154
#141 add simple product to cart #170
#141 add simple product to cart #170
Conversation
@@ -1,4 +1,4 @@ | |||
# CatalogGraphQl | |||
# QuoteGraphQl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like accidental change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Will fix that. Thank you for noticing!
/** | ||
* Catalog product option date validator | ||
*/ | ||
class DateType extends ProductDateOptionType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why it's needed is ProductDateOptionType wants to see date value as an associate array (like ['year' => '2018', 'month' => '1', 'day' => '1']). By this changes, we allow to use string as a date value. This meets our schema shape.
The same approach is used in REST API.
$optionValue = $option->getValueById($itemOption->getValue()); | ||
$selectedOptionValueData['price'] = [ | ||
'type' => strtoupper($optionValue->getPriceType()), | ||
'units' => '$', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this part, including files processing will be completed before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have been implemented this mock. Thank you for noticing!
{ | ||
if (!isset($data['product'])) { | ||
throw new GraphQlInputException( | ||
__('Missing key %1 in cart data', ['product']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this error will be useful for the end user. This should probably be a regular \LogicException
. Same for the other exception below.
What do you think?
} | ||
|
||
type SelectedCustomizableOptionValue { | ||
id: Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ID can be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be. Changing to id: Int!
. Thank you for mentioning!
$errorMessages[] = $error->getText(); | ||
} | ||
|
||
return implode(PHP_EOL, $errorMessages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably better to use semicolon or another separator for the messages in GraphQL output.
@@ -0,0 +1,26 @@ | |||
{ | |||
"name": "magento/module-simple-product-graph-ql", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of CatalogGraphql
module since simple product type is defined in Catalog
module.
QUERY; | ||
$response = $this->graphQlQuery($query); | ||
|
||
var_dump($response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not have var_dump
in tests.
|
||
var_dump($response); | ||
|
||
self::assertArrayHasKey('addSimpleProductsToCart', $response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be perfect to validate all the requested fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address review comments above.
36f075c
to
50a928c
Compare
@roma-glushko I found a few problems
|
3f3556d
to
7990b83
Compare
-- fix static builds
-- fix static builds
-- fix static tests
-- fix static tests
-- fix static tests
-- fix static tests
-- fix static tests
Description
The pull enables adding simple products to customer/guest carts via GraphQl mutations.
In order to add simple product to cart the following information can be passed:
As a result of executing such mutation, the current state of the cart will be returned. Cart items information can vary and depends on product types. For this case, simple cart item contains information about custom options.
Fixed Issues
Manual testing scenarios
The following query can be executed to test the changes: