View Issue Details

IDProjectCategoryView StatusLast Update
17261Development Otherpublic2021-08-05 15:41
Reportergabrieljenik Assigned Togabrieljenik  
Status resolvedResolutionfixed 
Summary17261: Create new code artifacts to handle QuestionAttributes fetching (& saving)
Description## Olle:

There's a design challenge in the application/models/services/QuestionAttributeHelper.php now. All the previous methods have no dependencies, but the new method depends on (static) methods from model QuestionAttribute and QuestionTheme. So should they be in the same class? Or should the previous methods be moved outside a class instead, but in a namespace? (Might be OK if they have neither state nor dependencies, contradicting what I said earlier.)

If you do constructor injection, you have to inject the dependencies even if they are not used (or do nullable dependencies).
Or you need a new helper: QuestionAttributeValuesHelper. :|

## Gabriel:

I would try to follow the rule:

DB Access, XML as a Source: Model
Handling information: Service / Helper
XML handling, like installation or scanning: Service / Helper
If I had to do these again, I would make the Attribute to be an abstract class with 3 implementation classes depending on the source: DB, XML and Plugin Events. I think all of them could be model-like classes (all sources of information) sharing part of their interfaces.

Then a QuestionAttributes Collection-Like class coordinating all of them (probably holding some of the code which is now on the service class).

The Service class would be then much slimer.

## Olle

Yes, this is the problem. We have one QuestionAttribute class for db, but as you say, since LS4, question attributes have multiple storage points, not only database.

If we have one domain model with two storage strategies, we could consider not using ActiveRecord, but maybe in fact the repository pattern?

Maybe it's too complex and overkill, but the point is to separate the domain model from whatever storage strategy is used. ActiveRecord is not adapted for that, since it assumes a database storage (and not file/XML).
Additional Information## Olle

$qf = new QuestionAttributeFetcher();
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);

## Denis
then need 2 function : one to get whole attributes definition + one for values (from DB ?)

what's the difference between whole definition and partial definition...?
- core attributes
- theme attribute
- plugin attribute
- (future) survey theme attribute
- (future) extend em attribute
- (future) i don't klnow attribute
- partial : see all issue questiontheme and plugin with question attribute since 4.0.0 : we need to be sure to have whole attribute … and it's not the case in 4.0, 4.1, 4.2, 4.3, 4.4 …

## Olle

Yes, and if setTheme is used, it would get attributes from core and theme (and plugin, by default). Can you amend my code with the changes you need?

## Denis

> Yes, and if `setTheme` is used, it would get attributes from core and theme (and plugin, by default). Can you amend my code with the changes you need?

I don't want to amend anytghing , i just answer about:

> Another thought, related to function composition: If we already have the question attributes, maybe it would be enough to do "populateValues(getQuestionAttributes());"?

Why not, but if we have a `populateValues` we need a `getAllAttributesDefinition` for a qid.

Else , about :

$qf = new QuestionAttributeFetcher();
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);

Since theme are not in question :

$qf = new QuestionAttributeFetcher();
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);


* set the theme for current qid
* @param string $theme, if null get current one by database
* @return void
public function setTheme($theme = null) {

TagsNo tags attached.


related to 16669 closedgabrieljenik Bug reports getQuestionAttributes function don't get the plugins attribute 
related to 17262 resolvedgabrieljenik Development  Create new getQuestionAttributes() endpoint for LimesurveyAPI which is used by plugins 
related to 17355 assigned Development  Code cleanup after attributeFetcher service 

Users monitoring this issue

User List There are no users monitoring this issue.




2021-05-07 16:56

manager   ~64291

Last edited: 2021-05-07 16:57

View 2 revisions


- QuestionAttributeProvider abstract class
  coreQuestionAttributeProvider, themeQuestionAttributeProvider, pluginQuestionAttributeProvider

- QuestionAttributeFetcher, only fetches definitions from the providers

- QuestionAttributeValueFetcher, fetches values from DB and merge it with defaults from definitions.
  This would be similar to the current service class. We could use the current one actually, without changing the name.

- QuestionAttribute model should be called QuestionAttributeValue.
  Wouldn't rename it so far, but would be great. This model would be called from QuestionAttributeValueFetcher

  Still, some of its methods should be extracted out:
  + getQuestionAttributesSettings
  + getAdvancedAttributesFromXml
  + getGeneralAttibutesFromXml
  + getOwnQuestionAttributesViaPlugin
  + addAdditionalAttributesFromExtendedTheme

  This should be moved either to a QuestionAttributeProvider implementation or to a service helper.

What do you think?


2021-05-10 16:16

manager   ~64330

First draft:
@ollehar comments?


2021-05-10 16:18

administrator   ~64331

Cool! Don't have time to read today, but tomorrow (I hope).


2021-05-26 18:24

manager   ~64572


Ready for reviewal

Issue History

Date Modified Username Field Change
2021-04-20 17:34 gabrieljenik New Issue
2021-04-20 17:34 gabrieljenik Status new => assigned
2021-04-20 17:34 gabrieljenik Assigned To => gabrieljenik
2021-04-20 17:34 gabrieljenik Issue generated from: 16669
2021-04-20 17:34 gabrieljenik Relationship added related to 16669
2021-04-20 17:34 gabrieljenik Project Bug reports => Development
2021-04-20 17:34 gabrieljenik Category Question editor => Other
2021-04-20 17:38 gabrieljenik Issue cloned: 17262
2021-04-20 17:38 gabrieljenik Relationship added related to 17262
2021-04-27 09:55 c_schmitz Product Version 4.3.16 =>
2021-04-27 09:55 c_schmitz Sync to Zoho Project Yes =>
2021-04-27 09:55 c_schmitz Sync to Zoho Project => |Yes|
2021-04-27 09:57 c_schmitz Sync to Zoho Project Yes =>
2021-04-27 10:02 jorynagel Sync to Zoho Project => |Yes|
2021-05-07 16:56 gabrieljenik Note Added: 64291
2021-05-07 16:57 gabrieljenik Note Edited: 64291 View Revisions
2021-05-10 16:16 gabrieljenik Note Added: 64330
2021-05-10 16:18 ollehar Note Added: 64331
2021-05-26 18:24 gabrieljenik Note Added: 64572
2021-05-26 18:24 gabrieljenik Status assigned => review
2021-06-07 20:20 gabrieljenik Issue cloned: 17355
2021-06-07 20:20 gabrieljenik Relationship added related to 17355
2021-08-05 15:41 ollehar Status review => resolved
2021-08-05 15:41 ollehar Resolution open => fixed