View Issue Details

This bug affects 1 person(s).
 8
IDProjectCategoryView StatusLast Update
18146Bug reportsSurvey editingpublic2022-07-05 12:27
ReporterMazi Assigned ToDenisChenu  
PrioritynoneSeveritypartial_block 
Status closedResolutionfixed 
Product Version5.3.x 
Fixed in Version5.3.x 
Summary18146: Using "other" as subquestion code at array questions leads to saving error
Description

When trying to use "other" as subquestion code at an array question I get the below error message. The error message is misleading though:
"An error happened: Could not save subquestion Array ( [title] => Array ( [0] => 'other' can not be used if the 'Other' option for this question is activated. ) )"

-> There simply is no "other" option for array questions. So to make array filter work with e.g. a multi choice question with "other" field, it would be great if we could accept "other" as subquestion code for arrays.

Steps To Reproduce

Steps to reproduce

Create an array question and use "other" as subquestion code.

Expected result

Accept "other" as valid code.

Actual result

Misleading error message is shown about the "other" free text option which das not exist for this question type.

TagsNo tags attached.
Attached Files
image.png (77,759 bytes)   
image.png (77,759 bytes)   
Bug heat8
Complete LimeSurvey version number (& build)5.3.13
I will donate to the project if issue is resolvedNo
BrowserChrome
Database type & versionMariaDB 10.1.48
Server OS (if known)Ubuntu 18
Webserver software & version (if known)Apache 2.0
PHP Version7.4

Users monitoring this issue

There are no users monitoring this issue.

Activities

DenisChenu

DenisChenu

2022-05-24 14:56

developer   ~69998

Seems parent question still have other == 'Y'
https://github.com/LimeSurvey/LimeSurvey/blob/a529a3d1a2c553cbd2494b2ea6d801ca87478c4f/application/models/Question.php#L171

You can try to save as array before try again ?

Mazi

Mazi

2022-05-24 14:59

updater   ~69999

Hmm, we have switched this question from a single choice question into an array questions. Could this be some kind of left-over?

DenisChenu

DenisChenu

2022-05-24 15:03

developer   ~70000

Last edited: 2022-05-24 15:04

Surely a single choice with other …

But : i think other='N' must be resetted when you save
Else : we have this error in 3.X

Maybe you can check with 3.X

  1. Create a single choice Other == 'Y'
  2. Save it
  3. Update to array
  4. Add other for subquestions

Then : dev can check how it's done in 3.X

Mazi

Mazi

2022-05-24 15:07

updater   ~70001

At LS3 the error does NOT show up. I did the steps you described and could use "other" there.

image-2.png (11,518 bytes)   
image-2.png (11,518 bytes)   
DenisChenu

DenisChenu

2022-05-24 15:18

developer   ~70003

Thank you !

DenisChenu

DenisChenu

2022-05-26 19:39

developer   ~70061

Pull request : https://github.com/LimeSurvey/LimeSurvey/pull/2443

DenisChenu

DenisChenu

2022-05-30 11:54

developer   ~70097

Improved pull request : https://github.com/LimeSurvey/LimeSurvey/pull/2443

gabrieljenik

gabrieljenik

2022-06-09 23:35

manager   ~70337

Strange. I wasn't able to reproduce the issue on latest master (no patch applied).
@DenisChenu @Mazi Am I missing something?

image-3.png (37,694 bytes)   
image-3.png (37,694 bytes)   
DenisChenu

DenisChenu

2022-06-10 08:22

developer   ~70340

You must

Create a single choice Other == 'Y'
Save and close it
Update to array
Save and close it
Add other for subquestions
Save and close it

Other are not resetted if question type didn't allow other

gabrieljenik

gabrieljenik

2022-06-27 02:25

manager   ~70543

Tested OK. Haven't done regression testing.
Still considering the strange reproduction steps needed and the code updated, as to avoid regressions, I would send it to DEV as to have the code be sinked and lived for a while.

DenisChenu

DenisChenu

2022-06-27 10:09

developer   ~70547

I don't understand what you want it in DEV, there are no major recoding here. Just create some var in place of fixed test

Fixed test :

                ($oQuestion->type == Question::QT_L_LIST)
                || ($oQuestion->type == Question::QT_EXCLAMATION_LIST_DROPDOWN)
                || ($oQuestion->type == Question::QT_P_MULTIPLE_CHOICE_WITH_COMMENTS)
                || ($oQuestion->type == Question::QT_M_MULTIPLE_CHOICE)

Moving to a clean function

And add this rule :

        $aRules[] = array('other', 'filter', 'filter' => function ($value) {
            if ($this->getAllowOther()) {
                return $value;
            }
            return 'N';
        });

Already say : i prefer a good complete fix than create 4 bugs for the same file part …

gabrieljenik

gabrieljenik

2022-06-27 13:45

manager   ~70553

First of all, we had problems tying to reproduce it.
If you look at the steps for reproducing, they are not the usual path a user would do.
Also, I believe I tried couple of times, mixing "save" with "save and close" and wasn't able to reproduce it.
So, that's why I say it is not a very common issue.

Also, the PR is not just 2 blocks of changes, you are updating the definition for every question, re organizing how the title validations is done for all questions, .... Nevertheless the test is only a unit test about this fix.

