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 Categories Add/Edit actions #10408

Merged
merged 100 commits into from Nov 16, 2018

Conversation

@sarjon
Member

sarjon commented Sep 12, 2018

Questions Answers
Branch? develop
Description? This PR migrates Categories Add & Edit actions.
Type? new feature
Category? BO
BC breaks? yes (overrides)
Deprecations? no
Fixed ticket? #10194
How to test? Access "Categories" page in BO. Actions "Add new root category", "Add new category" and "Edit" are now migrated and should work the same as before. NOTE: in order to keep this PR manageable, some functionality where skipped and will be implemented in another PR.

This change is Reviewable

@matks matks added the migration label Sep 14, 2018

@sarjon sarjon force-pushed the sarjon:migrate/categories-crud branch from 1b12315 to cb69917 Sep 17, 2018

@sarjon sarjon force-pushed the sarjon:migrate/categories-crud branch from 2cc6813 to ea1b2ff Sep 25, 2018

@sarjon sarjon referenced this pull request Sep 25, 2018

Merged

Add TranslatableType #10704

@matks

This comment has been minimized.

Contributor

matks commented Sep 27, 2018

In another PR, I will ask you to write unit tests for command objects. These unit tests should check:

  • the objects can be constructed
  • check that "classic" invalid argument usecases throw exception
  • check interfaces are implemented

These tests are not intended to find bugs now but rather to allow modifications later: if we break basic behavior they should fail (for ex. if we update an Interface without updating the object implementing it)

new TextWithLengthCounter();
new NameToLinkRewriteCopier();
new ChoiceTree('#category_id_parent');

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

👍

