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

Send an email on low stock #8396

Merged
merged 17 commits into from Oct 18, 2017

Conversation

@tomlev
Copy link
Member

tomlev commented Oct 6, 2017

Questions Answers
Branch? 1.7.3
Description? Add checkbox to get notifications when a product is below the low-stock level, send mail on low-stock quantity update
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/FF-124
How to test? Go to BO product page > quantities (on a without-combinations product), check lowStockAlert checkbox, set not-null quantity on low stock threshold. Validate an order to get product quantity goes below the low stock level. All users with access on BO product page should receive the low-stock alert email. Same test must be ran on combination. Combinations low stock alerts can be edited by bulk actions.

Important guidelines


This change is Reviewable

@PrestaShop PrestaShop deleted a comment Oct 6, 2017
Copy link
Contributor

LittleBigDev left a comment

Nice PR !
Few code style changes are needed though.

@@ -230,6 +230,9 @@ class Combination {
case "low_stock_threshold":
convertedInput = this.inputPattern + 'attribute_' + bulkInput;
break;
case "low_stock_alert":

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

Can't you combine it with case "low_stock_threshold": above ?

@@ -58,6 +58,9 @@ class CombinationCore extends ObjectModel
/** @var int|null Low stock for mail alert */
public $low_stock_threshold = null;
/** @var bool Low stock mail alert activated */
public $low_stock_alert = false;

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

When adding new variables, properties, methods, etc. please use the camelCase notation.

@@ -79,6 +79,9 @@ class ProductCore extends ObjectModel
/** @var int|null Low stock for mail alert */
public $low_stock_threshold = null;
/** @var bool Low stock mail alert activated */
public $low_stock_alert = false;

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

When adding new variables, properties, methods, etc. please use the camelCase notation.

@@ -1397,13 +1401,14 @@ public function addProductAttribute(
$upc,
$minimal_quantity,
$isbn,
$low_stock_threshold = null
$low_stock_threshold = null,
$low_stock_alert = false

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

When adding new variables, properties, methods, etc. please use the camelCase notation.

) {
Tools::displayAsDeprecated();
$id_product_attribute = $this->addAttribute(
$price, $weight, $unit_impact, $ecotax, $id_images,
$reference, $ean13, $default, $location, $upc, $minimal_quantity, array(), null, 0, $isbn, $low_stock_threshold
$reference, $ean13, $default, $location, $upc, $minimal_quantity, array(), null, 0, $isbn, $low_stock_threshold, $low_stock_alert

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor
  • When adding new variables, properties, methods, etc. please use the camelCase notation. (I won't spam more about this rule. Please fix this everywhere).
  • Either all parameters on the same line or one parameter per line
if ($upgrade->hasFailure()) {
die(1);
}

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

According to PSR-2, "All PHP files MUST end with a single blank line."

@@ -139,7 +142,8 @@ public function processProductAttribute($product, $combinationValues)
false,
array(),
$combinationValues['attribute_isbn'],
$combinationValues['attribute_low_stock_threshold']
$combinationValues['attribute_low_stock_threshold'],
$combinationValues['attribute_low_stock_alert']

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

Please add a trailing comma, even to the last array item

This comment has been minimized.

Copy link
@tomlev

tomlev Oct 6, 2017

Author Member

here is a function call ;)

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

ok -> 🚪

) {
return true;
}
}

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

I bet Codacy is grimacing about this if/else block. I don't like it either.
I'd go with :

if ($product->hasAttributes()) {
    $stuffToTest = (bool)$id_product_attribute 
        ? new \Combination($id_product_attribute) 
        : $product;
    if ($stuffToTest->low_stock_alert
        && (int) $stuffToTest->low_stock_threshold > 0
        && $newQuantity <= (int) $stuffToTest->low_stock_threshold
    ) {
        return true;
    }
}

And to go further, I would even create a method testing if "stuff" should trigger an alert (the method would test low_stock_alert, low_stock_threshold and quantity on "stuff")

This comment has been minimized.

Copy link
@tomlev

tomlev Oct 6, 2017

Author Member

I would not factorize here since the objects $product and $combination do not implement same interface : it would inject code refacto hardness
eg: rename Product::$low_stock_alert without renaming Combination::$low_stock_alert (allowed since not implementing same interface)

I understand the code is hard to read, I split it into 2 distinct methods ;)

'PS_SHOP_EMAIL',
'PS_SHOP_NAME',
), null, null, $id_shop
);

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

This method call does not follow the PSR requirements (one parameter per line).

);
// get emails on employees who have right to run stock page
$emails = array();
/** @var \Employee[] $employees */

