Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable LS to import answers (.vv) between different installations/versions #138

Merged
merged 4 commits into from Oct 7, 2013

Conversation

Grapsus
Copy link
Contributor

@Grapsus Grapsus commented Oct 2, 2013

enable LS to import answers (.vv) between different installations/versions

@Grapsus
Copy link
Contributor Author

Grapsus commented Oct 2, 2013

more details :
http://bugs.limesurvey.org/view.php?id=8225

@mfaber
Copy link
Contributor

mfaber commented Oct 4, 2013

Thanks, that makes it clearer.
While this works indeed well in your test case, I don't think it's a good idea to weaken the safety/specificity of the new import method and offer it as a general solution to the users. There is just no safeguard against mismatching of questions.
Imagine a user importing first a survey (eg. in txt-format) and something goes wrong during import (eg. one question is not imported). When the user now imports data, he will probably end up with a lot of garbage in the db.

Also, I would generally be careful importing surveys and data back and forth between versions as there have been quite some changes to the data structure. These are handled when updating a system (and the db) from 192-->2.00 and 2.00-->2.05 but this is (usually) not the case when importing.lss/.txt-files...same is true for data.

@Grapsus
Copy link
Contributor Author

Grapsus commented Oct 4, 2013

There is no safety or specificity in the new import system. It lets you import anything and if fields are missing, the user ends up with blank answers.

Can we at least display a warning if some keys are missing in $aKeyForFieldNames when the import is performed ?
Can we also add a checkbox "force import" and use my code if this checkbox is enabled ?

I really don't get how you can refuse my patch (or an equivalent solution) while the official solution today, when an import doesn't work automatically, is to edit CSV files by hand...

@Shnoulle
Copy link
Collaborator

Shnoulle commented Oct 5, 2013

Hi,

In 2.05 : you can:

  • Import first group from a survey and another from another survey.

WIth a 'order' solution : you broke this system.

You can too import some Var from one survey and leave other NULL. Import only know var.

  • First test on SGQA
  • Second test on EM code

@Shnoulle
Copy link
Collaborator

Shnoulle commented Oct 5, 2013

Please see : http://bugs.limesurvey.org/view.php?id=5740

@Grapsus
Copy link
Contributor Author

Grapsus commented Oct 5, 2013

I really don't understand your attitude here.

My code adds an else block for the case when the current import system doesn't work. Without this else block the import goes on, and answers for which no CSV fields were found are imported as blank and not even a warning is issued to the user. Can you please explain (with facts) how it can break anything ? How could a system that ignores import errors be made more broken by a fallback solution ?

In my opinion, #5740 isn't fixed at all because the test files provided by the person who reported the bug are still not working with LS 2.05. I was planning to re-open #5740 but it doesn't seem possible.
In #5740 you wrote "All csv can be used." which is wrong, only CSV generated with LS 2.05 can be used.
Then "Need more control on file validity but tested." so clearly nothing checks if the import system found columns for all questions.

All I want is to fix the situation when a user exported hundreds of lss and vv files from LS <2.05 and tries to import them into a new installation. Without my patch, this user has to edit all of his vv files by hand.

In case you write code to properly check imports before committing data, just add a checkbox "force import" to enable my fallback solution for users who know what they are doing.

@Shnoulle
Copy link
Collaborator

Shnoulle commented Oct 7, 2013

Stay calm, i don't refuse , i don't have this power ..... i'm not the (only) one who decide. I just say it can break some system .

If it can break some system, it must be an option (and think this option must be deactivated by default).

And for 2.00 > 2.05 update and have all survey set : you can already use "lsa system" it' surely better. And the best are : copy DB/ Update DB with LS incorporated system.

@SamMousa
Copy link
Contributor

SamMousa commented Oct 7, 2013

Grapsus, to me your solution looks fine. I just have a few questions:

Do you, or could you, check the total number of columns when importing such a file?
Does your solution work as you committed it:

  • There seems to be some errors in your variable naming with the csv index variable.
  • When I read the code it seems your code will only be executed the first iteration of the foreach loop, after which it will always assign the same number to the mapping array.

If you respond quickly enough I'd like to merge this into 2.05 before the feature freeze of Oct 8th.

Cheers!

@Grapsus
Copy link
Contributor Author

Grapsus commented Oct 7, 2013

Hey,

The code I posted seems to work.
Several people tested it with many vv files from LS 1.92 and LS 2.00 with various parameters (dates on/off, tokens on/off etc.) and it took a few adjustments to get it right.

The algorithm works as follows :

  • on the first iteration only (hence the isset() calls)
    • find the index where answers start in the destination table and store it in $table_ans_start_index
    • find the index where answers start in the CSV file and store it in $csv_ans_start_index
  • on each iteration :
    • map the field to the right position according to its offset, $table_ans_start_index and $csv_ans_start_index

What I can do and test today if you answer quickly :

  • make my algorithm an option (lets call it "force import") and disable it by default
  • I could check if the number of columns is ok, but there is a slight problem : your CSV parser often adds empty columns at the end, so the test may fail without reason. Maybe we can test that at least the CSV contains enough fields (>=) to fill the destination table ?

{
if(preg_match('/^\d+X\d+X\d+/', $name))
{
$ans_start_index = $i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be $csv_ans_start_index ?

@SamMousa
Copy link
Contributor

SamMousa commented Oct 7, 2013

Okay:

  • Make it optional.
  • Add some documentation to the wiki (does not have to be today).
  • Add minimum column check.
  • Add @todo for exact column check. (We should switch to the built-in csv parser in the future anyway).

Thanks in advance for the contribution!

alex added 2 commits October 7, 2013 11:48
Made fallback import optionnal and disabled by default.
@Grapsus
Copy link
Contributor Author

Grapsus commented Oct 7, 2013

Hey,

  • Sorry, there was indeed a typo in a variable name, I introduced it when cleaning code before pushing. It was corrected and code re-tested.
  • I made my algo optional, disabled by default.
  • I added code to check that at least one answer could be imported. It will prevent totally wrong imports from being saved to database. I don't think we will ever be able to check the exact number of columns, as Shnoulle said, the user may want to import only one group of questions, in that case (number of imported answer columns) < (number of columns in destination table) and it is ok.

I hope it's all good, tell me if you see any other correction before the feature freeze.

SamMousa added a commit that referenced this pull request Oct 7, 2013
enable LS to import answers (.vv) between different installations/versions
@SamMousa SamMousa merged commit ca21fdc into LimeSurvey:2.05 Oct 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants