View Issue Details

This bug affects 1 person(s).
 6
IDProjectCategoryView StatusLast Update
16488Bug reportsQuestion editorpublic2020-09-08 07:43
Reportergabrieljenik Assigned Togabrieljenik  
PrioritynoneSeverityminor 
Status closedResolutionfixed 
Product Version3.22.25 
Summary16488: Reviewing updateQuestionOrder usage
DescriptionOn v4 we have noticed that sometimes question_order number is set following the survey_id.
That was due to a wrong usage of question::updateQuestionOrder() mehod and its parameters.

As the codebase for that seems similar on v3, review there if it used correctly and possible impacts
Additional InformationSee application/controllers/admin/database.php::756
```
                    // if the group has changed then fix the sortorder of old and new group
                    Question::model()->updateQuestionOrder($oldgid, $iSurveyID);
```
TagsNo tags attached.
Bug heat6
Complete LimeSurvey version number (& build)3.22.25
I will donate to the project if issue is resolvedNo
Browser
Database & DB-VersionMysql
Server OS (if known)
Webserver software & version (if known)
PHP Version7

Relationships

related to 16454 closedgabrieljenik Sometimes, question_order is assigned from survey_id 

Activities

gabrieljenik

gabrieljenik

2020-07-10 20:47

manager   ~58904

The method `Question ::updateQuestionOrder()` receives 3 args: `gid`, `language`, `startPosition`.
The database controller calls this method wrongly three times directly, passing the survey ID as second parameter (where it was supposed to be language), so those invocations have no effect, as the questions query returns no items.

At last, when moving questions in between groups (to a lower group), the database controller calls the function shiftOrderQuestions (which in turn calls updateQuestionOrder). That seems to be OK and seems to be the only point where it's required.

Should we remove those bad invocations?
gabrieljenik

gabrieljenik

2020-08-04 19:54

manager   ~59307

- Removed language from Question::updateQuestionOrder parameters.
  The method now uses the survey base language to work.

- Changed how order is handled in `actionUpdateQuestion` (`database.php`)
- Removed call to `Question::updateQuestionOrder` in `actionInsertCopyQuestion()` (database.php), because order was already handled.
- Updated `shiftOrderQuestions` (common_helper.php) for not using language inside of it.
  This function is not used anymore in Limesurvey's codebase, but still it might be used by others, or in the future.
- Calls to Question::updateSortOrder where replaced by Question::updateQuestionOrder for consistency.
  `updateSortOrder` is not removed from the codebase, just in case.

Test scenarios:
- Insert Questions
- Edit question group attribute. Move to upper group
- Edit question group attribute. Move to lower group
- Move questions from sidebar to another group
- Move questions from sidebar within the same group
- Move questions from Reorganizer
- Copy from one group to another group
- Copy within the same group
- Delete
All times, order should be checked visually, refreshing screens and also on the DB.

PR: https://github.com/LimeSurvey/LimeSurvey/pull/1532
cdorin

cdorin

2020-09-07 16:00

manager   ~59727

In the latest update getting a lot of: 500: Internal Server Error
CDbCommand failed to execute the SQL statement: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'question_order' cannot be null when saving questions. Not sure if related to the changes from here.
gabrieljenik

gabrieljenik

2020-09-08 02:36

manager   ~59729

Yes, new PR uploaded https://github.com/LimeSurvey/LimeSurvey/pull/1583 and rebased on curren lsv3
gabrieljenik

gabrieljenik

2020-09-08 07:33

manager   ~59730

Fix committed to 3.x-LTS branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=30447
lime_release_bot

lime_release_bot

2020-09-08 07:43

administrator   ~59731

Fixed in Release 3.23.3+200909

Related Changesets

LimeSurvey: 3.x-LTS 27d16225

2020-09-08 07:33:46

gabrieljenik


Committer: GitHub Details Diff
Fixed Issue 16488: [Hotfix] Reviewing updateQuestionOrder usage

Code review
Affected Issues
16488
mod - application/controllers/admin/database.php Diff File

Issue History

Date Modified Username Field Change
2020-07-10 20:09 gabrieljenik New Issue
2020-07-10 20:09 gabrieljenik Issue generated from: 16454
2020-07-10 20:09 gabrieljenik Relationship added related to 16454
2020-07-10 20:47 gabrieljenik Note Added: 58904
2020-07-22 17:27 user225042 Status new => confirmed
2020-07-22 17:27 user225042 Zoho Sprints => |Yes|
2020-07-22 17:27 swendrich Zoho Sprints ID => 14469000000188037
2020-08-04 19:54 gabrieljenik Note Added: 59307
2020-09-07 16:00 cdorin Note Added: 59727
2020-09-08 02:36 gabrieljenik Note Added: 59729
2020-09-08 07:33 gabrieljenik Changeset attached => LimeSurvey 3.x-LTS 27d16225
2020-09-08 07:33 gabrieljenik Note Added: 59730
2020-09-08 07:33 gabrieljenik Assigned To => gabrieljenik
2020-09-08 07:33 gabrieljenik Resolution open => fixed
2020-09-08 07:43 lime_release_bot Zoho Sprints Yes => |Yes|
2020-09-08 07:43 lime_release_bot Note Added: 59731
2020-09-08 07:43 lime_release_bot Status confirmed => closed