[phpesp-dev] [ phpesp-Bugs-2672129 ] Copy survey reorders positions starting with 1 instead of 0

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Bugs item #2672129, was opened at 2009-03-08 00:21
Message generated for change (Settings changed) made by bishopb
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=108956&aid=2672129&group_id=8956

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Admin
Group: None
>Status: Closed
Resolution: Fixed
Priority: 5
Private: No
Submitted By: kswartz (kswartz)
Assigned to: bishop (bishopb)
Summary: Copy survey reorders positions starting with 1 instead of 0

Initial Comment:
When you initialize $pos before looping through the calls to survey_copy_questions, you set it to 1, so the first copied question has position=1. However, when you define questions for a survey, the first question's position is set to 0.

The application works fine with this bug in place UNTIL you go to copy conditions. That requires matching the questions from the new survey and the old survey, and the only way to do that is to match on the columns position and deleted. With this bug, however, the position columns do not correspond, so the question that was in position 1 in the old survey is in position 2 in the new survey.

The fix requires changing only two lines in survey_aggregate.inc:

$ diff survey_aggregate_old.inc survey_aggregate.inc
74c74
<     $pos = 1;
---
>     $pos = 0;
85c85
< function survey_copy_questions($sid, $new_sid, $pos = 1) {
---
> function survey_copy_questions($sid, $new_sid, $pos = 0) {

----------------------------------------------------------------------

Comment By: bishop (bishopb)
Date: 2009-04-13 13:42

Message:
After reviewing the code further, I believe that (a) initializing the
position to 0 during copy is correct and (b) the original survey_copy()
code (on which survey_aggregate() is based) was incorrect.

For example, in admin/include/function/survey_update.inc:326-330, we
have:
326             for($i = 0; $i < count($order); $i++) {
327                 $ord = _addslashes($order[$i]);
328                 $sql = "UPDATE
".$GLOBALS['ESPCONFIG']['question_table']." SET position=$i WHERE id=$ord
AND survey_id=$sid";
329                 execute_sql($sql);
330             }

That is a 0-based loop.  Other places in the code assume question
positions start at 0.  I have just now committed this change to trunk.

I will commit SFID 2669428 ("Copying an existing survey does not copy
conditions") separately.

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2009-04-08 03:34

Message:
Basically, that's true. The point is that when you copy the questions from
one survey to another, the position values must remain the same. It's okay
if some are 0 and some are 1, just as long as the source and destination
match.

----------------------------------------------------------------------

Comment By: Franky Van Liedekerke (liedekef)
Date: 2009-03-23 16:31

Message:
Well, I don't think it matters :-) Or more: to fix the ones starting at "1"
would be very troublesome ... the damage there is already done (if you have
conditions in place).
But after the fix is in place, it wouldn't matter: you start at position 0
in source and destination, even if position 0 is not defined it should
still be ok, no?

Franky

----------------------------------------------------------------------

Comment By: kswartz (kswartz)
Date: 2009-03-08 23:45

Message:
I haven't looked through the code yet for the initial "0" position, but I
can say that every survey I've created has 0 as the value of position for
the first question. I would expect that this gets set when using the
section to reorder the questions in creating a new survey.

One question (no pun intended) is whether or not it starts with "0" if you
do NOT reorder the questions; i.e.: if you only create a question and exit.
However, I think it doesn't really matter, and here's why...

As soon as we introduce the code to copy conditions (which is dependent on
this change), it's going to rely on the fact that positions between
original and copied surveys always start with the same value. Today, some
surveys will start with 0 and some start with 1. So the next release will
have to include an upgrade script that cleans this up, by updating the
position column so that ALL surveys start with the same value. I'm inclined
to say that value should be 0, based on what I'm seeing from the surveys
I've created from scratch and your last comment, bishopb.

----------------------------------------------------------------------

Comment By: bishop (bishopb)
Date: 2009-03-08 15:50

Message:
I am inclined to think that 0 is the correct position, and since I was the
most recent to work in that area of the (really old) code, I'll take on
this issue.

kswartz: are you seeing the initial definition of 0 somewhere specifically
in the code, or only as a post facto artifact in the generated rows?

----------------------------------------------------------------------

Comment By: Franky Van Liedekerke (liedekef)
Date: 2009-03-08 13:27

Message:
Hmmm ... I don't remember as well, probably the oldest programmer for that
part was way before me ...

Franky

----------------------------------------------------------------------

Comment By: bishop (bishopb)
Date: 2009-03-08 08:15

Message:
survey_aggregate() is a generalization of the original survey_copy() code,
and in fact the survey_copy_questions() helper function is a direct
descendant of the original survey_copy().

In the original survey_copy(), $pos was initialized to 1:

 65     $pos=1;
 66     array_shift($question_fields);
 67     while($question = fetch_row($result)) {
 68         $result->MoveNext();
 69         $tid = $question['type_id'];
 70         $qid = $question['id'];
 71         // fix some fields first
 72         $question['survey_id'] = $new_sid;
 73         $question['position'] = $pos++;

So, I can say that if this is a bug, it's existed for a while.  Is anyone
here familiar with the older code from survey_copy() that can comment on
this?

----------------------------------------------------------------------

Comment By: kswartz (kswartz)
Date: 2009-03-08 00:22

Message:
File Added: survey_aggregate.inc

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=108956&aid=2672129&group_id=8956