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

Buy button lite module #1

Merged
merged 6 commits into from Sep 26, 2018

Conversation

Projects
None yet
7 participants
@Joukz
Contributor

Joukz commented Sep 18, 2018

There are still some issues to fix, like the slider size and some css issues.
However, you can already review the code :)

Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated app/src/App.vue Outdated
Show outdated Hide outdated app/src/App.vue Outdated
Show outdated Hide outdated app/src/App.vue Outdated
Show outdated Hide outdated app/src/App.vue Outdated
Show outdated Hide outdated app/src/App.vue Outdated
'bannerPromoTranslations' => json_encode($bannerPromoTranslations),
'adminAjaxController' => $adminAjaxController,
'trackingAddonsLink' => $trackingAddons,
'psBaseUrl' => Tools::getHttpHost(true),

This comment has been minimized.

@matks
@matks

matks Sep 19, 2018

👍

$id_shop = $context->shop->id;
$query = pSQL(Tools::getValue('product_search'));
$sql = new DbQuery();

This comment has been minimized.

@matks

matks Sep 19, 2018

this huge block could be moved into a getSqlQuery() function to improve the readability of ajaxProcessSearchProducts()

@matks

matks Sep 19, 2018

this huge block could be moved into a getSqlQuery() function to improve the readability of ajaxProcessSearchProducts()

This comment has been minimized.

@Joukz

Joukz Sep 20, 2018

Contributor

Done :)

@Joukz

Joukz Sep 20, 2018

Contributor

Done :)

Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
$sql->leftJoin('product_attribute', 'pa', 'pa.`id_product` = p.`id_product`');
$where = 'pl.`name` LIKE \'%' . pSQL($query) . '%\'
OR p.`reference` LIKE \'%' . pSQL($query) . '%\'
OR EXISTS(SELECT * FROM `' . _DB_PREFIX_ . 'product_supplier` sp WHERE sp.`id_product` = p.`id_product` AND `product_supplier_reference` LIKE \'%' . pSQL($query) . '%\')';

This comment has been minimized.

@matks

matks Sep 19, 2018

And this SQL query is complex. It obviously carries a lot of meaning and logic in it 😉 what about split it in several parts easier to understand ?
Example:

$queryBeginning = '...';
/* (explanation about why this EXIST statement is required */
$firstExistStatement = '...':
/* (explanation about why this 2nd EXIST statement is required */
$secondExistStatement = '...';

$completeQuery = $queryBeginning . $firstExistStatement . $secondExistStatement . ...
@matks

matks Sep 19, 2018

And this SQL query is complex. It obviously carries a lot of meaning and logic in it 😉 what about split it in several parts easier to understand ?
Example:

$queryBeginning = '...';
/* (explanation about why this EXIST statement is required */
$firstExistStatement = '...':
/* (explanation about why this 2nd EXIST statement is required */
$secondExistStatement = '...';

$completeQuery = $queryBeginning . $firstExistStatement . $secondExistStatement . ...
class AdminAjaxPs_buybuttonliteController extends ModuleAdminController
{
public function ajaxProcessSearchProducts()

This comment has been minimized.

@matks

matks Sep 19, 2018

Some phpDoc would be nice 😉

@matks

matks Sep 19, 2018

Some phpDoc would be nice 😉

This comment has been minimized.

@Joukz

Joukz Sep 20, 2018

Contributor

Added! :)

@Joukz

Joukz Sep 20, 2018

Contributor

Added! :)

$this->content = json_encode($results_array);
}
public function getProductImage($id_product)

This comment has been minimized.

@matks

matks Sep 19, 2018

phpDoc is a must ;)

@matks

matks Sep 19, 2018

phpDoc is a must ;)

This comment has been minimized.

@Joukz

Joukz Sep 20, 2018

Contributor

Added EVERYWHERE ! ;)

@Joukz

Joukz Sep 20, 2018

Contributor

Added EVERYWHERE ! ;)

