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.

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

Prepares Command API for migrating Order view page #13712

Merged
merged 31 commits into from Jul 22, 2019

Conversation

@sarjon
Copy link
Member

commented May 8, 2019

Questions Answers
Branch? develop
Description? This PR prepares new Command API for migrating "Order view" page.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10582
How to test? Not yet.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner May 8, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@matks one more PR for you to review.

Show resolved Hide resolved src/Adapter/Order/CommandHandler/AbstractOrderCommandHandler.php Outdated
Show resolved Hide resolved src/Adapter/Order/CommandHandler/AbstractOrderCommandHandler.php
Show resolved Hide resolved src/Adapter/Order/CommandHandler/AddOrderPaymentHandler.php Outdated
Show resolved Hide resolved src/Adapter/Order/CommandHandler/AddProductToOrderHandler.php
Given there is order with reference "XKBKNABJK"
And order "XKBKNABJK" does not have any invoices
When I generate invoice for "XKBKNABJK" order
Then order "XKBKNABJK" should have invoice

This comment has been minimized.

Copy link
@matks

matks May 9, 2019

Contributor

Maybe we can assert a few properties from the invoice to check it was correctly generated ?

Also if there is a usecase where an invoice cannot be generated from the order (because of a bad status for example), we can add it here 😄

This comment has been minimized.

Copy link
@sarjon

sarjon May 9, 2019

Author Member

Sure, I have a few questions:

  1. Is it ok to use demo data in behat tests?
  2. Is this scenario ok? Do you have any more suggestions regarding steps?

This comment has been minimized.

Copy link
@matks

matks May 10, 2019

Contributor
  1. Yes it's OK to use demo data in behat tests if this helps to avoid big setup steps like "I create a Country, I create a Customer, I create an address for this customer for this country, I create a Category, I create a product in this Category ..."

At some point we'll probably have to expand the demo data to have better usecases ready to be used. Like a visitor, a customer with 1 order, a customer with a lot of orders, a customer with a lot of carts...

  1. This scenario would benefit from a little more assertions 😉we want to catch bad behaviors created from our modifications of the code. Comparing the invoice amount with the order amount could be interesting for example as it would help us feel safe when modifying the computation method.

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

🆙

@sarjon

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

ping @matks

@sarjon

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

@matks could you rerun build? I'm not sure if that was a random error.

@matks matks removed the WIP label May 28, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

ping @matks it's ready for review.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I've got one good news and one bad news :trollface:

The bad news: when I see all the processings you have extracted from the AdminOrdersController, I think "Woah ! So crazy !" then "Sarunas must hate me now" and finally "Sarunas is going to hate me even more".

The 3rd one is because the code is extracted is a mess. Yes, I know this is legacy code but we cannot let it like this.

I think we need at least to define a proper mapping of who does what.

For example, according to DeleteCartRuleFromOrderHandler implementation, to delete a cart rule from an order means:

  • update the related invoice "tax" and "paid" fields
  • update the related order "tax" and "paid" fields
  • delete the cart rule from the order

I'd like us to modify the code to make it very clear. Like

