added the export for orders #107
Conversation
| - "@sylius.exporters_transformer_pool" | ||
| tags: | ||
| - { name: sylius.exporter, type: sylius.order, format: csv } | ||
|
|
There was a problem hiding this comment.
Here is a blank line missing at the end of the file
There was a problem hiding this comment.
For me at row 30 I have a blank line?
There was a problem hiding this comment.
Hmm, I don't know what's then the problem there. Github does mention it.
Maybe there is some Whitespace in this line?
| - "@sylius.exporters_transformer_pool" | ||
| tags: | ||
| - { name: sylius.exporter, type: sylius.order, format: xlsx } | ||
|
|
| parent::init($idsToExport); | ||
|
|
||
| /** @var OrderInterface $resource */ | ||
| foreach ($this->resources as $resource) { |
There was a problem hiding this comment.
could you refactor this foreach a bit, so it gets a bit smaller? I would recommend to move the contents of the different if-conditionals to private methods, that would make the code of this init-method a bit clearer to understand.
| $items = []; | ||
|
|
||
| /** @var OrderItemInterface $orderItem */ | ||
| foreach ($resource->getItems() as $orderItem) { |
There was a problem hiding this comment.
Also the following both foreach-loops could be relatively easy extracted to a private-method like 'addOrderItemInformation' or something like that, I think.
| * @param PropertyAccessorInterface $propertyAccessor | ||
| * @param EntityManagerInterface $entityManager | ||
| */ | ||
| public function __construct( |
There was a problem hiding this comment.
this looks redundant and covered by the parent constructor
|
Thanks for the reviews and help, I did the requested changes.. its open for review again :) |
| /** | ||
| * @param OrderInterface $resource | ||
| */ | ||
| private function addCustomerData(OrderInterface $resource) |
There was a problem hiding this comment.
bit nitpicking, but can you add a TypeHint for the retrun-value of all added private Methods?
e.g. private function addCustomerData(OrderInterface $resource): void
We should typeHint as much as we can... :-)
| $items[$product->getName()] = 0; | ||
| } | ||
|
|
||
| $items[$product->getName()] += $orderItem->getQuantity(); |
There was a problem hiding this comment.
How does this with products with the same name but not the same product ID? Basically different product, but the count on the name gets upped.
There was a problem hiding this comment.
yeah .. this is risky .. should use something unique for the key
There was a problem hiding this comment.
Since I already merged this, I think we should open an issue for that. I have totally overseen this..
| if (null !== $shippingAddress) { | ||
| $this->addDataForResource($resource, 'Shipping_address', sprintf( | ||
| '%s, %s, %s, %s, %s', | ||
| $shippingAddress->getFullName() ? $shippingAddress->getFullName() : '', |
There was a problem hiding this comment.
I am not sure if anything is even needed here .. I assume the return value is either a string or null and then it will just silently get cast to an empty string.
There was a problem hiding this comment.
@enesaktay is this something we can make easier to read/use?
There was a problem hiding this comment.
@stefandoorn could potentially remove the ternary operators since with row 58 we are already checking if shippingadress exists
other than that i could also put sprintf inside another method since it is used 2 times in this class
There was a problem hiding this comment.
Indeed, might be worth trying if we need the operator at all as the methods probably return useful information anyway..
| private function addShippingAddressData(OrderInterface $resource): void | ||
| { | ||
| $shippingAddress = $resource->getShippingAddress(); | ||
| if (null !== $shippingAddress) { |
There was a problem hiding this comment.
What about early returns? if (null !== $shippingAddress) { return; }
|
I moved some comments to separate issues with cc to @enesaktay. |
#103 had unwanted commits in it so this is the same PR without the unwanted commits