Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey #3789

Shnoulle · 2024-03-12T09:02:26Z

Dev: simple for question
Dev: add specific array for conditions (again array dual scale)
Dev: avoid multiple call of same question attributes

Dev: simple for question Dev: add specific array for conditions (again array dual scale)

lajosarpad

Looks good. I have added some bug fixes for issues I encountered while testing. Those issues already existed before your changes, but since they broke the test, I fixed them, but for some reason they do not appear in the changes of this PR.

These are the changes I propose:

I do not understand why the changes are not appearing here even though I pushed them

Shnoulle · 2024-03-13T07:22:41Z

application/controllers/admin/PrintableSurvey.php

@@ -238,7 +247,7 @@ public function index($surveyid, $lang = null, $bReturn = false)
                                // cqid == 0  ==> token attribute match
                                $tokenData = getTokenFieldsAndNames($surveyid);
                                preg_match('/^{TOKEN:([^}]*)}$/', (string) $distinctrow['cfieldname'], $extractedTokenAttr);
-                                $sExplanation .= "Your " . $tokenData[strtolower($extractedTokenAttr[1])]['description'] . " ";
+                                $sExplanation .= "Your " . ($tokenData[strtolower($extractedTokenAttr[1])]['description'] ?? "") . " ";


If this is null : it's an issue with your Survey data …

@Shnoulle you may be correct, but the page should nevertheless not crash even then.

Shnoulle · 2024-03-13T07:23:20Z

application/controllers/admin/PrintableSurvey.php

@@ -631,7 +646,7 @@ public function index($surveyid, $lang = null, $bReturn = false)
                                }
                            }

-                            if ($arQuestion['other'] == 'Y') {
+                            if (($arQuestion['other'] == 'Y') && isset($qidattributes["printable_help"])) {


qidattributes["printable_help"] not set ? What question type ? It muts be always set to default : else it's another issue.

@Shnoulle if it's not set, for whatever reason, then we will have an error. We should not assume it is set unless we are absolutely sure. If we rely on it being set and it is not set, then - issue or not - we have a page crash. Instead, we should fail only for the question if such is the situation.

@Shnoulle if it's not set, for whatever reason, then we will have an error. We should not assume it is set unless we are absolutely sure. If we rely on it being set and it is not set, then - issue or not - we have a page crash. Instead, we should fail only for the question if such is the situation.

But you fix another issue in a dev commit. I can not reproduce your issue, maybe there are another issue : we must find and track the original issue !

@Shnoulle We should not assume it is set unless we are absolutely sure.

If it's not set : it's clearly an issue : code is done to set ALL attributes default value. Set in the database or not.

It's not an issue in this code, but in getQuestionAttribute : if getQuestionAttribute don't set default value : it's a BIG issue !

Shnoulle · 2024-03-13T07:24:30Z

Looks good. I have added some bug fixes for issues I encountered while testing. Those issues already existed before your changes, but since they broke the test, I fixed them, but for some reason they do not appear in the changes of this PR.

I do not understand why the changes are not appearing here even though I pushed them

You mean bugfix on master ? Because it's appear on the current commit and not in master

I use debug 2 on PHP8.1 with Sample survey for testing: i don't have any issue . Maybe you must check your survey or if there are another issue

I really think we must not fix such issue for example existing QuestionAttribute must always be set to default (printablesurvey) : if not it's an issue in getQuestionAttribute

Shnoulle · 2024-03-13T07:25:28Z

Maybe you need to https://manual.limesurvey.org/Check_data_integrity/en for testing ?

Shnoulle · 2024-03-13T07:27:55Z

application/controllers/admin/PrintableSurvey.php

                            }
                            //$distinctrow
                            $x++;
                        }
                        $s++;
                    }

