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

magento/graphql-ce#41: [Query] My Account > My Orders #212

Merged
merged 11 commits into from
Nov 15, 2018
Merged

magento/graphql-ce#41: [Query] My Account > My Orders #212

merged 11 commits into from
Nov 15, 2018

Conversation

sheepfy
Copy link
Contributor

@sheepfy sheepfy commented Oct 14, 2018

IMPORTANT

This interface Magento\CustomerGraphQl\Model\Customer\CheckCustomerAccountInterface;
will be merged in the pull request #213

Description (*)

New module to extract a list of orders by customer

Fixed Issues (if relevant)

#41: [Query] My Account > My Orders

Manual testing scenarios (*)

step 1: Install chrome extension ChromeiQL
step 2: Set endpoint http://<your_host>/graphql?XDEBUG_SESSION_START=idekey
step 3: run grahpql query
{
customerOrders {
items {
id
increment_id
created_at
state
status
grant_total
}
}
}

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Oct 14, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@paliarush paliarush left a comment

Choose a reason for hiding this comment

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

@sheepfy Thank you for the contribution, please address several comments before we can merge the PR.
Also there are some coding standard violations which will be highlighted by the static tests.


$customerId = $this->userContext->getUserId();

$this->checkCustomerAccount->execute($customerId, $this->userContext->getUserType());
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency purposes it is better to get info about current user from the $context object, it is done this way in all other resolvers.

# SalesGraphQl

**SalesGraphQl** provides type and resolver information for the GraphQl module
to generate sales orders information endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

endpoints => queries
endpoints is REST terminology,
queries and mutations is used in GraphQL

**SalesGraphQl** provides type and resolver information for the GraphQl module
to generate sales orders information endpoints.

Also will provides endpoints for modifying an order.
Copy link
Contributor

Choose a reason for hiding this comment

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

endpoints for modifying an order => order mutations

id: Int
increment_id: String
created_at: String
grant_total: Float
Copy link
Contributor

Choose a reason for hiding this comment

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

grant_total => grand_total

increment_id: String
created_at: String
grant_total: Float
state: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to expose state on the storefront?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will tend to provide a lot of info on the order, so the query can be made to extract just you need.

Choose a reason for hiding this comment

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

State is an internal concept, on storefront we normally expose only Status.

Suggested change
state: String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for info. I changed already

status: String
}

type Orders {
Copy link
Contributor

Choose a reason for hiding this comment

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

Orders => CustomerOrders

customerOrders: Orders @resolver(class: "Magento\\SalesGraphQl\\Model\\Resolver\\Orders") @doc(description: "List of customer orders")
}

type Order @doc(description: "Order mapping fields") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Order => CustomerOrder

@@ -0,0 +1,56 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file was committed by accident. Please remove.

query {
customerOrders {
items {
id
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 indentation.

$orders = $this->collectionFactory->create($customerId);
$items = [];

// @TODO Add shipping & billing address in response
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be addressed or moved to new issues before we can merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those todo's can be removed for now, i will come with updates in order to add more info on orders , if that's ok

@sheepfy
Copy link
Contributor Author

sheepfy commented Oct 15, 2018

Using the $context in resolver is a bad thing. You should consider to remove the dependency of $context and inject just what you need.

Anyway, i changed for now.

@magento-engcom-team
Copy link
Contributor

@sheepfy thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

*/
private $checkCustomerAccount;
/**
* @var CheckCustomerAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

Why concrete implementation is preferable in this case?

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.

7 participants