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

Migrate Customer view page #10992

Merged
merged 51 commits into from Nov 23, 2018

Conversation

@sarjon
Member

sarjon commented Oct 12, 2018

Questions Answers
Branch? develop
Description? Migrates Customers information page.
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #11010
How to test? Access admin-dev/index.php/sell/customers/1/view and compare page with legacy one. NOTE: it does not implement "Transform to a customer account" and "Add a private note" features to keep this PR manageable. It will be done in separate PR.

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 12, 2018

@matks can you take a look at this wip pr? i dont know if its okay to use that many dto object, wdyt?

@matks matks added the migration label Oct 12, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 15, 2018

@sarjon Yes, this page is complex so using DTOs here is a good idea. The alternative would be to have either an insane number of function arguments or big arrays that provide no guarantee about their content.

However we can try to make these DTOs very easy-to-use and easy-to-extend for module developers. One idea I can think about is to add a createFromArray() static function for them.

This allows a module developers that is used to manage customer order data by arrays to do this:

$this_is_my_array_I_love_to_use = array(
    'orderId' => ...,
    'orderPlacedDate' => ...,
    'paymentMethodName'=>...
);

// now the migrated page in SF wants a DTO ? okay I'll do that:
$this_is_the_dto_I_have_to_build = CustomerOrderInformation::createFromArray($this_is_my_array_I_love_to_use);

// yay ! this new concept "DTO" is not painful to handle

@sarjon sarjon changed the title from [WIP] Migrate Customer view page to Migrate Customer view page Oct 23, 2018

@matks

1st review (there will be more): reviewed GetCustomerInformationHandler

$this->assertCustomerWasFound($customerId, $customer);
Context::getContext()->customer = $customer;

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

Woah 😮 legacy behavior ?

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

yeah, you must set customer to Context in order to make it work. :/

$customerId = $query->getCustomerId();
$customer = new Customer($customerId->getValue());
$this->assertCustomerWasFound($customerId, $customer);

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

👍

