From 368e2b6a4dc37a15834d47178213429a11e68a2e Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Fri, 10 Aug 2012 15:37:15 -0400 Subject: [PATCH 1/5] Validation change for Text type attributes. The call on Mage_Eav_Model_Attribute_Data_Text:67 to empty returns true of a value of "0" is entered, as specified in the PHP spec. This prevents the entry of zero on the backend admin for fields. Zero should be a valid value, as it was in our case. No unit tests were created, as no unit tests exist currently for the Eav module. --- app/code/core/Mage/Eav/Model/Attribute/Data/Text.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/core/Mage/Eav/Model/Attribute/Data/Text.php b/app/code/core/Mage/Eav/Model/Attribute/Data/Text.php index c48ed5932025f..15432541815dc 100644 --- a/app/code/core/Mage/Eav/Model/Attribute/Data/Text.php +++ b/app/code/core/Mage/Eav/Model/Attribute/Data/Text.php @@ -64,7 +64,7 @@ public function validateValue($value) $value = $this->getEntity()->getDataUsingMethod($attribute->getAttributeCode()); } - if ($attribute->getIsRequired() && empty($value)) { + if ($attribute->getIsRequired() && empty($value) && $value !=='0') { $errors[] = Mage::helper('Mage_Eav_Helper_Data')->__('"%s" is a required value.', $label); } From 951423ae71491b65091b6101e5926873f497a293 Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Sun, 12 Aug 2012 16:59:21 -0400 Subject: [PATCH 2/5] Unit test created for previous commit (368e2b6a4dc37a15834d47178213429a11e68a2e) Sorry for not including this originally. --- .../Eav/Model/Attribute/Data/TextTest.php | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php diff --git a/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php b/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php new file mode 100644 index 0000000000000..33bcc1971c9de --- /dev/null +++ b/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php @@ -0,0 +1,102 @@ +_model = $this->getMock('Mage_Eav_Model_Attribute_Data_Text', array('getAttribute'), array(), '', false); + $attributeData = array( + 'store_label' => 'Test', + 'attribute_code' => 'test', + 'is_required' => 1, + 'validate_rules' => array( + 'min_text_length' => 0, + 'max_text_length' => 0, + 'input_validation' => 0 + ) + ); + + /** @var $model Mage_Core_Model_Abstract */ + $attribute = $this->getMock('Mage_Core_Model_Abstract', null, array($attributeData)); + //$this->_model->_attribute = $attribute; + $this->_attribute = $attribute; + $this->_model->expects($this->any()) + ->method('getAttribute') + ->will($this->returnValue($this->_attribute)); + $helper = $this->getMockBuilder('Mage_Core_Helper_String') + ->setMethods(array('__')) + ->disableOriginalConstructor() + ->getMock(); + $helper->expects($this->any()) + ->method('__') + ->will($this->returnArgument(0)); + Mage::register('_helper/Mage_Eav_Helper_Data', $helper); + Mage::register('_helper/Mage_Core_Helper_String', $helper); + + + } + + protected function tearDown() + { + $this->_model = null; + Mage::unregister('_helper/Mage_Eav_Helper_Data'); + Mage::unregister('_helper/Mage_Core_Helper_String'); + } + + /** + * This test is to check the change made to validateValue. + * A bug was found where a text attribute that has is_required==1 + * would not accept the string value of "0" (zero) as an input. + * That bug was fixed. + * @covers Mage_Eav_Model_Attribute_Data_Text::validateValue + * @param string|int|float|array $value + * @param string|int|float|array $expectedResult + * @dataProvider dataGetValuesAndResults + */ + public function testValidateValue($value, $expectedResult) + { + $this->assertEquals($expectedResult, $this->_model->validateValue($value)); + } + + public static function dataGetValuesAndResults() + { + return array( + array("0",true), //The string value of zero should be a valid input + array(0, array('"%s" is a required value.')) //Integer value of zero remains invalid + ); + } +} \ No newline at end of file From f0f7857014010c435a45c435ecd62e50dc2e5fe7 Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Sun, 12 Aug 2012 16:59:57 -0400 Subject: [PATCH 3/5] Removed commented out line. --- .../unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php b/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php index 33bcc1971c9de..e7f438cf5a5ab 100644 --- a/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php +++ b/dev/tests/unit/testsuite/Mage/Eav/Model/Attribute/Data/TextTest.php @@ -52,7 +52,7 @@ protected function setUp() /** @var $model Mage_Core_Model_Abstract */ $attribute = $this->getMock('Mage_Core_Model_Abstract', null, array($attributeData)); - //$this->_model->_attribute = $attribute; + $this->_attribute = $attribute; $this->_model->expects($this->any()) ->method('getAttribute') @@ -81,7 +81,7 @@ protected function tearDown() * This test is to check the change made to validateValue. * A bug was found where a text attribute that has is_required==1 * would not accept the string value of "0" (zero) as an input. - * That bug was fixed. + * That bug was fixed. * @covers Mage_Eav_Model_Attribute_Data_Text::validateValue * @param string|int|float|array $value * @param string|int|float|array $expectedResult From 697ad8e0b08fcc9307047205dbeae789a1bc328b Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Thu, 30 Aug 2012 10:03:15 -0400 Subject: [PATCH 4/5] Bug - Mage_Sales_Model_Order_Invoice_Total_Shipping:50 The line was if($previusInvoice->getShippingAmount() && ... This would return true if a previous invoice had zero shipping because it was returned as a string from the database. The string "0.0000". The line needs to be changed to have a greaterthan comparison to cause the string to cast to a numeric. Also added unit test. --- .../Model/Order/Invoice/Total/Shipping.php | 2 +- .../Order/Invoice/Total/ShippingTest.php | 84 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php diff --git a/app/code/core/Mage/Sales/Model/Order/Invoice/Total/Shipping.php b/app/code/core/Mage/Sales/Model/Order/Invoice/Total/Shipping.php index 8ccba86065c7b..09eefb132d5c1 100644 --- a/app/code/core/Mage/Sales/Model/Order/Invoice/Total/Shipping.php +++ b/app/code/core/Mage/Sales/Model/Order/Invoice/Total/Shipping.php @@ -46,7 +46,7 @@ public function collect(Mage_Sales_Model_Order_Invoice $invoice) * Check shipping amount in previus invoices */ foreach ($invoice->getOrder()->getInvoiceCollection() as $previusInvoice) { - if ($previusInvoice->getShippingAmount() && !$previusInvoice->isCanceled()) { + if ($previusInvoice->getShippingAmount() > 0 && !$previusInvoice->isCanceled()) { return $this; } } diff --git a/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php b/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php new file mode 100644 index 0000000000000..f9d119e5b9339 --- /dev/null +++ b/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php @@ -0,0 +1,84 @@ +_invoice = $this->getMock('Mage_Sales_Model_Order_Invoice', array('getOrder'), array(), '', false); + $this->_order = $this->getMock('Mage_Sales_Model_Order', array('getInvoiceCollection'), array(),'',false); + + $this->_invoice->expects($this->any()) + ->method('getOrder') + ->will($this->returnValue($this->_order)); + } + /** + * @covers Mage_Sales_Model_Order_Invoice_Total_Shipping::collect + * @param $collection array + * @param $shippingAmount float + * @dataProvider dataGetValuesAndResults + */ + public function testCollect($collection, $shippingAmount) + { + $temp_collection = array(); + foreach($collection as $tempShippingAmount){ + $temp_invoice = $this->getMock('Mage_Sales_Model_Order_Invoice', null, array(), '', false); + $temp_invoice->setShippingAmount($tempShippingAmount); + $temp_collection[] = $temp_invoice; + } + + $this->_order->expects($this->any()) + ->method('getInvoiceCollection') + ->will($this->returnValue($temp_collection)); + $this->_order->setData('shipping_amount',$shippingAmount); + $this->_order->setData('invoice_collection', $collection); + $total = new Mage_Sales_Model_Order_Invoice_Total_Shipping(); + $total->collect($this->_invoice); + $this->assertEquals($this->_invoice->getShippingAmount(), $this->_order->getShippingAmount()); + + + } + + public static function dataGetValuesAndResults() + { + return array( + array( + array("0.0000") + ,10.00 + ), + array( + array("10.000"), + 0 + ) + ); + } +} \ No newline at end of file From 2aadc5689a61b614bf2960483b7e7a1d8524de7b Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Thu, 30 Aug 2012 10:07:01 -0400 Subject: [PATCH 5/5] Bug - Mage_Sales_Model_Order_Invoice_Total_Shipping:50 The line was if($previusInvoice->getShippingAmount() && ... This would return true if a previous invoice had zero shipping because it was returned as a string from the database. The string "0.0000". The line needs to be changed to have a greaterthan comparison to cause the string to cast to a numeric. Also added unit test. Added comments --- .../Sales/Model/Order/Invoice/Total/ShippingTest.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php b/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php index f9d119e5b9339..fd2269c1fa1b0 100644 --- a/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php +++ b/dev/tests/unit/testsuite/Mage/Sales/Model/Order/Invoice/Total/ShippingTest.php @@ -26,9 +26,16 @@ */ class Mage_Sales_Model_Order_Invoice_Total_ShippingTest extends PHPUnit_Framework_TestCase { - + /** + * Invoice to be passed to the collect method in testCollect + * @var Mage_Sales_Model_Order_Invoice $_invoice + */ protected $_invoice; + /** + * Order to be returned when calling 'getOrder' inside collect method. + * @var Mage_Sales_Model_Order $_order + */ protected $_order;