public function getAttributes($id_product_attribute, $id_lang)
{
$sql = 'SELECT agl.name as label, al.name as value

This comment has been minimized.

@matks

matks Sep 19, 2018

👍 This SQL query is a lot easier to read thanks to the nice way it is written

Bonus: table aliases al, ag and agl are hard to distinguish, maybe use more meaningful names ?

@matks

matks Sep 19, 2018

👍 This SQL query is a lot easier to read thanks to the nice way it is written

Bonus: table aliases al, ag and agl are hard to distinguish, maybe use more meaningful names ?

$results = Db::getInstance(_PS_USE_SQL_SLAVE_)->executeS($sql);
foreach ($results as $attribute) {
$attributes[] = implode($attribute, ' - ');

This comment has been minimized.

@matks

matks Sep 19, 2018

Can you initialize this $attributes as an empty array ? It is better of $results is empty

$attributes = [];
foreach ($results as $attribute) {
...
@matks

matks Sep 19, 2018

Can you initialize this $attributes as an empty array ? It is better of $results is empty

$attributes = [];
foreach ($results as $attribute) {
...
Show outdated Hide outdated controllers/admin/index.php Outdated
Show outdated Hide outdated controllers/front/index.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated ps_buybuttonlite.php Outdated
Show outdated Hide outdated translations/index.php Outdated
import PsCard from '@/components/PsCard.vue'
import PsCardHeader from '@/components/PsCardHeader.vue'
import PsCardBlock from '@/components/PsCardBlock.vue'
import PsCardFooter from '@/components/PsCardFooter.vue'

This comment has been minimized.

@CaptainYouz

CaptainYouz Sep 19, 2018

Is this application going to grow ? If yes (and in 99% of the time), it is a good way to think in a 'component way'.
Here, you create a component for each part of your layout application. This is a good habit to have.
But in the case of this application, since:
1- it's a very small app
2- the fact that I don't think it is going to grow in a bigger application
3- and the fact that all of Ps* components have a very basic behavior
it would easier to implement the behavior of those components directly in this file, without creating a file every time. It would be as easy to understand as the current behavior, and will make your code lighter. Also, less files, less imports, better optimization.

And if the app would grow, then, it would be better to have a parent state (with vue-router), to handle all the common layout.

@CaptainYouz

CaptainYouz Sep 19, 2018

Is this application going to grow ? If yes (and in 99% of the time), it is a good way to think in a 'component way'.
Here, you create a component for each part of your layout application. This is a good habit to have.
But in the case of this application, since:
1- it's a very small app
2- the fact that I don't think it is going to grow in a bigger application
3- and the fact that all of Ps* components have a very basic behavior
it would easier to implement the behavior of those components directly in this file, without creating a file every time. It would be as easy to understand as the current behavior, and will make your code lighter. Also, less files, less imports, better optimization.

And if the app would grow, then, it would be better to have a parent state (with vue-router), to handle all the common layout.

axios.post(adminAjaxController, formData)
.then((response) => {
cb(response.data)
})

This comment has been minimized.

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! No errors handling (.catch)

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! No errors handling (.catch)

}
}).then((response) => {
this.product = response.data
})

This comment has been minimized.

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! No error handling (.catch)

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! No error handling (.catch)

}
</style>
<style lang="scss">

This comment has been minimized.

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! Two style element, is that normal?

@CaptainYouz

CaptainYouz Sep 19, 2018

Warning! Two style element, is that normal?

$this->displayName = $this->trans('Buy button lite', array(), 'Modules.Buybuttonlite.Admin');
$this->description = $this->trans('Increase your conversation rate and boost your sales, generate links and add them to your content so that visitors can easily proceed to checkout', array(), 'Modules.Buybuttonlite.Admin');
$this->ps_version = (bool)version_compare(_PS_VERSION_, '1.7', '>=');

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

When I see the module can only be installed on 1.7.0.0 minimum, I guess this variable is always true.

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

When I see the module can only be installed on 1.7.0.0 minimum, I guess this variable is always true.

@Shiryu75 Shiryu75 merged commit fda6ce5 into PrestaShop:dev Sep 26, 2018

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 26, 2018

@Joukz Since you merged this PR, does this mean all issues are fixed now 😄 ? Can QA start the validation process ?

matks commented Sep 26, 2018

@Joukz Since you merged this PR, does this mean all issues are fixed now 😄 ? Can QA start the validation process ?

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