View Issue Details

This bug affects 1 person(s).
 6
IDProjectCategoryView StatusLast Update
18058Bug reportsOtherpublic2022-05-13 08:51
Reportergabrieljenik Assigned Togabrieljenik  
PrioritynoneSeveritypartial_block 
Status assignedResolutionopen 
Product Version5.3.x 
Summary18058: Some survey attributes use the empty value for N and confuses inheritance rules
Description

This is a followup of 18050.

TagsNo tags attached.
Bug heat6
Complete LimeSurvey version number (& build)5.3.11
I will donate to the project if issue is resolvedNo
Browser
Database type & versionn/a
Server OS (if known)
Webserver software & version (if known)
PHP Versionn/a

Relationships

child of 18050 closedgabrieljenik Admin notification email sent out even when disabled 

Users monitoring this issue

User List There are no users monitoring this issue.

Activities

gabrieljenik

gabrieljenik

2022-04-27 20:49

manager   ~69268

Last edited: 2022-04-27 20:49

Since survey settings inheritance have been introduced, empty attributes have always been inherited.
But for some attributes, an empty value is actually a valid attribute.

In those cases, the inheritance rules get confused. Is the attribute empty because it should be inherited or because it is deactivated?
Created specific rules for specific attributes.

PR https://github.com/LimeSurvey/LimeSurvey/pull/2370

c_schmitz

c_schmitz

2022-04-27 20:49

administrator   ~69269

I thought in this case the field would store the value 'inherit'.

DenisChenu

DenisChenu

2022-04-27 20:49

developer   ~69270

I really think we must move inherit to null in database.
Then you can merge attributes
finalValues= array_merge(
$globalValues,
$groupSettings,
$surveySettings
);

Maybe we can reproduce with some array_filter ?
array_filter($surveySettings, function($setting) {
return $setting != "inherit";
});

gabrieljenik

gabrieljenik

2022-04-27 20:49

manager   ~69271

The solution I uploaded is a temporary solution.
Alternatives we thought:

Right now the buttons are "Inherit On", "Inherit Off".
Make them "On", "Off", "Inherir".
In case they select "Off", we can:
a) store "N" in the email field or
b) store "N" on a new field.
c) store empty on the email field.
That would be a UI complement to the current patch.
Not much else needed.

I think the best and the less error prone is alternative B.

gabrieljenik

gabrieljenik

2022-04-27 20:49

manager   ~69272

I really think we must move inherit to null in database.

Not sure I follow. Isn't that a different discussion?
The problem here is that we are using empty as a NO option but the inheritance is understanding it as an Inherit command

DenisChenu

DenisChenu

2022-04-28 09:16

developer   ~69278

Last edited: 2022-04-28 09:18

Not sure I follow. Isn't that a different discussion?

It's exactly the same discussion :)

"" or 0 still return "" or 0
BUT null value is used for inherit.
If inherited is ""

Then code can test if(!attribute) { }

A quick edit on https://github.com/LimeSurvey/LimeSurvey/pull/2370/files#diff-1a1aff57bf56ce79a03058f0d5bc46f25b3f768b3bc41b53bdcd0b07959ced90R469 private function shouldInherit($attribute)

Add || is_null($this->oOptions->{$attribute}) at line 479

        // Always returhn true if NULL or not set
        if(!isset($this->oOptions->{$attribute})) {
            return true;
        }
        // The attribute should be inherited if its value is 'inherit', 'I' or '-1'. (maybe filter by attribute name)
        if (
                $this->oOptions->{$attribute} === 'inherit'
                || $this->oOptions->{$attribute} === 'I'
                // NB: Do NOT use === here, it won't work with Postgresql.
                || $this->oOptions->{$attribute} == '-1'
            )
        ) {
            return true;
        }
        // return 0, false and empty string
        return false;
gabrieljenik

gabrieljenik

2022-05-12 21:55

manager   ~69618

It's exactly the same discussion :)

I think it is a different discussion: A much bigger one! With more impact :)

I really think we must move inherit to null in database

This would be much more error prone when thinking of importing/exporting files. Null could be easliy lost.
I don't like taking 0 or "" as inherit, but I believe it is not the time to review that.

I just want to fix it for this attributes with the tools we already have

DenisChenu

DenisChenu

2022-05-13 08:51

developer   ~69622

You make it here : https://github.com/LimeSurvey/LimeSurvey/pull/2412/files

null/not set => inherit.

And i think it's the best way

I don't like taking 0 or "" as inherit, but I believe it is not the time to review that.

It's exactly my point : use is_null or !isset … for inherit, not "I" or 'inherit' or … etc …

Issue History

Date Modified Username Field Change
2022-04-27 20:49 gabrieljenik New Issue
2022-04-27 20:49 gabrieljenik Issue generated from: 18050
2022-04-27 20:49 gabrieljenik Note Added: 69268
2022-04-27 20:49 gabrieljenik Note Added: 69269
2022-04-27 20:49 gabrieljenik Note Added: 69270
2022-04-27 20:49 guest Bug heat 2 => 6
2022-04-27 20:49 gabrieljenik Bug heat 6 => 2
2022-04-27 20:49 guest Bug heat 2 => 6
2022-04-27 20:49 gabrieljenik Note Added: 69271
2022-04-27 20:49 gabrieljenik Bug heat 6 => 2
2022-04-27 20:49 gabrieljenik Note Added: 69272
2022-04-27 20:49 gabrieljenik Relationship added child of 18050
2022-04-27 20:49 guest Bug heat 2 => 6
2022-04-27 20:49 gabrieljenik Note Edited: 69268
2022-04-27 20:49 gabrieljenik Note Edited: 69268
2022-04-28 09:16 DenisChenu Note Added: 69278
2022-04-28 09:17 DenisChenu Note Edited: 69278
2022-04-28 09:18 DenisChenu Note Edited: 69278
2022-04-28 10:36 galads Assigned To => gabrieljenik
2022-04-28 10:36 galads Status new => assigned
2022-05-12 21:55 gabrieljenik Note Added: 69618
2022-05-13 08:51 DenisChenu Note Added: 69622