View Issue Details

This bug affects 1 person(s).
 12
IDProjectCategoryView StatusLast Update
18627Bug reportsSurvey takingpublic2023-09-14 17:56
Reportertbart Assigned Totibor.pacalat  
PrioritynoneSeverityminor 
Status resolvedResolutionfixed 
Product Version3.28.x 
Summary18627: Missing SQL query errors in admin mail (SQL CODE THAT FAILED)
Description

Some participants enter their data but it cannot be saved.
I want to debug this, but I have no pointers as to what could go wrong.
People here see the same:
https://forums.limesurvey.org/forum/german-forum/124385-beim-speichern-ihrer-fragebogen-zugangsschl%C3%83%C2%BCssel-ist-ein-fehler-aufgetreten
https://forums.limesurvey.org/forum/can-i-do-this-with-limesurvey/124289-sql-code-that-failed-error-on-response-update
https://forums.limesurvey.org/forum/can-i-do-this-with-limesurvey/117903-a-response-could-not-be-saved

Basically, it boils down to
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_manager_helper.php#L5071

$message .= $this->gT("Unable to insert record into survey table"); // TODO - add SQL error?
submitfailed($this->gT("Unable to insert record into survey table"));

This SQL error is direly needed. submitfailed() supports providing the query as second parameter, but as I am no developer I don't know how to fetch the query at this point.
I am willing to write a proper bug report for the underlying issue, but I cannot without proper logging details.

I selected 3.28.x as current version, but this also affects current master on github.

Steps To Reproduce

Steps to reproduce

(Replace this text with detailed step-by-step instructions on how to reproduce the issue)

Expected result

(Write here what you expected to happen)

Actual result

(Write here what happened instead)

TagsNo tags attached.
Bug heat12
Complete LimeSurvey version number (& build)Version 3.28.19+220712
I will donate to the project if issue is resolvedNo
Browser
Database type & version10.3.37-MariaDB
Server OS (if known)
Webserver software & version (if known)
PHP Version7.4

Users monitoring this issue

tbart

Activities

gabrieljenik

gabrieljenik

2023-07-27 00:00

manager   ~76285

I believe the problem is here:
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/models/SurveyDynamic.php#L127

The error should be handled outside the model as to be able to know what happened.
Options:

@DenisChenu thoughts?

@tbart do we have reproduction steps?

gabrieljenik

gabrieljenik

2023-07-27 00:04

manager   ~76286

Another option would be to setup a new file logger on the internals and start logging these kind of things.

DenisChenu

DenisChenu

2023-07-27 07:55

developer   ~76289

I always think we must remove insertRecords and use cleand response model

We do it for update : https://github.com/LimeSurvey/LimeSurvey/blob/e4ce8389830892d6e1cd06eafec624d978d7b910/application/helpers/expressions/em_manager_helper.php#L5147
3.X : https://github.com/LimeSurvey/LimeSurvey/blob/5354af48a108cd0b20aee406641520c22204bcec/application/helpers/expressions/em_manager_helper.php#L5597

Added to use PDO : https://github.com/LimeSurvey/LimeSurvey/pull/1353

Else

I believe the problem is here:

There are 2 issue here

  1. $record->encryptSave(); didn't validate : we must validate : it fill the https://www.yiiframework.com/doc/api/1.1/CModel#getErrors-detail
  2. Maybe we can send the $e to https://www.yiiframework.com/doc/api/1.1/CModel#addErrors-detail ? in some way ?

One idea :

insertRecords do

  1. validate : if not return false ($model->errors ar filled)
  2. try/catch saveEncrypt : $model->addError('id', $e->message) : put it in id (why not)

But i think best solution is to use model directly and remove usage of insertRecords (maybe just the minimal : insert empty recorrd).

DenisChenu

DenisChenu

2023-07-27 08:03

developer   ~76290

PS : here : https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_manager_helper.php#L5167C37-L5167C37

It must not be

submitfailed(CHTml::errorSummary($oResponse)) ?