export default class NameToLinkRewriteCopier {
constructor() {
['category', 'root_category'].forEach((categoryType) => {
const $categoryForm = $('form[name="' + categoryType + '"]');

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

I guess @PierreRambaud would tell you to use ES6 notation here and in the whole file

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

Yep, our eslint configuration will cry otherwise, and it's pretty much easier to read.

// This is a workaround to make Category's object model work.
// Inside Category::add() & Category::update() method it checks if shop association is submitted
// by retrieving data directly from $_POST["checkBoxShopAsso_category"].
$_POST['checkBoxShopAsso_category'] = $command->getAssociatedShopIds();

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

😨
I know you have to deal with the legacy code but ... this looks so bad ...

This comment has been minimized.

@sarjon
/**
* {@inheritdoc}
*/
public function handle(AddCategoryCommand $command)

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

👍

$this->uploadImages($category, $command);
return new CategoryId($category->id);

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

I'm not sure whether it's useful but ... should we check whether new CategoryId($category->id) is not null ? Since we use ObjectModels which are not very reliable

class CategoryConstraintException extends CategoryException
{
/**
* Code is used when category does not have name.

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

👍

/**
* @param int $categoryId
*/
private function setCategoryId($categoryId)

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

What is the need for a private setter ?

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

If you want to change the id, you need to generate a new class.

@@ -62,6 +62,10 @@
'definition' => $definition,
]);
\Hook::exec('action' . $definition->getId() . 'GridDefinitionModifier', [

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

Why do we use both $this->hookDispatcher->dispatchWithParameter() and \Hook::exec() ? Should not it be either one or the other ?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

using \Hook::exec() works as expected. :D but i unintentionally left it here when playing with hooks, gonna remove it. :)

* This service helps retrieving image path from image tag generated by legacy ImageManager::thumbnail() method
* so image can be displayed in new pages.
*/
final class ImageTagSourceParser implements ImageTagSourceParserInterface

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

This is a great candidate for unit tests. Easy to isolate, easy dependency mocking. Input and outputs are simple.

return $this->handleConstraintException($exception);
}
$messages = [

This comment has been minimized.

@matks

matks Sep 27, 2018

Contributor

Can we rename it availableMessagesForDisplay or something like that ? It took me a few minutes to understand the if (isset($messages[$type])) logic so maybe we can make it a bit more understandable

public function parse($imageTag)
{
$replacement = 'src="/';
$imageTag = preg_replace('/src="(\\.\\.\\/)+/', $replacement, $imageTag);

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

Why \\ only one is needed otherwise you escape the \ and not the .

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

and same as previous broken regex.

return;
}
if ($parentWrapper.hasClass('collapsed')) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

This function says if you're not in expanded context you're in collapsed context, this condition look useless.

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

not really, because if li element does not have any children then its neither in expanded not collapsed context.

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

You need to add a test to be sure there is a closest li to be explicit please.

*/
export default class TextWithLengthCounter {
constructor() {
$(document).find('.js-text-with-counter-input-group').on('input', 'input[type="text"]', (e) => {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

$(document).on('input', '.js-text-with-counter-input-group input[type="text"]' will be faster

$categoryForm
.find('input[name="' + categoryType + '[link_rewrite][' + langId + ']"]')
.val(str2url($nameInput.val(), 'UTF-8'));

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

What is str2url? Idon't see any import

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

it comes from ps-root/js/admin.js which is included in every BO page, so its a global function. i know its not a clean way, but is there a better way to deal with it? 🤔

i saw this function was used the same way in other new JS file. :/

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

Could you have the time to add it? in admin-dev/themes/new-theme/js/components ?

public function handle(AddRootCategoryCommand $command)
{
$category = new Category();
$category->is_root_category = 1;

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

is_ means it should be a boolean no?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

in theory yes, but this object model fields are directly linked to db table, so it's 1 either way. :/

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

If you speak about:

`is_root_category` tinyint(1) NOT NULL DEFAULT '0',`

This is the way MySQL declare boolean.. Yeah, f**k logic, so you can set value to true.

More, the ObjectModel says:

    /** @var bool is Category Root */
    public $is_root_category;
*/
public function retrieve($categoryId)
{
$imageType = 'jpg';

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

const or options please.

$replacement = 'src="' . $this->legacyContext->getRootUrl();
$imageTag = preg_replace('/src="(\\.\\.\\/)+/', $replacement, $imageTag);
preg_match('@src="([^"]+)"@', $imageTag, $path);

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

see this regex twice at least, should be a const or a generic function.

// because legacy uses relative path to reach a directory under root directory...
$replacement = 'src="' . $this->legacyContext->getRootUrl();
$imageTag = preg_replace('/src="(\\.\\.\\/)+/', $replacement, $imageTag);

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

Broken regex. you don't escape the /, and \\ means you match the character \ literally.

Should be : src="(\.\.\/)+

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

thanks, im not really good at regex. :(

preg_match('@src="([^"]+)"@', $imageTag, $path);
return $path[1];

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

make sure $path[1] existe, can happened if src is empty (due to + instead of *)

ImageManager::resize(_PS_TMP_IMG_DIR_ . 'category_' . $categoryId->getValue() . '-thumb.jpg', _PS_TMP_IMG_DIR_ . 'category_' . $categoryId->getValue() . '-thumb.jpg', (int) $image_type['width'], (int) $image_type['height']);
}
$thumbSize = file_exists($thumb) ? filesize($thumb) / 1000 : false;

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

should be 1024 with filesize no?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

nope, 1024 would be for KiB and 1000 is for kB.

https://i.imgur.com/faKgB6e.png

i wanted to introduce some kind of size converter as its pretty common task, something like:
$converter->convert(new Bytes(26589), SIZE::KILOBYTE)

or (new Bytes(26589))->toKilobytes()

This comment has been minimized.

@jolelievre

jolelievre Sep 28, 2018

Contributor

this is so fucked up :P I got enraged when I learnt (only 8 months ago..) that this was the official conversion 😔
or how the Disk sellers managed to lobby brain the International System of Unit...

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

@jolelievre so true, that's why I prefer the usage of kiB instead of kB ^^

{
$menuThumbnails = [];
for ($i = 0; $i < 3; ++$i) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

Why 3?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

its maximum number of menu thumbnails category can have, i cant think of a good place to put this constant? somewhere under Domain/Category 🤔 ? @matks wdyt?

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

👍 for const even if I don't understand why there is a limit 😄

}
}
\Tools::clearSmartyCache();

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

Is there a Tools adapter instead?

/**
* @var string[]
*/
private $description;

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

same as @matks comments, so many @string[] but not in plurial.

*
* @throws CategoryConstraintException
*/
public function setName(array $name)

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

setNames?

return $this;
}
}

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

I'm really confused with this classe, many array without plurial :/

*
* @throws CategoryConstraintException
*/
public function __construct(array $name, array $linkRewrite, $isActive, $parentCategoryId)

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

same as previous comments, array $names or string $name? A category can have many names?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

same as above, it depends on how you think about it, is it single name in different languages or is it different names?

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 28, 2018

Contributor

In both situation I think if you have a name in different languages, you have many languages, so, many names :/

prestashop.adapter.category.command_handler.add_category_handler:
class: 'PrestaShop\PrestaShop\Adapter\Category\CommandHandler\AddCategoryHandler'
parent: 'prestashop.adapter.category.command_handler.abstract_category_handler'
public: true

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 27, 2018

Contributor

default is set to public, any reason to keep it here?

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

yeah, when you use parent you must explicitly set it to public otherwise is thrown as it cannot be inherited from _defaults.

/**
* Class GetDefaultGroups returns default groups data.
*/
class GetDefaultGroups

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 15, 2018

Contributor

empty object? if a Query is expected, why we dont have an interface for it? :)

@@ -1,6 +1,14 @@
<?php
/**
<<<<<<< HEAD

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 15, 2018

Contributor

conflicts here

{
const EXCEEDED_SIZE = 1;
const UNRECOGNIZED_FORMAT = 2;
const UNKNOWN_ERROR = 4;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 15, 2018

Contributor

not sure it's possible but how about:

try
{
    // Some code...
}
catch(Exception1 | Exception2 | Exception3 $e )
{
    
}
@matks

This comment has been minimized.

Contributor

matks commented Nov 15, 2018

@mickaelandrieu As this PR grew too big and too old, we're going to merge it then do a 2nd one which will handle your feedbacks, other code review feedbacks and QA validation 😄

@matks matks merged commit 15b067f into PrestaShop:develop Nov 16, 2018

0 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@eternoendless eternoendless added this to the 1.7.6.0 milestone Dec 7, 2018

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