At last as per "Already say : i prefer a good complete fix than create 4 bugs for the same file part …", I don't agree.
Doing bigger changes make issues harder to back-track, harder to test, more error prone, riskier to release, ...
We need to balance scope of fixing vs de-stabilization of the platform.
We are not perfect coders neither have the complete full view of the different situations that could arise.
So the least we update on a fix, sometimes is the better.

DenisChenu

DenisChenu

2022-06-27 15:01

developer   ~70560

Also, the PR is not just 2 blocks of changes, you are updating the definition for every question, re organizing how the title validations is done for all questions, .... Nevertheless the test is only a unit test about this fix.

See :
It's really 2 update … moving hardcoded definition to place where it need to be …

gabrieljenik

gabrieljenik

2022-06-27 15:05

manager   ~70561

Last edited: 2022-06-27 15:05

Well.. sometimes it is OK if we disagree :)

DenisChenu

DenisChenu

2022-06-27 15:34

developer   ~70565

You have some linefeed too , and moving

       /* Don't save empty or 'core' question theme name */
        $aRules[] = ['question_theme_name', 'questionThemeNameValidator'];

before if (!$this->isNewRecord) {

There are a related issue

DenisChenu

DenisChenu

2022-07-01 14:52

developer   ~70671

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=33613

LimeBot

LimeBot

2022-07-05 12:27

administrator   ~70709

Fixed in Release 5.3.23+220705

Related Changesets

LimeSurvey: master 69a9d438

2022-07-01 14:37:51

DenisChenu


Committer: GitHub Details Diff
Fixed issue 18146: Unable to use "other" as subquestion code (#2443) Affected Issues
18146
mod - application/controllers/QuestionAdministrationController.php Diff File
mod - application/models/Question.php Diff File
mod - application/models/QuestionTheme.php Diff File
mod - application/models/QuestionType.php Diff File
mod - application/views/survey/questions/answer/list_dropdown/config.xml Diff File
mod - application/views/survey/questions/answer/listradio/config.xml Diff File
mod - application/views/survey/questions/answer/multiplechoice/config.xml Diff File
mod - application/views/survey/questions/answer/multiplechoice_with_comments/config.xml Diff File

Issue History

Date Modified Username Field Change
2022-05-24 14:41 Mazi New Issue
2022-05-24 14:41 Mazi File Added: image.png
2022-05-24 14:56 DenisChenu Note Added: 69998
2022-05-24 14:56 DenisChenu Bug heat 0 => 2
2022-05-24 14:59 Mazi Note Added: 69999
2022-05-24 14:59 Mazi Bug heat 2 => 4
2022-05-24 15:03 DenisChenu Note Added: 70000
2022-05-24 15:04 DenisChenu Note Edited: 70000
2022-05-24 15:07 Mazi Note Added: 70001
2022-05-24 15:07 Mazi File Added: image-2.png
2022-05-24 15:18 DenisChenu Note Added: 70003
2022-05-24 15:18 DenisChenu Assigned To => DenisChenu
2022-05-24 15:18 DenisChenu Status new => assigned
2022-05-26 19:39 DenisChenu Note Added: 70061
2022-05-26 19:39 DenisChenu Assigned To DenisChenu =>
2022-05-26 19:39 DenisChenu Status assigned => ready for testing
2022-05-30 11:54 DenisChenu Note Added: 70097
2022-06-03 22:57 gabrieljenik Assigned To => gabrieljenik
2022-06-03 22:57 gabrieljenik Status ready for testing => in testing
2022-06-09 23:35 gabrieljenik Note Added: 70337
2022-06-09 23:35 gabrieljenik File Added: image-3.png
2022-06-09 23:35 gabrieljenik Bug heat 4 => 6
2022-06-09 23:35 gabrieljenik Status in testing => feedback
2022-06-10 08:22 DenisChenu Note Added: 70340
2022-06-10 08:22 DenisChenu File Added: Capture d’écran du 2022-06-10 08-20-58.png
2022-06-27 02:25 gabrieljenik Status feedback => ready for merge
2022-06-27 02:25 gabrieljenik Note Added: 70543
2022-06-27 10:09 DenisChenu Note Added: 70547
2022-06-27 13:45 gabrieljenik Note Added: 70553
2022-06-27 15:01 DenisChenu Note Added: 70560
2022-06-27 15:05 gabrieljenik Note Added: 70561
2022-06-27 15:05 gabrieljenik Note Edited: 70561
2022-06-27 15:34 DenisChenu Note Added: 70565
2022-07-01 14:52 DenisChenu Changeset attached => LimeSurvey master 69a9d438
2022-07-01 14:52 DenisChenu Note Added: 70671
2022-07-01 14:52 DenisChenu Assigned To gabrieljenik => DenisChenu
2022-07-01 14:52 DenisChenu Resolution open => fixed
2022-07-01 16:35 DenisChenu Status ready for merge => resolved
2022-07-01 16:35 DenisChenu Fixed in Version => 5.3.x
2022-07-05 12:27 LimeBot Note Added: 70709
2022-07-05 12:27 LimeBot Status resolved => closed
2022-07-05 12:27 LimeBot Bug heat 6 => 8