public function handle(DeleteCartRuleFromOrderCommand $command)
    {
        $this->modifyRelatedInvoiceAsPriceEvolves($command);
        $this->modifyRelatedOrderAsPriceEvolves($command);

        $this->deleteCartRule($command);

Then we'll have 3 private functions that, later, we'll be able to plug on correct implementations.
So it'll start like this:

    private function modifyRelatedInvoiceAsPriceEvolves($command) {
            $orderInvoice = new OrderInvoice($orderCartRule->id_order_invoice);
            if (!Validate::isLoadedObject($orderInvoice)) {
                throw new OrderException('Can\'t load Order Invoice object');
            }
            // Update amounts of Order Invoice
            $orderInvoice->total_discount_tax_excl -= $orderCartRule->value_tax_excl;
            $orderInvoice->total_discount_tax_incl -= $orderCartRule->value;
            $orderInvoice->total_paid_tax_excl += $orderCartRule->value_tax_excl;
            $orderInvoice->total_paid_tax_incl += $orderCartRule->value;
            // Update Order Invoice
            $orderInvoice->update();
    }

but one day we'll change it to

    private function modifyRelatedInvoiceAsPriceEvolves($command) {
            $this->orderInvoiceManagementSystem->modifyWhenCartRuleIsDeleted($orderCartRule->id_order_invoice);
    }

To sum it up:

  • I want to make the code easier to read using private function very obvious naming
  • I want to prepare the future to make things right easier later

Oh, the good news ? We'll do it in another PR, not this one 😅

$res = true;
foreach ($cartRules as &$cartRule) {
$cartRuleObj = new CartRule();

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

I would like to improve this although this is not the scope of this PR.
Do you think we can at least add @todo to indicate areas of improvements ?

Here we could create a private function to create the proper CartRuleObject.

if ($res) {
foreach ($cartRules as $orderInvoiceId => $cartRule) {
// Create OrderCartRule

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use private method

throw new OrderException('The discount type is invalid.');
}
$res = true;

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor
Suggested change
$res = true;
$result = true;
$command->getOrderInvoiceId() ?: 0
);
// update totals amount of order

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor
Suggested change
// update totals amount of order
// update totals amount of order
// @todo: use https://github.com/PrestaShop/decimal for prices computations
Hook::exec('actionOrderEdited', ['order' => $order]);
$oldCartRules = Context::getContext()->cart->getCartRules();
$newCartRules = Context::getContext()->cart->getCartRules();

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

😮 what's the difference between $oldCartRules and $newCartRules ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 12, 2019

Author Member

I've moved $oldCartRules to the top since they had to be retrieved from an old cart (because new cart is created somewhere in between).

This comment has been minimized.

Copy link
@matks

matks Jun 12, 2019

Contributor

But now there's nothing in between 🤔

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 12, 2019

Author Member

There will be after I push changes.

// Create Cart rule in order to make free shipping
if ($isFreeShipping) {
$freeShippingCartRule = new CartRule();

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use private method to create cart rule

$carrier = new Carrier((int) $order->id_carrier);
$taxCalculator = $carrier->getTaxCalculator($invoice_address);
$invoice->total_paid_tax_excl = Tools::ps_round((float) $cart->getOrderTotal(false, $totalMethod), 2);

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use https://github.com/PrestaShop/decimal to compute prices and taxes

foreach ($order->getOrderDetailList() as $orderDetail) {
$order_detail = new OrderDetail($orderDetail['id_order_detail']);
$fields = [

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use private method to handle this

}
// Update order && order_invoice amount
$fields = [

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use private method to handle this

Show resolved Hide resolved src/Adapter/Order/CommandHandler/ChangeOrderDeliveryAddressHandler.php
{
if ($orderDetail->id_order_invoice != 0) {
$order_invoice = new OrderInvoice($orderDetail->id_order_invoice);
$order_invoice->total_paid_tax_excl -= $orderDetail->total_price_tax_excl;

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use https://github.com/PrestaShop/decimal for price computations

*/
private function updateOrder(Order $order, OrderDetail $orderDetail)
{
$order->total_paid -= $orderDetail->total_price_tax_incl;

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use https://github.com/PrestaShop/decimal for price computations

}
}
$shippingCostAmount = (float) str_replace(',', '.', $command->getShippingCostRefundAmount()) ?: false;

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

@todo: use dedicated processing to deal with this issue (commas instead of colons)

$voucher = 0;
if ($command->getCartRuleRefundType() === 1) {
$amount -= $voucher = (float) 99; //@todo: get order_discount_price

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

😐 ?

throw new OrderException('Please enter an amount to proceed with your refund.');
}
$choosen = false;

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor
Suggested change
$choosen = false;
$chosen = false;

?

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 12, 2019

Author Member

I guess so. 🤷‍♂

Show resolved Hide resolved src/Core/Domain/Order/Product/Command/AddProductToOrderCommand.php
private $orderId;
/**
* @var int

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor
Suggested change
* @var int
* @var OrderDetailId

?

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 12, 2019

Author Member

Same here, to what Domain does it belong? 🤔

This comment has been minimized.

Copy link
@matks

matks Jun 12, 2019

Contributor

Domain/Order if the domain/Order folder is not too full yet
Else Domain/Order/Detail

Show resolved Hide resolved src/Core/Domain/Product/ValueObject/ProductId.php
$productId = Product::getIdByReference($productReference);
if (!$productId) {
throw new \RuntimeException(sprintf('Product with reference "%s" does not exist.', $productReference));

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

I have always used only RuntimeException in behat tests, because Exception seemed too generic 😅 basically we are writing assertions here, and we dont want to catch them. We want it to fail because it's intended to stop behat from working IMO

Given there is order with reference "XKBKNABJK"
And order "XKBKNABJK" does not have any invoices
When I generate invoice for "XKBKNABJK" order
Then order "XKBKNABJK" should have invoice

This comment has been minimized.

Copy link
@matks

matks Jun 10, 2019

Contributor

🆙

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

I'd like us to modify the code to make it very clear. Like

That makes sense and I totally agree, however, in a lot of cases code is very coupled, for example, method is 300 lines of code, then variables create in the first 50 lines are used in the last 50 lines or somewhere in the middle, so it's not that easy to split the code into little private functions.

Of course it does not apply to every situation.

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@matks let's keep this PR as is (I've addresses some issues, but not all) and:

  • Improve Behat tests with additional tests in separate PR
  • Address refactoring of handlers in separate PR per handler (to keep PRs small)

wdyt?

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@sarjon Agreed

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@sarjon Behat tests are failing, which blocks Travis. Can you fix it, or do you want me to dive into it if the error is irrational ?

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

I'll check it, but it's weird since 5.6 passed, but 7.2 build failed. 🤷‍♂

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

I'll check it, but it's weird since 5.6 passed, but 7.2 build failed. 🤷‍♂

You're right, it probably indicates a random failure 😞

I re-run, we'll see

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Interesting, it's not random.

--- Failed scenarios:
    Features/Scenario/Order/add_product.feature:7
234 scenarios (233 passed, 1 failed)
2821 steps (2819 passed, 1 failed, 1 skipped)
3m29.60s (55.27Mb)
Script @php -d date.timezone=UTC ./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml handling the integration-behaviour-tests event returned with error code 1
@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

Interesting, it's not random.

Rerun again? Let's keep rerunning until it passes. :trollface:

@matks matks referenced this pull request Jul 18, 2019

Open

Migrate "Orders > Orders" page #10582

2 of 38 tasks complete
@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

I'm going to check locally with php7.2

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Re-running Travis for e2e tests

@matks

matks approved these changes Jul 22, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

All green, ready for merge?

@matks matks added this to the 1.7.7.0 milestone Jul 22, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Thank you @sarjon

@matks matks merged commit 16577f6 into PrestaShop:develop Jul 22, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sarjon sarjon deleted the sarjon:m/orders/view branch Jul 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.