Else i found this to have the SQL : https://stackoverflow.com/a/60170086

gabrieljenik

gabrieljenik

2023-07-27 15:08

manager   ~76298

I always think we must remove insertRecords and use cleand response model

That looks more like a change for DEV.
Too risky for a bug fix from my POV.

insertRecords do
validate : if not return false ($model->errors ar filled)

We can set the parameter of saveEncrypt to true, and will do the trick.
I think it is worth it.

try/catch saveEncrypt : $model->addError('id', $e->message) : put it in id (why not)

I don't like to try/catch inside the model.
It could be fine, but

  • we don't do it anywhere else.
  • The real handling would be delegated to be done outside, not by the try catch but by the error model, so why catching it?
  • A model error is one thing (rules broken) and an exception is another, don't like to mix this concepts.

I would just not handle it inside the model, and handle it outside.

gabrieljenik

gabrieljenik

2023-07-27 15:09

manager   ~76299

Another option would be to setup a new file logger on the internals and start logging these kind of things.

You don't like the idea of only logging SQL queries which failed? Also errors?
Somehow I believe this logging option is not out of the box.

DenisChenu

DenisChenu

2023-07-27 15:19

developer   ~76300

You don't like the idea of only logging SQL queries which failed? Also errors?
Somehow I believe this logging option is not out of the box.

Why an new logger ?

Just use Yii::log ad error if you want

But : need to send the error in email.

DenisChenu

DenisChenu

2023-07-27 15:31

developer   ~76304

Else

That looks more like a change for DEV.

Maybe but i think it's the only good way to fix … or just add $e->message to the email and «basta»

gabrieljenik

gabrieljenik

2023-07-27 15:54

manager   ~76306

Last edited: 2023-07-27 15:55

Why an new logger ?
But : need to send the error in email.

People need to see the error. If it is SQL, good. But I guess error log will be better than nothgin (and easier). Probably will be enough.

Just use Yii::log ad error if you want

