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.

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

MAGENTO-PWA#2190: Fixed dependent GraphQL schema according to best practices #27620

Closed
wants to merge 1 commit into from
Closed

MAGENTO-PWA#2190: Fixed dependent GraphQL schema according to best practices #27620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/code/Magento/QuoteGraphQl/Model/Resolver/MergeCarts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ public function __construct(
*/
public function resolve(Field $field, $context, ResolveInfo $info, array $value = null, array $args = null)
{
if (empty($args['source_cart_id'])) {
if (empty($args['input']['source_cart_id'])) {
throw new GraphQlInputException(__('Required parameter "source_cart_id" is missing'));
}

if (empty($args['destination_cart_id'])) {
if (empty($args['input']['destination_cart_id'])) {
throw new GraphQlInputException(__('Required parameter "destination_cart_id" is missing'));
}

Expand All @@ -61,8 +61,8 @@ public function resolve(Field $field, $context, ResolveInfo $info, array $value
throw new GraphQlAuthorizationException(__('The current customer isn\'t authorized.'));
}

$guestMaskedCartId = $args['source_cart_id'];
$customerMaskedCartId = $args['destination_cart_id'];
$guestMaskedCartId = $args['input']['source_cart_id'];
$customerMaskedCartId = $args['input']['destination_cart_id'];

$currentUserId = $context->getUserId();
$storeId = (int)$context->getExtensionAttributes()->getStore()->getId();
Expand Down
7 changes: 6 additions & 1 deletion app/code/Magento/QuoteGraphQl/etc/schema.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type Mutation {
setPaymentMethodOnCart(input: SetPaymentMethodOnCartInput): SetPaymentMethodOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentMethodOnCart")
setGuestEmailOnCart(input: SetGuestEmailOnCartInput): SetGuestEmailOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetGuestEmailOnCart")
setPaymentMethodAndPlaceOrder(input: SetPaymentMethodAndPlaceOrderInput): PlaceOrderOutput @deprecated(reason: "Should use setPaymentMethodOnCart and placeOrder mutations in single request.") @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentAndPlaceOrder")
mergeCarts(source_cart_id: String!, destination_cart_id: String!): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts")
mergeCarts(input: MergeCartsInput): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the backward compatibility of this change.
@lenaorobei, could you confirm, please, that we may perform such changes?

Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of mergeCarts was added based on approved proposal from the architecture repo https://github.com/magento/architecture/blob/master/design-documents/graph-ql/coverage/shared-cart.md

Need to understand how this will help PWA devs.

Addind @cpartica @paliarush

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewbess could you please explain the isse with using mergeCarts(source_cart_id: String!, destination_cart_id: String!)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the main concern here is extensibility. If we use a separate type for input (as we do in most cases), we may extend this input (add fields, for example) in a separate module. If we have the parameters hardcoded as they are now, the extensibility is compromised.

But the main question here is the backward compatibility. If some merchant already uses the existing mergeCarts mutation somehow, he will be surprised after the upgrade.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @rogyar
The GraphQL for merging carts looks like incorrect. It doesn't use Magento best practices.
I don't understand why it has been changed from the correct code to incorrect.
Please take a look at commits below.
Correct commit (IMHO)
magento/graphql-ce#950
Incorrect commit that changes previous (IMHO)
f19f727
I also want to note that this is a relatively new feature.
It appeared only in version 2.3.4

Copy link
Author

Choose a reason for hiding this comment

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

Hello @lenaorobei
Thank you for your question.
This mutation isn't written according to Magento best practices.
Currently, I am working on PWA-studio#2190.
So, I think it would be better to change it now and don't alter PWA after changing this mutation in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rogyar regarding your concern about extensibility - both options should work - adding new field and parameter.

@andrewbess there was an issue on the DevDocs page that is already addressed magento/devdocs#7001
This recommendation was outdated.

Please refer to the extensibility approach in the architecture repo https://github.com/magento/architecture/blob/master/design-documents/graph-ql/extensibility.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @lenaorobei. Thank you for clarification. We used to use containers for input arguments for merging purposes. So containers with the same name from different modules were supposed to be merged. If you can confirm that the raw input parameters approach (with no containers) has merging capabilities, then we good to go.

Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

@rogyar yes, mutations/queries with the same name are supposed to be merged. This is the way of adding new parameters.

I would really appreciate if someone can double check that.

placeOrder(input: PlaceOrderInput): PlaceOrderOutput @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\PlaceOrder")
}

Expand Down Expand Up @@ -130,6 +130,11 @@ input SetPaymentMethodAndPlaceOrderInput {
payment_method: PaymentMethodInput!
}

input MergeCartsInput {
source_cart_id: String!
destination_cart_id: String!
}

input PlaceOrderInput {
cart_id: String!
}
Expand Down