From 0ea97f14e189e1c8399f69428171cfabd9f37a79 Mon Sep 17 00:00:00 2001
From: Mark Baker <mark@lange.demon.co.uk>
Date: Wed, 10 Jul 2019 21:22:16 +0200
Subject: [PATCH] Fixes to coupon functions (#1068)

* New Unit Tests for COUPNUM()

* COUPNUM should not return zero when settlement is in the last period

* Additional tests and fixes for COUPNCD() and COUPPCD() functions
---
 CHANGELOG.md                                  |  3 +
 src/PhpSpreadsheet/Calculation/Financial.php  | 16 ++--
 .../Calculation/FinancialTest.php             | 74 +++++++++----------
 tests/data/Calculation/Financial/COUPNCD.php  | 28 +++++++
 tests/data/Calculation/Financial/COUPNUM.php  | 28 +++++++
 tests/data/Calculation/Financial/COUPPCD.php  | 28 +++++++
 6 files changed, 130 insertions(+), 47 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cc558ee1..f15a894f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 
 ## [Unreleased]
 
+### Fixed
+
+- COUPNUM should not return zero when settlement is in the last period - [Issue #1020](https://github.com/PHPOffice/PhpSpreadsheet/issues/1020) and [PR #1021](https://github.com/PHPOffice/PhpSpreadsheet/pull/1021)
 
 ## [1.8.2] - 2019-07-08
 
diff --git a/src/PhpSpreadsheet/Calculation/Financial.php b/src/PhpSpreadsheet/Calculation/Financial.php
index 3cb6d40a..12dfa7a7 100644
--- a/src/PhpSpreadsheet/Calculation/Financial.php
+++ b/src/PhpSpreadsheet/Calculation/Financial.php
@@ -610,7 +610,7 @@ class Financial
             return Functions::VALUE();
         }
 
-        if (($settlement > $maturity) ||
+        if (($settlement >= $maturity) ||
             (!self::isValidFrequency($frequency)) ||
             (($basis < 0) || ($basis > 4))) {
             return Functions::NAN();
@@ -667,26 +667,22 @@ class Financial
             return Functions::VALUE();
         }
 
-        if (($settlement > $maturity) ||
+        if (($settlement >= $maturity) ||
             (!self::isValidFrequency($frequency)) ||
             (($basis < 0) || ($basis > 4))) {
             return Functions::NAN();
         }
 
-        $settlement = self::couponFirstPeriodDate($settlement, $maturity, $frequency, true);
-        $daysBetweenSettlementAndMaturity = DateTime::YEARFRAC($settlement, $maturity, $basis) * 365;
+        $daysPerYear = self::daysPerYear(DateTime::YEAR($settlement), $basis);
+        $daysBetweenSettlementAndMaturity = DateTime::YEARFRAC($settlement, $maturity, $basis) * $daysPerYear;
 
         switch ($frequency) {
             case 1: // annual payments
-                return ceil($daysBetweenSettlementAndMaturity / 360);
             case 2: // half-yearly
-                return ceil($daysBetweenSettlementAndMaturity / 180);
             case 4: // quarterly
-                return ceil($daysBetweenSettlementAndMaturity / 90);
             case 6: // bimonthly
-                return ceil($daysBetweenSettlementAndMaturity / 60);
             case 12: // monthly
-                return ceil($daysBetweenSettlementAndMaturity / 30);
+                return ceil($daysBetweenSettlementAndMaturity / $daysPerYear * $frequency);
         }
 
         return Functions::VALUE();
@@ -740,7 +736,7 @@ class Financial
             return Functions::VALUE();
         }
 
-        if (($settlement > $maturity) ||
+        if (($settlement >= $maturity) ||
             (!self::isValidFrequency($frequency)) ||
             (($basis < 0) || ($basis > 4))) {
             return Functions::NAN();
diff --git a/tests/PhpSpreadsheetTests/Calculation/FinancialTest.php b/tests/PhpSpreadsheetTests/Calculation/FinancialTest.php
index d9103896..2d8d770a 100644
--- a/tests/PhpSpreadsheetTests/Calculation/FinancialTest.php
+++ b/tests/PhpSpreadsheetTests/Calculation/FinancialTest.php
@@ -21,7 +21,7 @@ class FinancialTest extends TestCase
     public function testACCRINT($expectedResult, ...$args)
     {
         $result = Financial::ACCRINT(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerACCRINT()
@@ -37,7 +37,7 @@ class FinancialTest extends TestCase
     public function testACCRINTM($expectedResult, ...$args)
     {
         $result = Financial::ACCRINTM(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerACCRINTM()
@@ -53,7 +53,7 @@ class FinancialTest extends TestCase
     public function testAMORDEGRC($expectedResult, ...$args)
     {
         $result = Financial::AMORDEGRC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerAMORDEGRC()
@@ -69,7 +69,7 @@ class FinancialTest extends TestCase
     public function testAMORLINC($expectedResult, ...$args)
     {
         $result = Financial::AMORLINC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerAMORLINC()
@@ -85,7 +85,7 @@ class FinancialTest extends TestCase
     public function testCOUPDAYBS($expectedResult, ...$args)
     {
         $result = Financial::COUPDAYBS(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPDAYBS()
@@ -101,7 +101,7 @@ class FinancialTest extends TestCase
     public function testCOUPDAYS($expectedResult, ...$args)
     {
         $result = Financial::COUPDAYS(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPDAYS()
@@ -117,7 +117,7 @@ class FinancialTest extends TestCase
     public function testCOUPDAYSNC($expectedResult, ...$args)
     {
         $result = Financial::COUPDAYSNC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPDAYSNC()
@@ -133,7 +133,7 @@ class FinancialTest extends TestCase
     public function testCOUPNCD($expectedResult, ...$args)
     {
         $result = Financial::COUPNCD(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPNCD()
@@ -149,7 +149,7 @@ class FinancialTest extends TestCase
     public function testCOUPNUM($expectedResult, ...$args)
     {
         $result = Financial::COUPNUM(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPNUM()
@@ -165,7 +165,7 @@ class FinancialTest extends TestCase
     public function testCOUPPCD($expectedResult, ...$args)
     {
         $result = Financial::COUPPCD(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCOUPPCD()
@@ -181,7 +181,7 @@ class FinancialTest extends TestCase
     public function testCUMIPMT($expectedResult, ...$args)
     {
         $result = Financial::CUMIPMT(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCUMIPMT()
@@ -197,7 +197,7 @@ class FinancialTest extends TestCase
     public function testCUMPRINC($expectedResult, ...$args)
     {
         $result = Financial::CUMPRINC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerCUMPRINC()
@@ -213,7 +213,7 @@ class FinancialTest extends TestCase
     public function testDB($expectedResult, ...$args)
     {
         $result = Financial::DB(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerDB()
@@ -229,7 +229,7 @@ class FinancialTest extends TestCase
     public function testDDB($expectedResult, ...$args)
     {
         $result = Financial::DDB(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerDDB()
@@ -245,7 +245,7 @@ class FinancialTest extends TestCase
     public function testDISC($expectedResult, ...$args)
     {
         $result = Financial::DISC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerDISC()
@@ -261,7 +261,7 @@ class FinancialTest extends TestCase
     public function testDOLLARDE($expectedResult, ...$args)
     {
         $result = Financial::DOLLARDE(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerDOLLARDE()
@@ -277,7 +277,7 @@ class FinancialTest extends TestCase
     public function testDOLLARFR($expectedResult, ...$args)
     {
         $result = Financial::DOLLARFR(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerDOLLARFR()
@@ -293,7 +293,7 @@ class FinancialTest extends TestCase
     public function testEFFECT($expectedResult, ...$args)
     {
         $result = Financial::EFFECT(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerEFFECT()
@@ -309,7 +309,7 @@ class FinancialTest extends TestCase
     public function testFV($expectedResult, ...$args)
     {
         $result = Financial::FV(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerFV()
@@ -325,7 +325,7 @@ class FinancialTest extends TestCase
     public function testFVSCHEDULE($expectedResult, ...$args)
     {
         $result = Financial::FVSCHEDULE(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerFVSCHEDULE()
@@ -341,7 +341,7 @@ class FinancialTest extends TestCase
     public function testINTRATE($expectedResult, ...$args)
     {
         $result = Financial::INTRATE(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerINTRATE()
@@ -357,7 +357,7 @@ class FinancialTest extends TestCase
     public function testIPMT($expectedResult, ...$args)
     {
         $result = Financial::IPMT(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerIPMT()
@@ -373,7 +373,7 @@ class FinancialTest extends TestCase
     public function testIRR($expectedResult, ...$args)
     {
         $result = Financial::IRR(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerIRR()
@@ -389,7 +389,7 @@ class FinancialTest extends TestCase
     public function testISPMT($expectedResult, ...$args)
     {
         $result = Financial::ISPMT(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerISPMT()
@@ -405,7 +405,7 @@ class FinancialTest extends TestCase
     public function testMIRR($expectedResult, ...$args)
     {
         $result = Financial::MIRR(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerMIRR()
@@ -421,7 +421,7 @@ class FinancialTest extends TestCase
     public function testNOMINAL($expectedResult, ...$args)
     {
         $result = Financial::NOMINAL(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerNOMINAL()
@@ -437,7 +437,7 @@ class FinancialTest extends TestCase
     public function testNPER($expectedResult, ...$args)
     {
         $result = Financial::NPER(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerNPER()
@@ -453,7 +453,7 @@ class FinancialTest extends TestCase
     public function testNPV($expectedResult, ...$args)
     {
         $result = Financial::NPV(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerNPV()
@@ -471,7 +471,7 @@ class FinancialTest extends TestCase
         $this->markTestIncomplete('TODO: This test should be fixed');
 
         $result = Financial::PRICE(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerPRICE()
@@ -487,7 +487,7 @@ class FinancialTest extends TestCase
     public function testPRICEDISC($expectedResult, array $args)
     {
         $result = Financial::PRICEDISC(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerPRICEDISC()
@@ -503,7 +503,7 @@ class FinancialTest extends TestCase
     public function testPV($expectedResult, array $args)
     {
         $result = Financial::PV(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerPV()
@@ -521,7 +521,7 @@ class FinancialTest extends TestCase
         $this->markTestIncomplete('TODO: This test should be fixed');
 
         $result = Financial::RATE(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerRATE()
@@ -539,7 +539,7 @@ class FinancialTest extends TestCase
         $this->markTestIncomplete('TODO: This test should be fixed');
 
         $result = Financial::XIRR(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerXIRR()
@@ -555,7 +555,7 @@ class FinancialTest extends TestCase
     public function testPDURATION($expectedResult, array $args)
     {
         $result = Financial::PDURATION(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerPDURATION()
@@ -571,7 +571,7 @@ class FinancialTest extends TestCase
     public function testRRI($expectedResult, array $args)
     {
         $result = Financial::RRI(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerRRI()
@@ -587,7 +587,7 @@ class FinancialTest extends TestCase
     public function testSLN($expectedResult, array $args)
     {
         $result = Financial::SLN(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerSLN()
@@ -603,7 +603,7 @@ class FinancialTest extends TestCase
     public function testSYD($expectedResult, array $args)
     {
         $result = Financial::SYD(...$args);
-        self::assertEquals($expectedResult, $result, null, 1E-8);
+        self::assertEquals($expectedResult, $result, '', 1E-8);
     }
 
     public function providerSYD()
diff --git a/tests/data/Calculation/Financial/COUPNCD.php b/tests/data/Calculation/Financial/COUPNCD.php
index b0701d74..eea6ad13 100644
--- a/tests/data/Calculation/Financial/COUPNCD.php
+++ b/tests/data/Calculation/Financial/COUPNCD.php
@@ -37,4 +37,32 @@ return [
         3,
         1,
     ],
+    [
+        '#NUM!',
+        '24-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36884,
+        '23-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36884,
+        '24-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36793,
+        '23-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
 ];
diff --git a/tests/data/Calculation/Financial/COUPNUM.php b/tests/data/Calculation/Financial/COUPNUM.php
index a81b139e..683fc317 100644
--- a/tests/data/Calculation/Financial/COUPNUM.php
+++ b/tests/data/Calculation/Financial/COUPNUM.php
@@ -45,4 +45,32 @@ return [
         1,
         1,
     ],
+    [
+        '#NUM!',
+        '24-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        1,
+        '23-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        1,
+        '24-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        2,
+        '23-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
 ];
diff --git a/tests/data/Calculation/Financial/COUPPCD.php b/tests/data/Calculation/Financial/COUPPCD.php
index a6c14d97..911637d6 100644
--- a/tests/data/Calculation/Financial/COUPPCD.php
+++ b/tests/data/Calculation/Financial/COUPPCD.php
@@ -37,4 +37,32 @@ return [
         3,
         1,
     ],
+    [
+        '#NUM!',
+        '24-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36793,
+        '23-Dec-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36793,
+        '24-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
+    [
+        36701,
+        '23-Sep-2000',
+        '24-Dec-2000',
+        4,
+        0,
+    ],
 ];