-                    $qinfo = LimeExpressionManager::GetQuestionStatus($arQuestion['qid']);
+                    //Defaulting to dummy array to avoid crashes when qinfo is not found for this question
+                    $qinfo = LimeExpressionManager::GetQuestionStatus($arQuestion['qid']) ?? [


If qinfo return null : there are an issue in ExpressionManager not in this commit !
Or in your survey (old question not deleted)
Or in session reset

@Shnoulle I did not claim that your commit introduced the errors. My claim is that I have seen errors testing with a Survey that was exported from an actual customer's instance. We need to make sure that our code is bullet-proof.

@Shnoulle I did not claim that your commit introduced the errors. My claim is that I have seen errors testing with a Survey that was exported from an actual customer's instance. We need to make sure that our code is bullet-proof.

Did you try with master : if issues exist in master : it must be fixed in master NOT in another commit …

lajosarpad · 2024-03-13T10:47:29Z

Looks good. I have added some bug fixes for issues I encountered while testing. Those issues already existed before your changes, but since they broke the test, I fixed them, but for some reason they do not appear in the changes of this PR.
I do not understand why the changes are not appearing here even though I pushed them

You mean bugfix on master ? Because it's appear on the current commit and not in master

I use debug 2 on PHP8.1 with Sample survey for testing: i don't have any issue . Maybe you must check your survey or if there are another issue

I really think we must not fix such issue for example existing QuestionAttribute must always be set to default (printablesurvey) : if not it's an issue in getQuestionAttribute

It did not appear here yesterday, but for some reason it appears now.

I will check for Survey errors, but the code should never crash due to this reason. So if you would like to move these fixes someplace else instead, then I'm fine with that, as long as we have the issue fixed and no new issues are created in the process.

Shnoulle · 2024-03-13T10:54:04Z

we have the issue fixed and no new issues are created in the process.

Did you try with master : if issues exist in master : it must be fixed in master NOT in another commit …

lajosarpad · 2024-03-13T12:56:34Z

Maybe you need to https://manual.limesurvey.org/Check_data_integrity/en for testing ?

Did it. Here's the result:

git hash: `05a9d4e`

559767: crashes with old version
581394: crashes with old version
362779: works
764586: crashes with old version
793828: crashes with old version
775547: works

git hash: `e2c4a33`

559767: works
581394: works
362779: works
764586: works
793828: works
775547: works

Explanation:

Those ids are survey ids in my local database, basically results of the query of

select sid, count(*) from lime_conditions join lime_questions on lime_conditions.cqid = lime_questions.qid group by sid order by count(*) desc limit 0, 10;

How did I test:

        $obj = new PrintableSurvey();
        $obj->index(559767);
        exit;

(and I changed the id to be passed as a parameter)

Shnoulle · 2024-03-13T13:40:55Z

Did it. Here's the result:

The question is with master, not with your fix.

lajosarpad · 2024-03-13T16:37:39Z

Did it. Here's the result:

The question is with master, not with your fix.

The issue was not created by your work. It already existed. But we cannot accept a PR that crashes the page.

I understand that you want to create a separate ticket for this purpose, but I'm not sure it is a good idea to do so, because then we create prerequisites for this and thusly we postpone the completion of speed improvement, which, as a result will postpone the switch from LS5 to LS6.

These fixes are making the code less volatile and more reliable overall.

I have no issues if you create a new ticket in the future with the purpose of fixing these issues, but you will need to detect the problems during your testing then. In this case, they were detected while I tested with your work with some surveys and they were crashing. the page.

It would be quite the sacrifice in terms of time needed to create a new ticket, wait for approval, create a new PR, have it accepted, merge it into master, merge master back to this one, fix the merge conflicts that will not exist if we fix the bugs when we see a broken branch. All these prerequisites that we create this way postpones the completion of the task

Shnoulle · 2024-03-13T17:15:55Z

…

It's not about time issue , there are 2 issues on this method (PR with multiple issue)

here there are 2 problems which are, in my opinion, specific and which require an understanding of why there is a problem. Otherwise, the problem will reappear somewhere else.
We can not follow when something was update and why

If you don't want to create mantis issue : create a PR for each and merge it after @tiborpacalat test it.
I can make a review, but here : i didn't accept

https://github.com/LimeSurvey/LimeSurvey/pull/3789/files#r1522656350 : must fix the way we get token attributes
https://github.com/LimeSurvey/LimeSurvey/pull/3789/files#r1522662524 : LimeExpressionManager::GetQuestionStatus muts NOT return an NULL value if the question exist in the survey :

LimeSurvey/application/helpers/expressions/em_manager_helper.php

Line 6679 in 4f7ecb9

if (isset($LEM->currentQset[$qid])) {
https://github.com/LimeSurvey/LimeSurvey/pull/3789/files#r1522657115 here : maybe it's related to quetiontype ? But if an attribute exist he must be always set. If printable_help didn't exist for a question : it must be added (even Equation need printable_help)

Shnoulle · 2024-03-13T17:28:14Z

…
2. We can not follow when something was update and why

This is the worst part : when you try to find what this line was added, if we can improve and have a cleaner code : know the reason of each update are really great !
And GIT history and blame really help for this !

Shnoulle added 2 commits March 12, 2024 09:49

Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey

0436779

Dev: simple for question Dev: add specific array for conditions (again array dual scale)

Dev: avoid multiple call of same question attributes

05a9d4e

Shnoulle added Needs code review Needs testing labels Mar 12, 2024

Shnoulle requested a review from lajosarpad March 12, 2024 09:46

MW-1040: Applied some fixes

9eacbb8

lajosarpad approved these changes Mar 13, 2024

View reviewed changes

Added a comment

1ed8ce7

Shnoulle commented Mar 13, 2024

View reviewed changes

MW-1040: Fixed a typo

e2c4a33

kevin-foster-uk approved these changes Mar 21, 2024

View reviewed changes

kevin-foster-uk merged commit 570b810 into master Mar 21, 2024
20 checks passed

kevin-foster-uk deleted the bug/19452_questionAttribute_PrintableSurvey branch March 21, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey #3789

Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey #3789

Shnoulle commented Mar 12, 2024

lajosarpad left a comment •

edited

Shnoulle Mar 13, 2024

lajosarpad Mar 13, 2024

Shnoulle Mar 13, 2024

lajosarpad Mar 13, 2024

Shnoulle Mar 13, 2024

Shnoulle Mar 13, 2024

Shnoulle commented Mar 13, 2024 •

edited

Shnoulle commented Mar 13, 2024

Shnoulle Mar 13, 2024 •

edited

lajosarpad Mar 13, 2024

Shnoulle Mar 13, 2024

lajosarpad commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

lajosarpad commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

lajosarpad commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey #3789

Dev #19452: Improve usage of getQuestionAttributes PrintableSurvey #3789

Conversation

Shnoulle commented Mar 12, 2024

lajosarpad left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shnoulle commented Mar 13, 2024 • edited

Shnoulle commented Mar 13, 2024

Shnoulle Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lajosarpad commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

lajosarpad commented Mar 13, 2024

git hash: 05a9d4e

git hash: e2c4a33

Explanation:

Shnoulle commented Mar 13, 2024

lajosarpad commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

Shnoulle commented Mar 13, 2024

lajosarpad left a comment •

edited

Shnoulle commented Mar 13, 2024 •

edited

Shnoulle Mar 13, 2024 •

edited

git hash: `05a9d4e`

git hash: `e2c4a33`