Fixed issue #CR-1347: duplicate entries in template configuration and…

… incorrect theme being saved in general settings or loaded when a theme does not exist (#3491)

* prevent duplicate entries from being created and delete duplicates

* fix caching for surveys during requests

* add check to getInstance to prevent and invalid template to be loaded

* added the inherited template name to display when selecting the theme in general settings of survey instead of a survey groups default

---------

Co-authored-by: Kevin Foster <kevin.foster.uk@gmail.com>

application/config/version.php

@@ -12,7 +12,7 @@
  */
 
 $config['versionnumber'] = '6.3.4';
-$config['dbversionnumber'] = 616;
+$config['dbversionnumber'] = 617;
 $config['buildnumber'] = '';
 $config['updatable'] = true;
 $config['templateapiversion']  = 3;

application/helpers/update/updates/Update_617.php

@@ -0,0 +1,36 @@
+<?php
+
+namespace LimeSurvey\Helpers\Update;
+
+use CDbException;
+use CException;
+
+class Update_617 extends DatabaseUpdateBase
+{
+    /**
+     * @inheritDoc
+     * @throws CException
+     */
+    public function up(): void
+    {
+        $this->deleteDuplicateTemplateConfigurationEntries();
+    }
+
+    /**
+     * @throws CDbException
+     * @throws CException
+     */
+    private function deleteDuplicateTemplateConfigurationEntries(): void
+    {
+        $aKeepIds = $this->db->createCommand()
+            ->select("MAX(id) AS maxRecordId")
+            ->from("{{template_configuration}}")
+            ->group(['template_name', 'sid', 'gsid', 'uid'])
+            ->queryAll();
+        $aKeepIds = array_column($aKeepIds, 'maxRecordId');
+        $criteria = $this->db->getCommandBuilder()->createCriteria();
+        $criteria->select = 'id, template_name, sid, gsid, uid';
+        $criteria->addNotInCondition('id', $aKeepIds);
+        $this->db->getCommandBuilder()->createDeleteCommand('{{template_configuration}}', $criteria)->execute();
+    }
+}

application/models/Survey.php

@@ -149,15 +149,7 @@ class Survey extends LSActiveRecord implements PermissionInterface
 {
     use PermissionTrait;
 
-    /**
-     * This is a static cache, it lasts only during the active request. If you ever need
-     * to clear it, like on activation of a survey when in the same request a row is read,
-     * saved and read again you can use resetCache() method.
-     *
-     * @var array $findByPkCache
-     */
-    protected $findByPkCache = array();
-
+    protected static array $findByPkCache = [];
 
     // survey options
     public $oOptions;
@@ -338,10 +330,9 @@ public function delete($recursive = true)
         }
 
         // Remove from cache
-        if (array_key_exists($this->sid, $this->findByPkCache)) {
-            unset($this->findByPkCache[$this->sid]);
+        if (array_key_exists($this->sid, self::$findByPkCache)) {
+            unset(self::$findByPkCache[$this->sid]);
         }
-
         return true;
     }
 
@@ -990,12 +981,12 @@ public function findByPk($pk, $condition = '', $params = array())
     {
         /** @var self $model */
         if (empty($condition) && empty($params)) {
-            if (array_key_exists($pk, $this->findByPkCache)) {
-                return $this->findByPkCache[$pk];
+            if (array_key_exists($pk, self::$findByPkCache)) {
+                return self::$findByPkCache[$pk];
             } else {
                 $model = parent::findByPk($pk, $condition, $params);
                 if (!is_null($model)) {
-                    $this->findByPkCache[$pk] = $model;
+                    self::$findByPkCache[$pk] = $model;
                 }
                 return $model;
             }
@@ -1007,9 +998,9 @@ public function findByPk($pk, $condition = '', $params = array())
     /**
      * findByPk uses a cache to store a result. Use this method to force clearing that cache.
      */
-    public function resetCache()
+    public function resetCache(): void
     {
-        $this->findByPkCache = array();
+        self::$findByPkCache = [];
     }
 
     /**

application/models/SurveysGroupsettings.php

@@ -102,12 +102,12 @@ public function rules()
             array('expires, startdate, datecreated, attributedescriptions, emailresponseto, emailnotificationto', 'safe'),
             // The following rule is used by search().
             // @todo Please remove those attributes that should not be searched.
-            array('gsid, owner_id, admin, expires, startdate, adminemail, anonymized, format, 
-			savetimings, template, datestamp, usecookie, allowregister, allowsave, autonumber_start, 
-			autoredirect, allowprev, printanswers, ipaddr, refurl, datecreated, showsurveypolicynotice, 
-			publicstatistics, publicgraphs, listpublic, htmlemail, sendconfirmation, tokenanswerspersistence, 
-			assessments, usecaptcha, bounce_email, attributedescriptions, emailresponseto, emailnotificationto, 
-			tokenlength, showxquestions, showgroupinfo, shownoanswer, showqnumcode, showwelcome, showprogress, 
+            array('gsid, owner_id, admin, expires, startdate, adminemail, anonymized, format,
+			savetimings, template, datestamp, usecookie, allowregister, allowsave, autonumber_start,
+			autoredirect, allowprev, printanswers, ipaddr, refurl, datecreated, showsurveypolicynotice,
+			publicstatistics, publicgraphs, listpublic, htmlemail, sendconfirmation, tokenanswerspersistence,
+			assessments, usecaptcha, bounce_email, attributedescriptions, emailresponseto, emailnotificationto,
+			tokenlength, showxquestions, showgroupinfo, shownoanswer, showqnumcode, showwelcome, showprogress,
 			questionindex, navigationdelay, nokeyboard, alloweditaftercompletion', 'safe', 'on' => 'search'),
         );
     }
@@ -329,7 +329,7 @@ public static function getInstance($iSurveyGroupId = 0, $oSurvey = null, $instan
                 $instance->showInherited = 1;
             }
 
-            // set instance options from survey model, used for frontend redering
+            // set instance options from survey model, used for frontend rendering
             if (($oSurvey !== null && $bRealValues)) {
                 foreach ($instance->optionAttributes as $key => $attribute) {
                     $instance->oOptions->{$attribute} = $oSurvey->$attribute;
@@ -356,6 +356,26 @@ public static function getInstance($iSurveyGroupId = 0, $oSurvey = null, $instan
             }
         }
 
+        // check if the template actually exists and modify it if invalid
+        if (
+            !$instance->shouldInherit('template')
+            && !Template::checkIfTemplateExists($instance->oOptions->template)
+        ) {
+            if ($iSurveyGroupId === 0) {
+                $instance->oOptions->template = App()->getConfig('defaulttheme');
+            } else {
+                $instance->oOptions->template = 'inherit';
+            }
+        }
+
+        // check the global configuration for template inheritance if surveygroup is 0 (global survey) and template set to inherit
+        if (
+            $iSurveyGroupId === 0
+            && $instance->shouldInherit('template')
+        ) {
+            $instance->oOptions->template = App()->getConfig('defaulttheme');
+        }
+
         // fetch parent instance only if parent_id exists
         if ($iSurveyGroupId > 0 && !empty($model->SurveysGroups) && $model->SurveysGroups->parent_id !== null) {
             self::getInstance($model->SurveysGroups->parent_id, null, $instance, $iStep + 1);

application/models/TemplateConfiguration.php

@@ -67,7 +67,7 @@ class TemplateConfiguration extends TemplateConfig
     /** @var array $aInstancesFromTemplateName cache for method getInstanceFromTemplateName*/
     public static $aInstancesFromTemplateName;
 
-    /** @var array $aInstancesFromTemplateName cache for method prepareTemplateRendering*/
+    /** @var array $aPreparedToRender cache for method prepareTemplateRendering*/
     public static $aPreparedToRender;
 
     /** @var boolean $bTemplateCheckResult is the template valid?*/
@@ -275,9 +275,8 @@ public static function getInstanceFromSurveyId($iSurveyId, $sTemplateName = null
 
         $oTemplateConfigurationModel = TemplateConfiguration::model()->find($criteria);
 
-        // No specific template configuration for this surveygroup => create one
-        // TODO: Move to SurveyGroup creation, right now the 'lazy loading' approach is ok.
-        if (!is_a($oTemplateConfigurationModel, 'TemplateConfiguration') && $sTemplateName != null) {
+        // If the TemplateConfiguration could not be found go up the inheritance hierarchy
+        if (empty($oTemplateConfigurationModel)) {
             $oTemplateConfigurationModel = TemplateConfiguration::getInstanceFromTemplateName(
                 $sTemplateName,
                 $abstractInstance
@@ -340,54 +339,23 @@ public static function getThemeOptionsFromSurveyId($iSurveyId = 0, $bInherited =
     /**
      * For a given survey, it checks if its theme have a all the needed configuration entries (survey + survey group).
      * Else, it will create it.
-     * @TODO: recursivity for survey group
      * @param int $iSurveyId
      * @return TemplateConfiguration the template configuration for the survey group
      */
     public static function checkAndcreateSurveyConfig($iSurveyId)
     {
-        //if a template name is given also check against that
         $oSurvey = Survey::model()->findByPk($iSurveyId);
-        $sTemplateName  = $oSurvey->oOptions->template;
         $iSurveyGroupId = $oSurvey->gsid;
+        // set real value from inheritance
+        $sTemplateName = $oSurvey->oOptions->template;
 
-        $criteria = new CDbCriteria();
-        $criteria->addCondition('sid=:sid');
-        $criteria->addCondition('template_name=:template_name');
-        $criteria->params = array('sid' => $iSurveyId, 'template_name' => $sTemplateName);
-
-        $oTemplateConfigurationModel = TemplateConfiguration::model()->find($criteria);
-
-        // TODO: Move to SurveyGroup creation, right now the 'lazy loading' approach is ok.
-        if (!is_a($oTemplateConfigurationModel, 'TemplateConfiguration') && $sTemplateName != null) {
-            $oTemplateConfigurationModel = TemplateConfiguration::getInstanceFromTemplateName($sTemplateName);
-            $oTemplateConfigurationModel->bUseMagicInherit = false;
-            $oTemplateConfigurationModel->id = null;
-            $oTemplateConfigurationModel->isNewRecord = true;
-            $oTemplateConfigurationModel->gsid = null;
-            $oTemplateConfigurationModel->sid = $iSurveyId;
-            $oTemplateConfigurationModel->setToInherit();
-            $oTemplateConfigurationModel->save();
-        }
-
-        $criteria = new CDbCriteria();
-        $criteria->addCondition('gsid=:gsid');
-        $criteria->addCondition('template_name=:template_name');
-        $criteria->params = array('gsid' => $iSurveyGroupId, 'template_name' => $sTemplateName);
-        $oTemplateConfigurationModel = TemplateConfiguration::model()->find($criteria);
+        // load or create a new entry if none is found based on $iSurveyId
+        self::getInstanceFromSurveyId($iSurveyId, $sTemplateName);
 
-        if (!is_a($oTemplateConfigurationModel, 'TemplateConfiguration') && $sTemplateName != null) {
-            $oTemplateConfigurationModel = TemplateConfiguration::getInstanceFromTemplateName($sTemplateName);
-            $oTemplateConfigurationModel->bUseMagicInherit = false;
-            $oTemplateConfigurationModel->id = null;
-            $oTemplateConfigurationModel->isNewRecord = true;
-            $oTemplateConfigurationModel->sid = null;
-            $oTemplateConfigurationModel->gsid = $iSurveyGroupId;
-            $oTemplateConfigurationModel->setToInherit();
-            $oTemplateConfigurationModel->save();
-        }
+        // load or create a new entry if none is found based on $iSurveyGroupId
+        $oGroupTemplateConfigurationModel = self::getInstanceFromSurveyGroup($iSurveyGroupId, $sTemplateName);
 
-        return $oTemplateConfigurationModel;
+        return $oGroupTemplateConfigurationModel;
     }
 
     /**
@@ -645,7 +613,7 @@ public function checkTemplate()
     }
 
     /**
-     * @todo document me
+     * Sets bUseMagicInherit sTemplate isStandard and path of the theme
      *
      * @param string $sTemplateName
      * @param string $iSurveyId

application/views/admin/survey/subview/accordion/_generaloptions_panel.php

@@ -317,10 +317,7 @@ function checkSelect2Languages(mylangs) {
         </div>
         <?php
             $themeConf = TemplateConfiguration::getInstanceFromTemplateName(($oSurvey->template === 'inherit') ? $oSurveyOptions->template : $oSurvey->template);
-            $inheritThemeName = $oSurveyOptions->template;
-            if ($oSurveyOptions->template == 'inherit') {
-                $inheritThemeName = Yii::app()->getConfig('defaulttheme');
-            }
+            $inheritedThemeName = $oSurvey->oOptions->template;
         ?>
         <!-- Theme -->
         <div class="mb-3" >
@@ -330,7 +327,7 @@ function checkSelect2Languages(mylangs) {
                         data-inherit-template-name='<?= $themeConf->template_name ?>'>
                     <?php if ($bShowInherited || $bGlobalSettings) : ?>
                         <option value="inherit" <?= ($oSurvey->template == 'inherit') ? 'selected="selected"' : ''; ?>>
-                            <?= gT('Inherit') . ' [' . CHtml::encode($inheritThemeName) . ']' ?>
+                            <?= gT('Inherit') . ' [' . CHtml::encode($inheritedThemeName) . ']' ?>
                         </option>
                     <?php endif; ?>
                     <?php

tests/TestBaseClass.php

@@ -4,6 +4,7 @@
 
 use PHPUnit\Framework\TestCase;
 use Exception;
+use Survey;
 
 class TestBaseClass extends TestCase
 {
@@ -85,7 +86,7 @@ protected static function importSurvey($fileName, $asuser = 1)
         // Reset the cache to prevent import from failing if there is a cached survey and it's active.
         // When importing, activating, deleting and importing again (usual with automated tests),
         // as using the same SID, it was picking up the cached (old) version of the survey
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
         $translateLinksFields = false;
         $newSurveyName = null;
         $result = \importSurveyFile(
@@ -98,7 +99,8 @@ protected static function importSurvey($fileName, $asuser = 1)
             if (!empty($result['error'])) {
                 throw new Exception(sprintf('Could not import survey %s: %s', $fileName, $result['error']));
             }
-            \Survey::model()->resetCache(); // Reset the cache so findByPk doesn't return a previously cached survey
+            // Reset the cache so findByPk doesn't return a previously cached survey
+            Survey::model()->resetCache();
             self::$testSurvey = \Survey::model()->findByPk($result['newsid']);
             self::$surveyId = $result['newsid'];
         } else {

tests/TestHelper.php

@@ -2,6 +2,7 @@
 
 namespace ls\tests;
 
+use Survey;
 use Yii;
 use Exception;
 use Facebook\WebDriver\Exception\WebDriverException;
@@ -140,7 +141,8 @@ public function activateSurvey($surveyId)
         $survey->refurl = '';
         $survey->savetimings = '';
         $survey->save();
-        \Survey::model()->resetCache();  // Make sure the saved values will be picked up
+        // Make sure the saved values will be picked up
+        Survey::model()->resetCache();
 
         $surveyActivator = new SurveyActivator($survey);
         $result = $surveyActivator->activate();

tests/functional/frontend/ShortUrlTest.php

@@ -3,6 +3,7 @@
 namespace ls\tests;
 
 use Facebook\WebDriver\WebDriverBy;
+use Survey;
 
 /**
  * @group shorturl
@@ -58,7 +59,7 @@ public function testShortUrlOnOpenSurvey($alias, $params, $welcomeText, $firstQu
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 
     /**
@@ -107,7 +108,7 @@ public function testShortUrlOnOpenSurveyWithPrefill($alias, $params)
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 
     /**
@@ -158,7 +159,7 @@ public function testShortUrlOnClosedSurvey($alias, $params, $welcomeText, $first
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 
     /**
@@ -208,7 +209,7 @@ public function testShortUrlOnClosedSurveyWithPrefill($alias, $params)
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 
     /**

tests/unit/helpers/ParticipantBlacklistHandlerTest.php

@@ -4,6 +4,7 @@
 
 use Participant;
 use LimeSurvey\Models\Services\ParticipantBlacklistHandler;
+use Survey;
 
 /**
  * Tests for the ParticipantBlacklistHandler service class.
@@ -39,7 +40,7 @@ public function testAddToBlacklist()
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 
     /**
@@ -74,6 +75,6 @@ public function testRemoveFromBlacklist()
         // Cleanup
         self::$testSurvey->delete();
         self::$testSurvey = null;
-        \Survey::model()->resetCache();
+        Survey::model()->resetCache();
     }
 }