dev/core#1558 Data conversion for non-standard setting.

Per https://lab.civicrm.org/dev/core/issues/1558 the 'contribution_invoice_settings' 'setting' does not follow our standard of one setting per key and this has led to all sorts of hacks in various places. In trying to unravel this I eventually concluded we needed an interim step where the data would be stored as the correct settings but set and get would still work on the deprecated settinng. The reason for goinng this way is that I found more than one place where extension code is accessing these settings and the amount of work to fix up all the places in core that access it and the settings form were too big to deal with without a stepping stone. With this merged the settinngs are correctly defined and a definition for the deprecated setting still exists. Requesting the deprecated setting returns an array retrieved from the correct settings so is unchanged from calling functions point of view. Saving the deprecated setting saves to the correct places. However, it's now also possible to get & set the correct settings and once the core places are removed we will deprecate & remove the deprecated setting. Unit tests cover past & desired behaviour and the upgrade routine.
civicrm · Jan 30, 2020 · ff6f993 · ff6f993
1 parent 41de5d4
commit ff6f993
Show file tree

Hide file tree

Showing 10 changed files with 268 additions and 21 deletions.
diff --git a/CRM/Core/SelectValues.php b/CRM/Core/SelectValues.php
@@ -443,6 +443,27 @@ public static function geoProvider() {
     return $geo;
   }
 
+  /**
+   * Get options for displaying tax.
+   *
+   * @return array
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function taxDisplayOptions() {
+    return [
+      'Do_not_show' => ts('Do not show breakdown, only show total - i.e %1', [
+        1 => CRM_Utils_Money::format(120),
+      ]),
+      'Inclusive' => ts('Show [tax term] inclusive price - i.e. %1', [
+        1 => ts('%1 (includes [tax term] of %2)', [1 => CRM_Utils_Money::format(120), 2 => CRM_Utils_Money::format(20)]),
+      ]),
+      'Exclusive' => ts('Show [tax term] exclusive price - i.e. %1', [
+        1 => ts('%1 + %2 [tax term]', [1 => CRM_Utils_Money::format(120), 2 => CRM_Utils_Money::format(20)]),
+      ]),
+    ];
+  }
+
   /**
    * Get the Address Standardization Providers from available plugins.
    *

diff --git a/CRM/Invoicing/Utils.php b/CRM/Invoicing/Utils.php
@@ -24,6 +24,8 @@ class CRM_Invoicing_Utils {
    * @param bool $oldValue
    * @param bool $newValue
    * @param array $metadata
+   *
+   * @throws \CiviCRM_API3_Exception
    */
   public static function onToggle($oldValue, $newValue, $metadata) {
     if ($oldValue == $newValue) {
@@ -32,7 +34,7 @@ public static function onToggle($oldValue, $newValue, $metadata) {
     $existingUserViewOptions = civicrm_api3('Setting', 'get', ['return' => 'user_dashboard_options'])['values'][CRM_Core_Config::domainID()]['user_dashboard_options'];
     $optionValues = civicrm_api3('Setting', 'getoptions', ['field' => 'user_dashboard_options'])['values'];
     $invoiceKey = array_search('Invoices / Credit Notes', $optionValues);
-    $existingIndex = in_array($invoiceKey, $existingUserViewOptions);
+    $existingIndex = array_search($invoiceKey, $existingUserViewOptions);
 
     if ($newValue && $existingIndex === FALSE) {
       $existingUserViewOptions[] = $invoiceKey;

diff --git a/CRM/Upgrade/Incremental/Base.php b/CRM/Upgrade/Incremental/Base.php
@@ -9,6 +9,8 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Core\SettingsBag;
+
 /**
  * Base class for incremental upgrades
  */
@@ -195,11 +197,16 @@ public static function updateMessageTemplates($ctx, $version) {
    * @return bool
    */
   public static function updateContributeSettings($ctx) {
-    $settings = Civi::settings()->get('contribution_invoice_settings');
-    $metadata = \Civi\Core\SettingsMetadata::getMetadata();
-    $conversions = array_intersect_key((array) $settings, $metadata);
-    foreach ($conversions as $key => $conversion) {
-      Civi::settings()->set($key, $conversion);
+    // Use a direct query as api now does some handling on this.
+    $settings = CRM_Core_DAO::executeQuery("SELECT value, domain_id FROM civicrm_setting WHERE name = 'contribution_invoice_settings'");
+
+    while ($settings->fetch()) {
+      $contributionSettings = (array) CRM_Utils_String::unserialize($settings->value);
+      foreach (array_merge(SettingsBag::getContributionInvoiceSettingKeys(), ['deferred_revenue_enabled' => 'deferred_revenue_enabled']) as $possibleKeyName => $settingName) {
+        if (!empty($contributionSettings[$possibleKeyName]) && empty(Civi::settings($settings->domain_id)->getExplicit($settingName))) {
+          Civi::settings($settings->domain_id)->set($settingName, $contributionSettings[$possibleKeyName]);
+        }
+      }
     }
     return TRUE;
   }

diff --git a/Civi/Core/SettingsBag.php b/Civi/Core/SettingsBag.php
@@ -136,6 +136,7 @@ public function loadValues() {
       while ($dao->fetch()) {
         $this->values[$dao->name] = ($dao->value !== NULL) ? \CRM_Utils_String::unserialize($dao->value) : NULL;
       }
+      $dao->values['contribution_invoice_settings'] = $this->getContributionSettings();
     }
 
     return $this;
@@ -253,12 +254,51 @@ public function revert($key) {
    * @return SettingsBag
    */
   public function set($key, $value) {
+    if ($key === 'contribution_invoice_settings') {
+      $this->setContributionSettings($value);
+      return $this;
+    }
     $this->setDb($key, $value);
     $this->values[$key] = $value;
     $this->combined = NULL;
     return $this;
   }
 
+  /**
+   * Temporary handling for phasing out contribution_invoice_settings.
+   *
+   * Until we have transitioned we need to handle setting & retrieving
+   * contribution_invoice_settings.
+   *
+   * Once removed from core we will add deprecation notices & then remove this.
+   *
+   * https://lab.civicrm.org/dev/core/issues/1558
+   *
+   * @param array $value
+   */
+  public function setContributionSettings($value) {
+    foreach (SettingsBag::getContributionInvoiceSettingKeys() as $possibleKeyName => $settingName) {
+      $keyValue = $value[$possibleKeyName] ?? '';
+      $this->set($settingName, $keyValue);
+    }
+    $this->values['contribution_invoice_settings'] = $this->getContributionSettings();
+  }
+
+  /**
+   * Temporary function to handle returning the contribution_settings key despite it being deprecated.
+   *
+   * See more in comment block on previous function.
+   *
+   * @return array
+   */
+  public function getContributionSettings() {
+    $contributionSettings = [];
+    foreach (SettingsBag::getContributionInvoiceSettingKeys() as $keyName => $settingName) {
+      $contributionSettings[$keyName] = $this->values[$settingName] ?? '';
+    }
+    return $contributionSettings;
+  }
+
   /**
    * @return \CRM_Utils_SQL_Select
    */
@@ -378,4 +418,22 @@ protected function setDb($name, $value) {
     }
   }
 
+  /**
+   * @return array
+   */
+  public static function getContributionInvoiceSettingKeys(): array {
+    $convertedKeys = [
+      'credit_notes_prefix' => 'credit_notes_prefix',
+      'invoice_prefix' => 'invoice_prefix',
+      'due_date' => 'invoice_due_date',
+      'due_date_period' => 'invoice_due_date_period',
+      'notes' => 'invoice_notes',
+      'is_email_pdf'  => 'invoice_is_email_pdf',
+      'tax_term' => 'tax_term',
+      'tax_display_settings' => 'tax_display_settings',
+      'invoicing' => 'invoicing',
+    ];
+    return $convertedKeys;
+  }
+
 }
diff --git a/settings/Contribute.setting.php b/settings/Contribute.setting.php
@@ -50,7 +50,7 @@
       'tax_display_settings' => 'Inclusive',
     ],
     'add' => '4.7',
-    'title' => ts('Contribution Invoice Settings'),
+    'title' => ts('Deprecated setting'),
     'is_domain' => 1,
     'is_contact' => 0,
     'help_text' => NULL,
@@ -71,6 +71,97 @@
       'CRM_Invoicing_Utils::onToggle',
     ],
   ],
+  'credit_notes_prefix' => [
+    'group_name' => 'Contribute Preferences',
+    'group' => 'contribute',
+    'name' => 'credit_notes_prefix',
+    'html_type' => 'text',
+    'quick_form_type' => 'Element',
+    'add' => '5.23',
+    'type' => CRM_Utils_Type::T_STRING,
+    'title' => ts('Credit Notes Prefix'),
+    'is_domain' => 1,
+    'is_contact' => 0,
+    'description' => ts('Prefix to be prepended to credit note ids'),
+    'default' => 'CN_',
+    'help_text' => ts('The credit note ID is generated when a contribution is set to Refunded, Cancelled or Chargeback. It is visible on invoices, if invoices are enabled'),
+  ],
+  'invoice_prefix' => [
+    'html_type' => 'text',
+    'name' => 'invoice_prefix',
+    'add' => '5.23',
+    'type' => CRM_Utils_Type::T_STRING,
+    'title' => ts('Invoice Prefix'),
+    'description' => ts('Enter prefix to be be preprended when creating an invoice number'),
+    'is_domain' => 1,
+    'is_contact' => 0,
+  ],
+  'invoice_due_date' => [
+    'name' => 'invoice_due_date',
+    'html_type' => 'text',
+    'title' => ts('Due Date'),
+    'add' => '5.23',
+    'type' => CRM_Utils_Type::T_INT,
+    'is_domain' => 1,
+    'is_contact' => 0,
+  ],
+  'invoice_due_date_period' => [
+    'html_type' => 'select',
+    'name' => 'invoice_due_date_period',
+    'title' => ts('For transmission'),
+    'weight' => 4,
+    'add' => '5.23',
+    'type' => CRM_Utils_Type::T_STRING,
+    'is_domain' => 1,
+    'is_contact' => 0,
+    'description' => ts('Select the interval for due date.'),
+    'options' => [
+      'select' => ts('- select -'),
+      'days' => ts('Days'),
+      'months' => ts('Months'),
+      'years' => ts('Years'),
+    ],
+  ],
+  'invoice_notes' => [
+    'name' => 'invoice_notes',
+    'html_type' => 'wysiwyg',
+    'title' => ts('Notes or Standard Terms'),
+    'type' => CRM_Utils_Type::T_STRING,
+    'add' => '5.23',
+    'is_domain' => 1,
+    'is_contact' => 0,
+    'description' => ts('Enter note or message to be displayed on PDF invoice or credit notes '),
+    'attributes' => ['rows' => 2, 'cols' => 40],
+  ],
+  'invoice_is_email_pdf' => [
+    'name' => 'invoice_is_email_pdf',
+    'html_type' => 'checkbox',
+    'add' => '5.23',
+    'type' => CRM_Utils_Type::T_BOOLEAN,
+    'is_domain' => 1,
+    'is_contact' => 0,
+    'title' => ts('Automatically email invoice when user purchases online'),
+    'description' => ts('Should a pdf invoice be emailed automatically?'),
+  ],
+  'tax_term' => [
+    'name' => 'tax_term',
+    'html_type' => 'text',
+    'add' => '5.23',
+    'title' => ts('Tax Term'),
+    'type' => CRM_Utils_Type::T_STRING,
+    'is_domain' => 1,
+    'is_contact' => 0,
+  ],
+  'tax_display_settings' => [
+    'html_type' => 'select',
+    'name' => 'tax_display_settings',
+    'type' => CRM_Utils_Type::T_STRING,
+    'add' => '5.23',
+    'title' => ts('Tax Display Settings'),
+    'is_domain' => 1,
+    'is_contact' => 0,
+    'pseudoconstant' => ['callback' => 'CRM_Core_SelectValues::taxDisplayOptions'],
+  ],
   'acl_financial_type' => [
     'group_name' => 'Contribute Preferences',
     'group' => 'contribute',

diff --git a/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php b/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php
@@ -135,6 +135,9 @@ public function testDashboardContentContributionsWithInvoicingEnabled() {
 
   /**
    * Test the content of the dashboard.
+   *
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public function testDashboardContentContributions() {
     $this->contributionCreate(['contact_id' => $this->contactID]);

diff --git a/tests/phpunit/CRM/Core/BAO/SettingTest.php b/tests/phpunit/CRM/Core/BAO/SettingTest.php
@@ -29,31 +29,77 @@ public function tearDown() {
     parent::tearDown();
   }
 
+  /**
+   * Test that enabling a valid component works.
+   */
   public function testEnableComponentValid() {
-    $config = CRM_Core_Config::singleton(TRUE, TRUE);
-
+    CRM_Core_Config::singleton(TRUE, TRUE);
     $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign');
-
     $this->assertTrue($result);
   }
 
+  /**
+   * Test that we get a success result if we try to enable an enabled component.
+   */
   public function testEnableComponentAlreadyPresent() {
-    $config = CRM_Core_Config::singleton(TRUE, TRUE);
-
-    $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign');
+    CRM_Core_Config::singleton(TRUE, TRUE);
+    CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign');
     $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign');
-
     $this->assertTrue($result);
   }
 
+  /**
+   * Test that we get a false result if we try to enable an invalid component.
+   */
   public function testEnableComponentInvalid() {
-    $config = CRM_Core_Config::singleton(TRUE, TRUE);
-
+    CRM_Core_Config::singleton(TRUE, TRUE);
     $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviFake');
-
     $this->assertFalse($result);
   }
 
+  /**
+   * Test temporary retrieval & setting of converted settings.
+   *
+   * As a transitional measure we allow the settings that were munged into
+   * contribution_invoice_setting. This tests that the current method of getting via the 'old' key
+   * works. This will be deprecated & removed over the next few versions but
+   * 1) some extensions use these settings &
+   * 2) there is a lot of work to fix this mess in core so a transitional method makes sense.
+   *
+   * https://lab.civicrm.org/dev/core/issues/1558
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testHandlingOfContributionInvoiceSetting() {
+    $contributionSettings = [
+      'invoice_prefix' => 'G_',
+      'credit_notes_prefix' => 'XX_',
+      'due_date' => '20',
+      'due_date_period' => 'weeks',
+      'notes' => '<p>Give me money</p>',
+      'tax_term' => 'Extortion',
+      'tax_display_settings' => 'Exclusive',
+      'invoicing' => 1,
+      'is_email_pdf' => '1',
+    ];
+    Civi::settings()->set('contribution_invoice_settings', $contributionSettings);
+    $settingsFromGet = Civi::settings()->get('contribution_invoice_settings');
+    $settingsFromAPI = $this->callAPISuccess('Setting', 'get', ['return' => 'contribution_invoice_settings'])['values'][CRM_Core_Config::domainID()]['contribution_invoice_settings'];
+    $getVersion = $this->callAPISuccessGetValue('Setting', ['name' => 'contribution_invoice_settings']);
+    $this->assertEquals($settingsFromAPI, $settingsFromGet);
+    $this->assertAPIArrayComparison($getVersion, $settingsFromGet);
+    $this->assertEquals($contributionSettings, $settingsFromGet);
+
+    // These are the preferred retrieval methods.
+    $this->assertEquals('G_', Civi::settings()->get('invoice_prefix'));
+    $this->assertEquals('XX_', Civi::settings()->get('credit_notes_prefix'));
+    $this->assertEquals('20', Civi::settings()->get('invoice_due_date'));
+    $this->assertEquals('weeks', Civi::settings()->get('invoice_due_date_period'));
+    $this->assertEquals('<p>Give me money</p>', Civi::settings()->get('invoice_notes'));
+    $this->assertEquals('Extortion', Civi::settings()->get('tax_term'));
+    $this->assertEquals('Exclusive', Civi::settings()->get('tax_display_settings'));
+  }
+
   /**
    * Ensure that overrides in $civicrm_setting apply when
    * using getItem($group,$name).

diff --git a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php
@@ -57,6 +57,8 @@ public function tearDown() {
    *
    * The scenario is that a pending contribution exists and the IPN call will update it to completed.
    * And also if Tax and Invoicing is enabled, this unit test ensure that invoice pdf is attached with email recipet
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testInvoiceSentOnIPNPaymentSuccess() {
     $this->enableTaxAndInvoicing();
@@ -81,7 +83,7 @@ public function testInvoiceSentOnIPNPaymentSuccess() {
     $_REQUEST = ['q' => CRM_Utils_System::url('civicrm/payment/ipn/' . $this->_paymentProcessorID)] + $this->getPaypalTransaction();
 
     $mut = new CiviMailUtils($this, TRUE);
-    $paymentProcesors = civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $this->_paymentProcessorID]);
+    $paymentProcesors = $this->callAPISuccessGetSingle('PaymentProcessor', ['id' => $this->_paymentProcessorID]);
     $payment = Civi\Payment\System::singleton()->getByProcessor($paymentProcesors);
     $payment->handlePaymentNotification();
 

diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php
@@ -360,7 +360,7 @@ public function testBalanceDueIfDeferredRevenueEnabled() {
     ]);
     $balance = CRM_Contribute_BAO_Contribution::getContributionBalance($contribution['id'], $totalAmount);
     $this->assertEquals(0.0, $balance);
-    Civi::settings()->revert('contribution_invoice_settings');
+    Civi::settings()->set('deferred_revenue_enabled', FALSE);
   }
 
   /**