This comment has been minimized.

Copy link
@LittleBigDev

LittleBigDev Oct 6, 2017

Contributor

It's not possible to enhance \Employee::getEmployees() doc block ?

@PrestaShop PrestaShop deleted a comment Oct 6, 2017
@PrestaShop PrestaShop deleted a comment Oct 6, 2017
@PrestaShop PrestaShop deleted a comment Oct 6, 2017
@@ -1001,6 +1001,7 @@ public function logWarning($quickInfo, $id, $transVariables = array(), $dbInfo =
public function logError($quickInfo, $id, $transVariables = array(), $dbInfo = false)
{
echo $quickInfo."\n";

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Oct 9, 2017

Member

To be removed

<fieldset class="form-group">
{{ form_errors(form.attribute_low_stock_alert) }}
{{ form_widget(form.attribute_low_stock_alert) }}
<span class="help-box" data-toggle="popover" data-html="true" data-content="{{ "The email will be sent to all the users who have there right to run this page. To modify the permissions, go to [1]Advanced Parameters > Team[/1]"|trans({'[1]':'<a href=&quot;'~getAdminLink("AdminEmployees")~'&quot;>','[/1]':'</a>'}, 'Admin.Catalog.Help')|raw }}" ></span>

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Oct 9, 2017

Member

Did you get this line from somewhere else? This "There rights" is disturbing.

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Oct 9, 2017

Hello @tomlev

I receive the email when the stock is = at the low stock level defined on product page.
I think I will have to receive the mail when it's < (and not =)

If I change something with the bulk action (low stock value or something else), it's not saved.
I have added a screenrecord on the forge ticket.

We have a little issue with the wording below:
capture du 2017-10-09 15-50-11
And I think it should be "all the users who have the right to run the stock page" instead of "this page".
I receive the email if I am allowed to see the stock page and not the product page.

Finally, maybe we can improve the display here:
capture du 2017-10-09 15-48-32
And here:
capture du 2017-10-09 16-24-04

@PrestaShop PrestaShop deleted a comment Oct 10, 2017
@emmanuelf75009

This comment has been minimized.

Copy link

emmanuelf75009 commented Oct 10, 2017

@marionf Sorry for the late comment. The user must received the email is the stock are = or < to the low-stock level.
Ok with the wording.

@tomlev tomlev force-pushed the tomlev:FF-124 branch from 6d0d3ce to 9a1c557 Oct 11, 2017
@PrestaShop PrestaShop deleted a comment Oct 11, 2017
@tomlev tomlev force-pushed the tomlev:FF-124 branch from 9a1c557 to 5865a6d Oct 11, 2017
@tomlev tomlev force-pushed the tomlev:FF-124 branch from d779aaa to dc76d91 Oct 12, 2017
@PrestaShop PrestaShop deleted a comment Oct 12, 2017
@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 12, 2017
@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Oct 13, 2017

:lgtm:


Reviewed 10 of 27 files at r1, 2 of 3 files at r2, 5 of 5 files at r5, 2 of 5 files at r6, 8 of 8 files at r7, 5 of 5 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Oct 13, 2017

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment Oct 13, 2017
@maximebiloe

This comment has been minimized.

Copy link
Contributor

maximebiloe commented Oct 13, 2017

⚠️ Be careful, you're adding a new e-mail template. Don't forget to add it here: https://github.com/PrestaShop/ps_emailgenerator/
Otherwise, it won't be translated (ping @eternoendless)

@xBorderie

This comment has been minimized.

Copy link
Contributor

xBorderie commented Oct 16, 2017

Thanks for the heads-up, Maxime!
I'll ping @LouiseBonnard too, who is charge of Content.

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Oct 18, 2017

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Oct 18, 2017

Thank you @tomlev

@eternoendless eternoendless merged commit fa48a13 into PrestaShop:develop Oct 18, 2017
3 checks passed
3 checks passed
codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 30 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maximebiloe

This comment has been minimized.

Copy link
Contributor

maximebiloe commented Oct 18, 2017

It exists for the ps_emailalerts module, not as a core email. You need to move it (into the module), otherwise it won't be found and at best, it will send the english mail, at worst, it will throw a 500 💣

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Oct 18, 2017

Oh you mean it needs to go into templates/core/ on the module!
@tomlev can you see to it please?

@maximebiloe

This comment has been minimized.

Copy link
Contributor

maximebiloe commented Oct 19, 2017

Yes 😄
You can add to the roadmap a task to "templatize" the mails :trollface:

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