Skip to content

Commit

Permalink
Fixed issue #17120: After picking a new question theme, the settings …
Browse files Browse the repository at this point in the history
…are not properly updated (#1842)

Code reorganization.
- Added some comments.
- Have created helper functions as to be able to have a better outlook of what some function do.

Solution Summary.
The key of the solution is to be able to pickup theme attributes from a theme name and not from a the attribute set on the question. Then added a theme override parameter which will set which theme attributes should be picked up.

So now, question attributes may contain the attributes of a temp theme, which is not set on the DB.

* Move QuestionAttribute related static methods to a new service class

* Move QuestionAttribute related static methods to a new service class - Missing class

Co-authored-by: encuestabizdevgit <devgit@encuesta.biz>
  • Loading branch information
gabrieljenik and encuestabizdevgit committed Apr 8, 2021
1 parent e48a951 commit fe3a90d
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 72 deletions.
22 changes: 10 additions & 12 deletions application/controllers/QuestionAdministrationController.php
Expand Up @@ -2278,25 +2278,23 @@ function ($oAnswerOption) {
*
* @param int $iQuestionId
* @param string $sQuestionType
* @param string $question_template
* @param string $sQuestionTheme
* @return array
* @throws CException
* @throws Exception
*/
private function getAdvancedOptions($iQuestionId = null, $sQuestionType = null, $question_template = 'core')
private function getAdvancedOptions($iQuestionId = null, $sQuestionType = null, $sQuestionTheme = 'core')
{
//here we get a Question object (also if question is new --> QuestionCreate)

$oQuestion = $this->getQuestionObject($iQuestionId, $sQuestionType);
$advancedSettings = $oQuestion->getAdvancedSettingsWithValuesByCategory(null);
// TODO: Why can empty array be saved as value?
foreach ($advancedSettings as &$category) {
foreach ($category as &$setting) {
if ($setting['value'] === []) {
$setting['value'] = null;
}
}
}

// Get the advanced settings array
$advancedSettings = $oQuestion->getAdvancedSettingsWithValues(null, $sQuestionTheme);

// Group the array in categories
$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$advancedSettings = $questionAttributeHelper->groupAttributesByCategory($advancedSettings);

// This category is "general setting".
unset($advancedSettings['Attribute']);

Expand Down
78 changes: 27 additions & 51 deletions application/models/Question.php
Expand Up @@ -282,67 +282,40 @@ public function updateQuestionOrder($gid, $startingOrder = 1)
* including their values set in the database
*
* @param string|null $sLanguage If you give a language then only the attributes for that language are returned
* @param string|null $sQuestionThemeOverride Name of the question theme to use instead of the question's current theme
* @return array
*/
public function getAdvancedSettingsWithValues($sLanguage = null)
public function getAdvancedSettingsWithValues($sLanguage = null, $sQuestionThemeOverride = null)
{
$oSurvey = $this->survey;
if (empty($oSurvey)) {
throw new Exception('This question has no survey - qid = ' . json_encode($this->qid));
}
if (is_null($sLanguage)) {
$aLanguages = array_merge(
[$oSurvey->language],
$oSurvey->additionalLanguages
);
} else {
$aLanguages = array($sLanguage);
}
$aAttributeValues = QuestionAttribute::model()->getQuestionAttributes($this->qid, $sLanguage);
// TODO: move getQuestionAttributesSettings() to QuestionAttribute model to avoid code duplication

// Get attribute values
$aAttributeValues = QuestionAttribute::getAttributesAsArrayFromDB($this->qid);

// Get question theme name if not specified
$sQuestionTheme = !empty($sQuestionThemeOverride) ? $sQuestionThemeOverride : $aAttributeValues['question_template'][''];

// Get advanced attribute definitions for the question type
$advancedOnly = true;
$aAttributeNames = QuestionAttribute::getQuestionAttributesSettings($this->type, $advancedOnly);

// If the question has a custom template, we first check if it provides custom attributes
$aAttributeNames = self::getQuestionTemplateAttributes($aAttributeNames, $aAttributeValues, $this);

uasort($aAttributeNames, 'categorySort');
foreach ($aAttributeNames as $iKey => $aAttribute) {
if ($aAttribute['i18n'] == false) {
if (isset($aAttributeValues[$aAttribute['name']])) {
$aAttributeNames[$iKey]['value'] = $aAttributeValues[$aAttribute['name']];
} else {
$aAttributeNames[$iKey]['value'] = $aAttribute['default'];
}
} else {
foreach ($aLanguages as $sLanguage) {
if (isset($aAttributeValues[$aAttribute['name']][$sLanguage])) {
$aAttributeNames[$iKey][$sLanguage]['value'] = $aAttributeValues[$aAttribute['name']][$sLanguage];
} else {
$aAttributeNames[$iKey][$sLanguage]['value'] = $aAttribute['default'];
}
}
}
}
$aQuestionTypeAttributes = QuestionAttribute::getQuestionAttributesSettings($this->type, $advancedOnly);

return $aAttributeNames;
}
// Get question theme attribute definitions
$aThemeAttributes = QuestionTheme::getAdditionalAttrFromExtendedTheme($sQuestionTheme, $this->type);

/**
* As getAdvancedSettingsWithValues but with category as array key.
* Used by advanced settings widget.
*
* @param string|null $sLanguage
* @return array
*/
public function getAdvancedSettingsWithValuesByCategory($sLanguage = null)
{
$aAttributeNames = $this->getAdvancedSettingsWithValues($sLanguage);
$aByCategory = [];
foreach ($aAttributeNames as $aAttribute) {
$aByCategory[$aAttribute['category']][] = $aAttribute;
}
return $aByCategory;
// Merge the attributes with the question theme ones
$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$aAttributes = $questionAttributeHelper->mergeQuestionAttributes($aQuestionTypeAttributes, $aThemeAttributes);

uasort($aAttributes, 'categorySort');

// Fill attributes with values
$aLanguages = is_null($sLanguage) ? $oSurvey->allLanguages : [$sLanguage];
$aAttributes = $questionAttributeHelper->fillAttributesWithValues($aAttributes, $aAttributeValues, $aLanguages);

return $aAttributes;
}

/**
Expand All @@ -353,6 +326,9 @@ public function getAdvancedSettingsWithValuesByCategory($sLanguage = null)
* @param array $aAttributeValues $attributeValues['question_template'] != 'core', only if this is true the function changes something
* @param Question $oQuestion this is needed to check if a questionTemplate has custom attributes
* @return mixed returns the incoming parameter $aAttributeNames or
*
* @deprecated use QuestionTheme::getAdditionalAttrFromExtendedTheme() to retrieve question theme attributes and
* QuestionAttributeHelper->mergeQuestionAttributes() to merge with base attributes.
*/
public static function getQuestionTemplateAttributes($aAttributeNames, $aAttributeValues, $oQuestion)
{
Expand Down
23 changes: 14 additions & 9 deletions application/models/QuestionAttribute.php
Expand Up @@ -266,6 +266,8 @@ private static function rewriteQuestionAttributeArray($aAttributeNames, $aAttrib
{
$aQuestionAttributes = array();
foreach ($aAttributeNames as $aAttribute) {
// Initializes expression. This is overwritten if no i18n definition found.
// Is that expected?
$aQuestionAttributes[$aAttribute['name']]['expression'] = isset($aAttribute['expression']) ? $aAttribute['expression'] : 0;

// convert empty array to empty string
Expand Down Expand Up @@ -340,8 +342,10 @@ public static function addAdditionalAttributesFromExtendedTheme($aAttributeNames
['qid' => $oQuestion->qid, 'attribute' => 'question_template']
);
if ($oAttributeValue !== null) {
$aAttributeValueQuestionTemplate['question_template'] = $oAttributeValue->value;
$retAttributeNamesExtended = Question::getQuestionTemplateAttributes($retAttributeNamesExtended, $aAttributeValueQuestionTemplate, $oQuestion);
$sQuestionTheme = $oAttributeValue->value;
$aThemeAttributes = QuestionTheme::getAdditionalAttrFromExtendedTheme($sQuestionTheme, $oQuestion->type);
$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$retAttributeNamesExtended = $questionAttributeHelper->mergeQuestionAttributes($retAttributeNamesExtended, $aThemeAttributes);
}

return $retAttributeNamesExtended;
Expand Down Expand Up @@ -549,13 +553,8 @@ protected static function getAdvancedAttributesFromXml($sXmlFilePath)
}

// Filter all pesky '[]' values (empty values should be null, e.g. <default></default>).
foreach ($aAttributes as $attributeName => $attribute) {
foreach ($attribute as $fieldName => $value) {
if ($value === []) {
$aAttributes[$attributeName][$fieldName] = null;
}
}
}
$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$aAttributes = $questionAttributeHelper->sanitizeQuestionAttributes($aAttributes);

return $aAttributes;
}
Expand Down Expand Up @@ -600,6 +599,11 @@ protected static function getGeneralAttibutesFromXml($sXmlFilePath)
$aAttributes[$xmlAttribute]['name'] = $xmlAttribute;
}
}

// Filter all pesky '[]' values (empty values should be null, e.g. <default></default>).
$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$aAttributes = $questionAttributeHelper->sanitizeQuestionAttributes($aAttributes);

return $aAttributes;
}

Expand Down Expand Up @@ -630,4 +634,5 @@ public static function getOwnQuestionAttributesViaPlugin()

return (array) $result->get('questionAttributes');
}

}
4 changes: 4 additions & 0 deletions application/models/QuestionTheme.php
Expand Up @@ -1047,6 +1047,10 @@ public static function getAdditionalAttrFromExtendedTheme($sQuestionThemeName, $
}
}
}

$questionAttributeHelper = new LimeSurvey\Models\Services\QuestionAttributeHelper();
$additionalAttributes = $questionAttributeHelper->sanitizeQuestionAttributes($additionalAttributes);

return $additionalAttributes;
}

Expand Down
129 changes: 129 additions & 0 deletions application/models/services/QuestionAttributeHelper.php
@@ -0,0 +1,129 @@
<?php

namespace LimeSurvey\Models\Services;

class QuestionAttributeHelper
{
/**
* Merges the 'base' attributes (ex: core question attributes) with the extended question attributes
* (ex: question theme attributes). It also removes all attributes where extended attribute's inputType is
* empty.
* If an extended attribute's name cannot be determined, it's omitted.
*
* @param array $aBaseAttributes the base set of attributes
* @param array $aExtendedAttributes the attributes to merge into the base set
*
* @return array the merged attributes
*/
public function mergeQuestionAttributes($aBaseAttributes, $aExtendedAttributes)
{
$aAttributes = $aBaseAttributes;
foreach ($aExtendedAttributes as $key => $attribute) {
// TODO: move to sanitizeQuestionAttributes()
// Determine the attribute name or continue with the next
if (!isset($attribute['name'])) {
if (!is_numeric($key)) {
$attribute['name'] = $key;
} else {
continue;
}
}

$sAttributeName = $attribute['name'];
$sInputType = $attribute['inputtype'];
// remove attribute if inputtype is empty
if (empty($sInputType)) {
unset($aAttributes[$sAttributeName]);
} else {
$aCustomAttribute = array_merge(
\QuestionAttribute::getDefaultSettings(),
array("category" => gT("Template")),
$attribute
);
$aAttributes[$sAttributeName] = $aCustomAttribute;
}
}
return $aAttributes;
}

/**
* Sanitizes an array of question attributes.
* Currently just replaces empty arrays (generally resulting from empty xml nodes) with null.
*
* @param array $aAttributes the array of attributes to sanitize
*
* @return array the array of sanitized attributes
*/
public function sanitizeQuestionAttributes($aAttributes)
{
// Replace empty arrays with null
foreach ($aAttributes as &$aAttribute) {
foreach ($aAttribute as $propertyName => $propertyValue) {
if ($propertyValue === []) {
$aAttribute[$propertyName] = null;
}
}
}
return $aAttributes;
}

/**
* Returns the received array of attributes filled with the values specified, taking into account the
* 'i18n' property of the attributes.
*
* Both this and rewriteQuestionAttributeArray() are helper methods and accomplish quite similar tasks,
* but the output is different: rewriteQuestionAttributeArray returns a name -> value array, while here
* we return a complete definition map and the value as a piece of information mingled into it.
*
* @param array $aAttributes the attributes to be filled
* @param array $aAttributeValues the values for the attributes
* @param array $aLanguages the languages to use for i18n attributes
*
* @return array the same source attributes with their corresponding values (when available)
*/
public function fillAttributesWithValues($aAttributes, $aAttributeValues, $aLanguages = [])
{
foreach ($aAttributes as $iKey => $aAttribute) {
if ($aAttribute['i18n'] == false) {
if (isset($aAttributeValues[$aAttribute['name']][''])) {
$aAttributes[$iKey]['value'] = $aAttributeValues[$aAttribute['name']][''];
} else {
$aAttributes[$iKey]['value'] = $aAttribute['default'];
}
// Sanitize value in case it's saved as empty array
if ($aAttributes[$iKey]['value'] === []) {
$aAttributes[$iKey]['value'] = null;
}
} else {
foreach ($aLanguages as $sLanguage) {
if (isset($aAttributeValues[$aAttribute['name']][$sLanguage])) {
$aAttributes[$iKey][$sLanguage]['value'] = $aAttributeValues[$aAttribute['name']][$sLanguage];
} else {
$aAttributes[$iKey][$sLanguage]['value'] = $aAttribute['default'];
}
// Sanitize value in case it's saved as empty array
if ($aAttributes[$iKey][$sLanguage]['value'] === []) {
$aAttributes[$iKey][$sLanguage]['value'] = null;
}
}
}
}
return $aAttributes;
}

/**
* Receives an array of question attributes and groups them by category.
* Used by advanced settings widget.
*
* @param array $aAttributes
* @return array Grouped question attributes, with category as array key
*/
public function groupAttributesByCategory($aAttributes)
{
$aByCategory = [];
foreach ($aAttributes as $aAttribute) {
$aByCategory[$aAttribute['category']][] = $aAttribute;
}
return $aByCategory;
}
}

0 comments on commit fe3a90d

Please sign in to comment.