Branch: bug/out-of-memory-statistics (based on 5.x, but needs to be done in master too)

Started some work here already.

The code raw SQL inside displaySimpleResults() needs to be restored and Response::model()->findAll() removed, together with the loops. The raw SQL need to compare with encrypted version of empty string or “-oth-” if the question is encrypted.

Response::findAll() didn't decrypt all response ? Right ?

it does not, Denis, but Dominik looped all responses to decrypt them, which obviously will never work with lots of participants.

it does not, Denis, but Dominik looped all responses to decrypt them, which obviously will never work with lots of participants.

Right … really complex here

What solution we have ?

We :

• Check ALL questions in statistics
• Ask for graph for all too

I'm unsure we have a clean solution …

Capture d’écran du 2023-05-02 16-06-46.png (45,568 bytes)   

Capture d’écran du 2023-05-02 16-06-46.png (45,568 bytes)   

What solution we have ?

Read my commit in the branch.

We don't salt the encryptions, so exact compare is possible, which is all that it takes. Dominik didn't realize, it seems.

We don't salt the encryptions, so exact compare is possible, which is all that it takes. Dominik didn't realize, it seems.

Yes, it's a low security encryption here. But it's here : https://github.com/LimeSurvey/LimeSurvey/blob/f0e7e3acbfe76a78237ff8ab4b0989d2acf692d2/application/helpers/admin/statistics_helper.php#L530 (for single choice).

Available only for single choice, you can encrypt Answers code value.
Maybe get the encryoted value her e: https://github.com/LimeSurvey/LimeSurvey/blob/f0e7e3acbfe76a78237ff8ab4b0989d2acf692d2/application/helpers/admin/statistics_helper.php#L639

But if you have 50 free text question : still broken …

From my point of view: For the open text questions,

• either we query for exact match
• or we don't allow to filter by open text questions which are encrypted. (right now that doesn't work)

I believe a list of text answers is also possible to show there.
Besides being encrypted or not, that would also be very complex with surveys with many answers.
Maybe something to remove? (That's not stats anyway, just list of responses). :)

2 POV

About filter

• We can filter by encrypted value for single choice (or Y code) : Good idea
• Disable filter by free text is a bad idea : it work if you have some response and/or a lot of memory : put an alert is the best .

About using to construct the graph (or export etc …)
Maybe we can use an instance when we have a lot ?
See export part for example : https://github.com/LimeSurvey/LimeSurvey/blob/77a234a70fbca29ce65180e895cde85b85ebdbf8/application/helpers/export_helper.php#L133

foreach ($result as $row) {
     $oResponse->decrypt();
}

Maybe : decrypt can use some already Answers->crypted value ? Done at start with all crypted value of single answer ?
Take memory too (the [cryptedValue=>showValue] must be put in memory) but it can really improve a lot of functionnality ?

Ehm, there's no reason to restrict a feature if we can just fix it, or? Unless the fix is very hard.

Disable filter by free text is a bad idea : it work if you have some response and/or a lot of memory : put an alert is the best .

either we query for exact match

A checkbox (with explain that can allow to search on big data in case of memory issue) ?

there's no reason to restrict a feature if we can just fix it, or? Unless the fix is very hard.

Well,

• We want to avoid old approach (downloading data into memory + loops + decrypt).
• We can only query a encrypted field by exact match
• Filtiering a text input by exact match seems to lead to confusion, false expectations and probably lot of support issues and complains.
  So, what other fix alternatives do we have?

We can filter by encrypted value for single choice (or Y code) : Good idea

Yes. We are already doing that in some places.
And implementing on others.

Disable filter by free text is a bad idea : it work if you have some response and/or a lot of memory : put an alert is the best .

I don't like the alert. "Use at your own risk. Please take into account this may fail"? :)

About using to construct the graph (or export etc …)

Will review it. Bump into this blocking thing. Wanted to solve it as it may guide the rest of the solution.

I don't like the alert. "Use at your own risk. Please take into account this may fail"? :)

But remove a feature to all user because some can have issue it's worst …

You can not remove the current feature allowing to search in contains on 200 response because you need to disable (maybe) in 5 000

You can not remove the current feature allowing to search in contains on 200 response because you need to disable (maybe) in 5 000

Well, that's a downside :) haha
I just don't value that feature that much (and think that doesn't fit there), so I think could be removed in favor of a more ordered script/screen.
But I get your point.

Would like to hear @ollehar about other probable fixing ideas.

A checkbox (with explain that can allow to search on big data in case of memory issue) ?

This I think could be a good workaround.

Wasn't the current code already limited to exact match? Maybe I misunderstood it.

Well, there is some work to be done here.
This will not work on encrypted questions. I guess we will have to clarify it

image.png (53,637 bytes)   

image.png (53,637 bytes)   

Is that old or current code in your screenshot?
There might be multiple problems, of course, but we're mainly concerned about getting the customer past the current white screen of death.

Branch updated:
https://github.com/LimeSurvey/LimeSurvey/compare/5.x...bug/out-of-memory-statistics

Left fixing of some problems out of the scope as to focus on the memory stuff.
Will tackle them later on a different ticket, to be created once this is done.

Added a PR for the branch: https://github.com/LimeSurvey/LimeSurvey/pull/3111

Nice, it works, but customer didn't actually have any encrypted questions, what I can see in the responses view. Sooooo how to test??

I am actually working on some unit test scripts for it :)

Brilliant!

Initial set of tests: https://github.com/LimeSurvey/LimeSurvey/pull/3121

Cool. You wanna rebranch it to 5.x? That's where the fix is based on. We need to cherry-pick it to master later.

New PR for master: SimpleStatistics
https://github.com/LimeSurvey/LimeSurvey/pull/3132

Cool. You wanna rebranch it to 5.x?

Sorry, got delayed enhancing the test with more question types.
Will do it of course.

Status.

At the very beginning we had this branch (PR 3121):
https://github.com/LimeSurvey/LimeSurvey/tree/dev/rc_export_statistics_tests

As we were developing it we stumble with this bug.
https://bugs.limesurvey.org/view.php?id=18814
The fix is here:
https://github.com/LimeSurvey/LimeSurvey/commit/3fb81e51289b6ce90cd2d3749bcc62b2bbf699b5

So then created a new branch (deriving from the first branch) holding the tests for array and the bug fix
https://github.com/LimeSurvey/LimeSurvey/tree/dev/rc_export_statistics_array_questions_tests

All this is for v6
We are porting it to v5.

By looking at the code we see more bugs similar to 18814.
Nevertheless, we still didn't get to create its test for pushing the solution+test at the same time.

@ollehar Not sure how you prefer to merge this:

• One branch/PR at a time
• Or close the PR and just create a new one

PR for v5 including fix and tests.
https://github.com/LimeSurvey/LimeSurvey/pull/3141

This is the only PR pending for v6
https://github.com/LimeSurvey/LimeSurvey/pull/3165

I rerun failed job to have green and merge if it's OK

I believe the error is coming from master.
Merged from master but that didn't work.
Will wait for a while and try again

It was. I reverted the offending commit, please try to merge again now. :)

Does the person that worked on this fix has some survey file that they can share to test and verify the fix here https://github.com/LimeSurvey/LimeSurvey/pull/3111?
Also some testing instructions would be great! Thank you in advance ;)

The fix is very wide in terms as it updates some basic queries.
There has been auto tests created for this.

So you are saying it doesn't need testing or?

I am saying the fix is not specific as to have specific testing instructions.

Sure, but what are the steps to reproduce one use case where this happens (plus .lsa file)?

We had a customer report this error, and I could use the customer's data to reproduce it.

But which customer...

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

I merged this now, since it's too hard to test anyway.

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

Had to revert, CI got red. Please redo the PR and fix any bugs.

New v5 PR: https://github.com/LimeSurvey/LimeSurvey/pull/3231

Merged, thank you Gabriel!

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

Fixed in Release 5.6.28+230627

Need to do it for master as well, right?

Master PR: https://github.com/LimeSurvey/LimeSurvey/pull/3267

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

Fixed in Release 6.2.0+230732