Problem is that by default, on live envs, nothing is logged, not even errors.
Usually even hard to find PHP errors on webserver logs (sometimes I find them, sometimes don't).
So enabling a way for error loggin on file out of the box could be a solution for this and other unkown (hard to reproduce) situations.

gabrieljenik

gabrieljenik

2023-07-27 15:55

manager   ~76307

Last edited: 2023-07-27 15:55

That looks more like a change for DEV.
Maybe but i think it's the only good way to fix … or just add $e->message to the email and «basta»

For me, 2 different tickets: BUg and Enahncement.

DenisChenu

DenisChenu

2023-07-27 17:11

developer   ~76310

then adding $e->message in email.

gabrieljenik

gabrieljenik

2023-07-27 19:19

manager   ~76316

then adding $e->message in email.

Yes, consdering then that the exception is not treated inside the model

tbart

tbart

2023-08-07 18:53

reporter   ~76466

I'd very much like to add reproduction steps, but that's a chicken and egg problem. If I don't know when something goes wrong, I am unable to trigger it :-(
If there's an easy interim patch I can add to the mailing function, I can do so and we'll probably see a little more if we hit the error by chance. It might reveal details for better reproduction as well!

gabrieljenik

gabrieljenik

2023-08-07 20:41

manager   ~76468

Will prepare something so you can apply individually and test... You were referring to that?

gabrieljenik

gabrieljenik

2023-08-18 14:56

manager   ~76643

Master: https://github.com/LimeSurvey/LimeSurvey/pull/3372

DenisChenu

DenisChenu

2023-08-30 12:02

developer   ~76782

@tbart : a way to test

Activate a survey (number 1234)
Lauch,
Move to 1st page

Rename a column of survey_1234 via the DB

guest

guest

2023-09-14 17:55

viewer   ~77053

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

guest

guest

2023-09-14 17:55

viewer   ~77054

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

tibor.pacalat

tibor.pacalat

2023-09-14 17:56

administrator   ~77055

Tested and merged fix for master.

Related Changesets

LimeSurvey: master 19d1cad3

2023-09-14 17:55:20

Gabriel Jenik


Committer: GitHub Details Diff
Fixed bug 18627: Missing SQL query errors in admin mail (SQL CODE THAT FAILED) (#3372)

Co-authored-by: Lapiu Dev <devgit@lapiu.biz>
Affected Issues
18627
mod - application/helpers/admin/import_helper.php Diff File
mod - application/helpers/expressions/em_manager_helper.php Diff File
mod - application/models/SurveyDynamic.php Diff File
mod - tests/TestBaseClass.php Diff File
add - tests/unit/models/SurveyDynamicTest.php Diff File

LimeSurvey: master 19d1cad3

2023-09-14 17:55:20

Gabriel Jenik


Committer: GitHub Details Diff
Fixed bug 18627: Missing SQL query errors in admin mail (SQL CODE THAT FAILED) (#3372)

Co-authored-by: Lapiu Dev <devgit@lapiu.biz>
Affected Issues
18627
mod - application/helpers/admin/import_helper.php Diff File
mod - application/helpers/expressions/em_manager_helper.php Diff File
mod - application/models/SurveyDynamic.php Diff File
mod - tests/TestBaseClass.php Diff File
add - tests/unit/models/SurveyDynamicTest.php Diff File

Issue History

Date Modified Username Field Change
2023-02-07 14:37 tbart New Issue
2023-02-07 14:38 tbart Issue Monitored: tbart
2023-02-07 14:38 tbart Bug heat 0 => 2
2023-07-27 00:00 gabrieljenik Note Added: 76285
2023-07-27 00:00 gabrieljenik Bug heat 2 => 4
2023-07-27 00:04 gabrieljenik Note Added: 76286
2023-07-27 07:55 DenisChenu Note Added: 76289
2023-07-27 07:55 DenisChenu Bug heat 4 => 6
2023-07-27 08:03 DenisChenu Note Added: 76290
2023-07-27 15:08 gabrieljenik Note Added: 76298
2023-07-27 15:09 gabrieljenik Note Added: 76299
2023-07-27 15:09 gabrieljenik Status new => confirmed
2023-07-27 15:19 DenisChenu Note Added: 76300
2023-07-27 15:31 DenisChenu Note Added: 76304
2023-07-27 15:54 gabrieljenik Note Added: 76306
2023-07-27 15:55 gabrieljenik Note Added: 76307
2023-07-27 15:55 gabrieljenik Note Edited: 76306
2023-07-27 15:55 gabrieljenik Note Edited: 76307
2023-07-27 17:11 DenisChenu Note Added: 76310
2023-07-27 19:19 gabrieljenik Note Added: 76316
2023-08-07 18:53 tbart Note Added: 76466
2023-08-07 18:53 tbart Bug heat 6 => 8
2023-08-07 20:41 gabrieljenik Note Added: 76468
2023-08-18 14:56 gabrieljenik Assigned To => DenisChenu
2023-08-18 14:56 gabrieljenik Status confirmed => ready for code review
2023-08-18 14:56 gabrieljenik Note Added: 76643
2023-08-30 12:00 DenisChenu Assigned To DenisChenu =>
2023-08-30 12:00 DenisChenu Status ready for code review => ready for testing
2023-08-30 12:02 DenisChenu Note Added: 76782
2023-08-30 15:52 gabrieljenik Assigned To => tibor.pacalat
2023-09-14 17:55 Changeset attached => LimeSurvey master 19d1cad3
2023-09-14 17:55 guest Note Added: 77053
2023-09-14 17:55 Changeset attached => LimeSurvey master 19d1cad3
2023-09-14 17:55 guest Note Added: 77054
2023-09-14 17:55 guest Bug heat 8 => 10
2023-09-14 17:56 tibor.pacalat Status ready for testing => resolved
2023-09-14 17:56 tibor.pacalat Resolution open => fixed
2023-09-14 17:56 tibor.pacalat Note Added: 77055
2023-09-14 17:56 tibor.pacalat Bug heat 10 => 12