View Issue Details

This bug affects 1 person(s).
 8
IDProjectCategoryView StatusLast Update
18699Bug reportsOtherpublic2023-08-09 16:39
Reportergabrieljenik Assigned Toollehar  
PrioritynoneSeverityblock 
Status in code reviewResolutionopen 
Product Version5.6.x 
Summary18699: LSActiveRecord::getMaxId() doesn't get refreshed when using from SurveyDynamic with multiple surveys on the same HTTP request
Description

Caught while reviewing 18655
https://github.com/LimeSurvey/LimeSurvey/pull/2998#pullrequestreview-1352365154


GJ: The diema is this is only happening on these tests as the model is used with different surveyIds within the same HTTP request. This is not happening on real life.

DC: Until we use in in Question::model and Survey::model in same call …
There are an issue in this code … there are an easy way to fix : why you don't want to fix it ?


A workaround was implemented here for being able to use the remote_control->export_responses.
https://github.com/LimeSurvey/LimeSurvey/pull/2998

Steps To Reproduce

Steps to reproduce

Undo this change
https://github.com/LimeSurvey/LimeSurvey/pull/2998/files#diff-22d09ac4b2de7e29721c3fc14eb239f215498a280b8c37f4b718074646f01f5dR3168

Make a script for RC for exporting rsposnes from survey A and then from survey B.

Expected result

Survey B responses will be exported.

Actual result

No survey B responses will be exported.

TagsNo tags attached.
Bug heat8
Complete LimeSurvey version number (& build)5.6.7 + 6
I will donate to the project if issue is resolvedNo
Browser
Database type & versionMySQL
Server OS (if known)
Webserver software & version (if known)
PHP Version8.0

Relationships

related to 18655 closedc_schmitz Data loss if field in response table is missing 

Users monitoring this issue

User List There are no users monitoring this issue.

Activities

gabrieljenik

gabrieljenik

2023-03-23 14:37

manager   ~74246

I would go with:

1- Add a new optional parameter to getMaxId
public function getMaxId($field = null, $forceRefresh = false, $clear=false)

2 - Create LSActiveRecord::refreshMetadata() (overload of CActiveRecord).
Make the refreshMatedata to call getMaxId using the clear parameter.

gabrieljenik

gabrieljenik

2023-03-23 14:38

manager   ~74247

DC said...

a lot must be automatic. If not : it's unusuable

For example, you can do

$maxId1 = \Group::model()->getMaxId();
$maxId2 = \QuestionGroup::model()->getMaxId();
$maxId3 = \Token::model(123)->getMaxId();
$maxId4 = \SurveyDynamic::model(123)->getMaxId();
$maxId5 = \SurveyDynamic::model(321)->getMaxId();
$maxId6 = \Response::model(321)->getMaxId();

You must have the real $maxId at each time.

And more

$maxId2 = \QuestionGroup::model()->getMaxId();
$oQuestionGroup = new \QuestionGroup());
…
$oQuestionGroup->save();
$maxId3 = \QuestionGroup::model()->getMaxId();
gabrieljenik

gabrieljenik

2023-03-23 14:38

manager   ~74248

DC Said

A question : did we really need this one ? Maybe just delete this statci var

I think it's best to optimize other request (and use static or cache)

https://bugs.limesurvey.org/view.php?id=18473
https://bugs.limesurvey.org/view.php?id=18472
etc …

DenisChenu

DenisChenu

2023-03-23 14:53

developer   ~74250

There are an issue in this code … there are an easy way to fix : why you don't want to fix it ?

Not so easy … ;)

gabrieljenik

gabrieljenik

2023-03-23 15:37

manager   ~74251

haha I was trying to port the conversation :)

DenisChenu

DenisChenu

2023-03-23 15:41

developer   ~74252

Draft broken test : https://github.com/LimeSurvey/LimeSurvey/pull/3006

I like to keep some statci var when multiple load too

For example : wen you ask Options for Survey #1 in Group #1 : you ask option in Survey #1 + option on Group #1 + Global option , the n if you as after option in Survey #2 : i like to keep option on Group #1 + Global option

Same for Template option (where you can have more than on Global option …)

maxIds is a good test for all of this ;)

gabrieljenik

gabrieljenik

2023-03-23 21:59

manager   ~74259

Is it ok if I take this?
I will do a test script as well as the fix

DenisChenu

DenisChenu

2023-03-24 09:04

developer   ~74270

I think i get it
https://github.com/LimeSurvey/LimeSurvey/pull/3006

I need to add same for minIds

OK ?

DenisChenu

DenisChenu

2023-06-09 08:26

developer   ~75504

5.X : https://github.com/LimeSurvey/LimeSurvey/pull/3006 (already reviewed)
master : https://github.com/LimeSurvey/LimeSurvey/pull/3209

Di i create test ? I create only if confirmed , don't want to create and must fix again in 6 month.

DenisChenu

DenisChenu

2023-06-13 09:35

developer   ~75572

@ollehar : did i create test ? If auto test is OK : can merge ?

ollehar

ollehar

2023-06-13 09:48

administrator   ~75573

Are you sure this is not premature optimization? https://softwareengineering.stackexchange.com/questions/80084/is-premature-optimization-really-the-root-of-all-evil

DenisChenu

DenisChenu

2023-06-13 09:58

developer   ~75577

Alternative solution : reset if $dynamicId are updated.

What is your proposed solution ?

Issue still there.

ollehar

ollehar

2023-06-13 10:07

administrator   ~75578

The solution with less code but worse performance is to never cache it in a local variable.

ollehar

ollehar

2023-06-13 10:08

administrator   ~75579

But the trade-off with simpler code is worth it if performance is only very slightly affected. Depends on how often the code is called, if it's in a tight loop or so.

DenisChenu

DenisChenu

2023-06-13 10:48

developer   ~75583

Solutions

  1. Remove static var
  2. Get static var but reset if dynamic id was update
  3. Current PR

I can do 2 here.

DenisChenu

DenisChenu

2023-06-13 11:08

developer   ~75588

Maybe 1 is OK here

controllers/admin/Export.php:            $iMaximum = SurveyDynamic::model($iSurveyID)->getMaxId();
helpers/remotecontrol/remotecontrol_handle.php:        if (!($maxId = SurveyDynamic::model($iSurveyID)->getMaxId(null, true))) {
helpers/remotecontrol/remotecontrol_handle.php:        if (!($maxId = SurveyDynamic::model($iSurveyID)->getMaxId())) {
helpers/admin/activate_helper.php:    $iMaxQID = Question::model()->getMaxId('qid', true); // Always refresh as we insert new qid's
models/LSActiveRecord.php:    public function getMaxId($field = null, $forceRefresh = false)
ollehar

ollehar

2023-06-13 11:17

administrator   ~75589

Yes, assuming the export code is not in a loop?

DenisChenu

DenisChenu

2023-06-13 11:25

developer   ~75590

No,

and remove the $forceRefresh then …

ollehar

ollehar

2023-06-13 11:26

administrator   ~75591

Yep, not needed. :)

gabrieljenik

gabrieljenik

2023-06-13 14:31

manager   ~75600

Sorry got lost. What's wrong with Denis solution?

My initial idea (https://bugs.limesurvey.org/view.php?id=18699#c74246) was to:
1- Add a new optional parameter to getMaxId
public function getMaxId($field = null, $forceRefresh = false, $clear=false)

2 - Create LSActiveRecord::refreshMetadata() (overload of CActiveRecord).
Make the refreshMatedata to call getMaxId using the clear parameter.
refreshMetadata is already called inside the dynamic model methods when changing dynamicIds.

That can be complemented by making this variable inside the method static $maxIds = []; to be declared as a class attribute.
That would make things more flexible in the future and create methods which are only for refreshing or clearing, ....

By looking at Denis solution, now in DEV branch, I would also add some more stuff like:

  • adding getDynamicId() to some classes
  • making more classes to derive from Dynamic
DenisChenu

DenisChenu

2023-06-13 15:13

developer   ~75609

@gabrieljenik : removing static var and no issue.

Since don't seems to be used more than one time in action : we don't need a static var

gabrieljenik

gabrieljenik

2023-06-13 16:15

manager   ~75615

Could be. It would be inline with simplifying. It would remove the cache.

Still, what's wrong with Denis solution?

tibor.pacalat

tibor.pacalat

2023-06-14 13:50

administrator   ~75623

Is this ready for testing or not? I can't tell, because the discussion is still ongoing and this is tagged with "in code review", but PRs are tagged with "Code review done" :/
If this is ready to test, please provide me with some good instructions, plus the script that is mentioned in "how to reproduce".

tibor.pacalat

tibor.pacalat

2023-08-09 16:39

administrator   ~76524

@DenisChenu Is this ready for testing or not? I can't tell, because the discussion is still ongoing and this is tagged with "in code review", but PRs are tagged with "Code review done" :/
If this is ready to test, please provide me with some good instructions, plus the script that is mentioned in "how to reproduce".

Issue History

Date Modified Username Field Change
2023-03-23 14:35 gabrieljenik New Issue
2023-03-23 14:35 gabrieljenik Issue generated from: 18655
2023-03-23 14:35 gabrieljenik Relationship added related to 18655
2023-03-23 14:36 gabrieljenik Description Updated
2023-03-23 14:37 gabrieljenik Note Added: 74246
2023-03-23 14:37 gabrieljenik Bug heat 0 => 2
2023-03-23 14:38 gabrieljenik Note Added: 74247
2023-03-23 14:38 gabrieljenik Note Added: 74248
2023-03-23 14:53 DenisChenu Note Added: 74250
2023-03-23 14:53 DenisChenu Bug heat 2 => 4
2023-03-23 15:37 gabrieljenik Note Added: 74251
2023-03-23 15:41 DenisChenu Note Added: 74252
2023-03-23 21:59 gabrieljenik Note Added: 74259
2023-03-23 22:00 gabrieljenik Status new => confirmed
2023-03-24 09:04 DenisChenu Note Added: 74270
2023-06-09 08:26 DenisChenu Note Added: 75504
2023-06-09 08:26 DenisChenu Assigned To => gabrieljenik
2023-06-09 08:26 DenisChenu Status confirmed => ready for code review
2023-06-09 08:26 DenisChenu Complete LimeSurvey version number (& build) 5.6.7 => 5.6.7 + 6
2023-06-09 14:57 gabrieljenik Assigned To gabrieljenik => DenisChenu
2023-06-09 14:57 gabrieljenik Status ready for code review => ready for testing
2023-06-13 09:35 DenisChenu Note Added: 75572
2023-06-13 09:48 ollehar Note Added: 75573
2023-06-13 09:48 ollehar Bug heat 4 => 6
2023-06-13 09:58 DenisChenu Note Added: 75577
2023-06-13 10:07 ollehar Note Added: 75578
2023-06-13 10:08 ollehar Note Added: 75579
2023-06-13 10:48 DenisChenu Note Added: 75583
2023-06-13 10:48 DenisChenu Assigned To DenisChenu => ollehar
2023-06-13 10:48 DenisChenu Status ready for testing => new
2023-06-13 10:48 DenisChenu Status new => in code review
2023-06-13 11:08 DenisChenu Note Added: 75588
2023-06-13 11:17 ollehar Note Added: 75589
2023-06-13 11:25 DenisChenu Note Added: 75590
2023-06-13 11:26 ollehar Note Added: 75591
2023-06-13 14:31 gabrieljenik Note Added: 75600
2023-06-13 15:13 DenisChenu Note Added: 75609
2023-06-13 16:15 gabrieljenik Note Added: 75615
2023-06-14 13:50 tibor.pacalat Note Added: 75623
2023-06-14 13:50 tibor.pacalat Bug heat 6 => 8
2023-08-09 16:39 tibor.pacalat Note Added: 76524