if ($totalPaid = Db::getInstance()->getValue($sql)) {
$sql = '
SELECT SQL_CALC_FOUND_ROWS COUNT(*)

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

No mistake ? SQL_CALC_FOUND_ROWS along with COUNT(*) ?

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

well... i thought about trying to refactor it, but dont know if its worth, do you? it will be refactored later at some point and this code works for now.

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

We need to improve this, following the boy scout rule 😉

Db::getInstance()->getValue($sql);
return (int) Db::getInstance()->getValue('SELECT FOUND_ROWS()') + 1;

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

And you use FOUND_ROWS after SQL_CALC_FOUND_ROWS and COUNT(*) 🤔 . I'm sorry I don't get it.
Even if this comes from legacy code, we need to improve it.

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

🆙

if (!isset($order['order_state'])) {
$order['order_state'] = $this->translator->trans(
'There is no status defined for this order.',

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

Should this be here or in the Twig template ? Is it really an order_state property of the Order, or is it a display rule only for templating ?

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

i think its for templating, but since query GetCustomerInformation is all about getting view data, maybe its ok? 🤔 if you think no, ill update. :)

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

I think this will be better in the template

if ($order['valid']) {
$validOrders[] = $customerOrderInformation;
$totalSpent += $order['total_paid_real_not_formated'] / $order['conversion_rate'];

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

Do not assume $order['conversion_rate'] will be an integer different from zero. Assert it 😉

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

it must be. :) it comes directly from db and it's decimal(13,6).

This comment has been minimized.

@matks

matks Oct 31, 2018

Contributor

Still cant assume it. Do not rely on your database structure. Trust no one 😛

This comment has been minimized.

@sarjon

sarjon Nov 7, 2018

Member

well, it was working for years (probably :D), so i think it safe to leave it like it is, at some point it will be totally refactored along with other adapters. :)

however, if you really want me to assert it, then what fallback action should it take if it fails?

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

We can assume that if the data is not valid, although it comes from the database, it must be

  • either a developer modifying the code, so it's a mistake
  • or a database structure not valid
    In both cases we should throw an Exception as these are critical scenarii so we can fail the request. Crash early.

This comment has been minimized.

@sarjon

sarjon Nov 12, 2018

Member

ok, but what exception should be thrown here? its part of domain, but i dont think its neither CustomerException nor DomainException would be correct here, maybe global LogicException, wdyt?

foreach ($carts as $cart) {
$cart = new Cart((int) $cart['id_cart']);
Context::getContext()->cart = $cart;

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

What is the purpose of setting Context::getContext()->cart in this foreach loop ?

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

to make legacy things work. :D i know it sucks, but in this case it must be set to make $cart->getSummaryDetails(); work. :/

);
if (!Validate::isLoadedObject($product)) {
continue;

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

Should we continue ... or display a warning message (although I dont know how - exception ? flash message ?) ? It might be a PM decision.

This comment has been minimized.

@sarjon

sarjon Nov 5, 2018

Member

i think not a flash message for sure, because command can be dispatched using console application as well.

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

Since this is a view, I would not want it to crash because of a bad product.
But we can log the issue at least :) so that it's easier to debug if someone is wondering "why my product X is not displayed in my customer view ?"

This comment has been minimized.

@sarjon

sarjon Nov 12, 2018

Member

hmm, i think it may be done this way on purpose.

if admin deletes a product that was previously viewed by customers, i dont think he cares any more about it, right? so why would you want to have history of all products (even those who were deleted year or more ago) that customer viewed? wdyt?

$customerMessages[] = new MessageInformation(
(int) $message['id_customer_thread'],
substr(strip_tags(html_entity_decode($message['message'], ENT_NOQUOTES, 'UTF-8')), 0, 75),

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

😅

This comment has been minimized.

@sarjon

sarjon Oct 24, 2018

Member

i think this can be done in twig? :D

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

Is this processing intended to decode the stored data or is it intended to display properly the data in an HTML page ?
If answer is 1, we keep it here. If answer is 2, this goes in the template

This comment has been minimized.

@sarjon

sarjon Nov 12, 2018

Member

to be honest, i dont really know why it was done this way. :/ i've simplified it a little.

$customerMessages = [];
$messages = CustomerThread::getCustomerMessages((int) $customer->id);
$messageStatues = [

This comment has been minimized.

@matks

matks Oct 24, 2018

Contributor

$messageStatues => $messageStatuses

</div>
<div class="col-8">
{% if customerInformation.generalInformation.customerBySameEmailExists %}
<p>{{ 'A registered customer account using the defined email address already exists. '|trans({}, 'Admin.Orderscustomers.Notification') }}</p>

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Oct 24, 2018

Contributor

Watch the space at the end of the wording, unless it is made on purpose?

This comment has been minimized.

@sarjon

sarjon Nov 5, 2018

Member

translation key contains this space. :/

@matks

2nd review done :)

Show resolved Hide resolved src/Core/Domain/Customer/Dto/CartInformation.php
Show resolved Hide resolved src/Core/Domain/Customer/Dto/CustomerInformation.php
* @param OrderInformation[] $validOrders
* @param OrderInformation[] $invalidOrders
*/
public function __construct($totalSpent, array $validOrders, array $invalidOrders)

This comment has been minimized.

@matks

matks Oct 31, 2018

Contributor

Can we add smoke tests for DTOs ? It would be a test that creates the object, and calls 2 getters "just to check it works". It ensures that, if someone modifies the class and introduces a syntax error (missing ;), it gets caught immediately. It's a very basic test that allows to perform big IDE-driven refactoring such as namespace renaming or class/methods renaming 😃

Exemple

class OrdersInformationTest extends TestCase
{
    public function testCanBeBuilt()
    {
        $a = new OrdersInformation(10, [], []);
        $this->assertEquals('a', $a-> getTotalSpent());
    }
}

This comment has been minimized.

@matks

matks Oct 31, 2018

Contributor

Same for all DTO

This comment has been minimized.

@sarjon

sarjon Nov 5, 2018

Member

yes, sure. 👍

This comment has been minimized.

@matks

matks Nov 8, 2018

Contributor

I added a checkbox here #11010 (comment)
so once we have merged this PR we can do a 2nd PR to do these tests

Show resolved Hide resolved src/Core/Domain/Customer/Dto/OrdersInformation.php
@sarjon

This comment has been minimized.

Member

sarjon commented Nov 5, 2018

@matks currently command is called GetCustomerInformation but now i think that GetCustomerForViewing name would be more appropriate as it reflects "View" action in Back Office, wdyt?

@matks

This comment has been minimized.

Contributor

matks commented Nov 8, 2018

@sarjon You are right, this data is definitely "enriched" for view so this name is better 👍

@sarjon sarjon force-pushed the sarjon:migrate/customers-view branch from 735defa to 6f2ae4b Nov 12, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 12, 2018

@matks i've addressed all your comments, there are still few minor questions to resolve but i dont think this will change PR a lot, so maybe you can add waiting for QA? :)

@matks

This comment has been minimized.

Contributor

matks commented Nov 14, 2018

@sarjon Yes 😉 we only got a small lint step for Travis to go green

@marionf marionf self-assigned this Nov 14, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Nov 15, 2018

Hello @sarjon

In "already see" block, product's name are not correctly displayed when there is accents

capture d ecran_623

In "orders" block, when I click to display an order, I am redirected to the list of orders instead of the specific order. In the link I see "viewcustomer", before it was "vieworder".

capture d ecran_620

@marionf

This comment has been minimized.

Contributor

marionf commented Nov 15, 2018

@TristanLDD there are many things on this page, tell me when you want to look at it

@sarjon sarjon force-pushed the sarjon:migrate/customers-view branch from 6f2ae4b to 081e4c7 Nov 15, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 15, 2018

@marionf thanks for feedback, issues were fixed. :)

@sarjon sarjon force-pushed the sarjon:migrate/customers-view branch from 46c7121 to 0d2109a Nov 23, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 23, 2018

rebased. :)

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 23, 2018

@PierreRambaud can you merge? ^^

@Quetzacoalt91

Let's merge before the next conflicts.

@Quetzacoalt91 Quetzacoalt91 merged commit 44943cc into PrestaShop:develop Nov 23, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Nov 23, 2018

Thank you @sarjon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment