Moodle

question->questiontext and question->generalfeedback should always be strings

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 2.2.5, 2.3.2, 2.4
  • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
  • Component/s: Questions
  • Labels:
  • Testing Instructions:
    Hide

    This code doesn't bring any new feature or fix any non working feature. It's just code improvement as asked by Eloy in his comment to MDL-35147
    So we are only testing there is no regression
    All tests should be conducted with developer debugging set to enabled to verify that no error or warning is displayed during imports.

    TEST IMPORT OF AN "ASSESSEMENT QTI" .ZIP ARCHIVE WITH SOME IMAGES

    Download the testgen_images.ZIP file attached to MDL-25492 tracker issue.
    Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image.
    This file should import 1 MCQ question with name : Describe this image. Verify that both in preview and in editing the question text and each choice display a book image (the right answer is not what you may think first).
    Looking at the question text the image src should be a valid draftfile url ending with 

    ppg__questions with image1127111308__f1q1g1.jpg


    Looking at the choice with the image that looks the same, the img src should be a valid draftfile url ending with 

    ppg__questions with image1127111308__f1q1g3.jpg

    TEST IMPORT OF A "QUESTION POOL" .ZIP ARCHIVE WITH SOME IMAGES

    Download the 6.4 greeks.zip file from http://moodle.tccsa.net/tccsa2/mod/resource/view.php?id=635 1072 ko. Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image.
    This file should import 12 MCQ questions. One of these questions has a name that display as * but is in fact * followed by a non breaking space entity. This name is wierd but is the expected result as we don't want any image included in question's names.
    Preview this question, it should diplay an image with some text in the question's text (text in the image begin by atlas), verify it is working as expected, the right answer is Greeks.
    Open the same question for editing and look at the html source of the question text. The image src should be a valid draftfile url with 

    ppg__examview__6-4-greeks__mc009-1.jpg

    as last part, alt should be just "mc009-1.jpg".

    TEST THAT LESSON IMPORT IS STILL WORKING FOR ALL FORMATS

    As lesson don't support images import for now we only tests that the sample included in Moodle are correctly imported
    I don't think it is necessary to test all these files because if there is a regression, most likely all formats should be broken
    Create a lesson and try to import some questions from some of these files :

    • try to import the question/format/aiken/tests/fixtures/questions.aiken.txt
    • try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat file
    • try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_qti.dat file
    • try to import the question/format/examview/tests/fixtures/examview_sample.xml file
    • try to import the question/format/gift/tests/fixtures/questions.gift.txt file

    TEST THAT QUESTION BANK IMPORT IS STILL WORKING

    Try to import questions from some of the files listed in the previous step in a category of the question bank.

    Show
    This code doesn't bring any new feature or fix any non working feature. It's just code improvement as asked by Eloy in his comment to MDL-35147 So we are only testing there is no regression All tests should be conducted with developer debugging set to enabled to verify that no error or warning is displayed during imports. TEST IMPORT OF AN "ASSESSEMENT QTI" .ZIP ARCHIVE WITH SOME IMAGES Download the testgen_images.ZIP file attached to MDL-25492 tracker issue. Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image. This file should import 1 MCQ question with name : Describe this image. Verify that both in preview and in editing the question text and each choice display a book image (the right answer is not what you may think first). Looking at the question text the image src should be a valid draftfile url ending with ppg__questions with image1127111308__f1q1g1.jpg Looking at the choice with the image that looks the same, the img src should be a valid draftfile url ending with ppg__questions with image1127111308__f1q1g3.jpg TEST IMPORT OF A "QUESTION POOL" .ZIP ARCHIVE WITH SOME IMAGES Download the 6.4 greeks.zip file from http://moodle.tccsa.net/tccsa2/mod/resource/view.php?id=635 1072 ko. Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image. This file should import 12 MCQ questions. One of these questions has a name that display as * but is in fact * followed by a non breaking space entity. This name is wierd but is the expected result as we don't want any image included in question's names. Preview this question, it should diplay an image with some text in the question's text (text in the image begin by atlas), verify it is working as expected, the right answer is Greeks. Open the same question for editing and look at the html source of the question text. The image src should be a valid draftfile url with ppg__examview__6-4-greeks__mc009-1.jpg as last part, alt should be just "mc009-1.jpg". TEST THAT LESSON IMPORT IS STILL WORKING FOR ALL FORMATS As lesson don't support images import for now we only tests that the sample included in Moodle are correctly imported I don't think it is necessary to test all these files because if there is a regression, most likely all formats should be broken Create a lesson and try to import some questions from some of these files : try to import the question/format/aiken/tests/fixtures/questions.aiken.txt try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat file try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_qti.dat file try to import the question/format/examview/tests/fixtures/examview_sample.xml file try to import the question/format/gift/tests/fixtures/questions.gift.txt file TEST THAT QUESTION BANK IMPORT IS STILL WORKING Try to import questions from some of the files listed in the previous step in a category of the question bank.
  • Affected Branches:
    MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
  • Fixed Branches:
    MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
  • Pull from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

Description

As part of MDL-25492 I changed question->questiontext and question->generalfeedback to be arrays when question are imported using the blackboard_six question import format.
This needed adjustments both in question/format.php and in mod/lesson/format.php (see MDL-35147)
As Eloy pointed out in MDL-35147 comments :
"But I don't think it's the correct solution. IMO the lesson importer should be receiving constant structures. And this seems a nasty exception.
And worse, I don't know which impact can have within qbanks importer, or how the hell that importer is able to handle both strings and arrays."
So this proposal is to find a way so that question object has a constant structure.
This would permit to remove nearly entirely Eloy commit http://fisheye.moodle.org/changelog/Moodle?cs=1c3b1f7aee31ef897c08d16e1e4c094a69d727d5 becaus eit would not be necessary anymore.

Issue Links

has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-35147 Question import into lesson is broken

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-25492 Blackboard V6+ question import is broken

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-37147 Question choices html editor field ridiculously small.

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-30001 WEBCT Import does not work for 2,0

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jean-Michel Vedrine added a comment -

Hello Tim
If you can have a look at this. I think it is ready for peer-review.
Of course, this is really minor and I know you are taking some well deserved holidays. So better to wait your return !
The unit tests are OK and I tested to import some files with images with no apparent problem.

Show
Jean-Michel Vedrine added a comment - Hello Tim If you can have a look at this. I think it is ready for peer-review. Of course, this is really minor and I know you are taking some well deserved holidays. So better to wait your return ! The unit tests are OK and I tested to import some files with images with no apparent problem.
Hide
Permalink
Michael de Raadt added a comment -

I'll leave this to Tim to review, but it would be good if you could add some testing instructions.

Tim is away until the end of the week, so he might not respond for a while.

Show
Michael de Raadt added a comment - I'll leave this to Tim to review, but it would be good if you could add some testing instructions. Tim is away until the end of the week, so he might not respond for a while.
Hide
Permalink
Jean-Michel Vedrine added a comment -

Hello Michael,
Thanks for looking at this issue.
Yes I know Tim is away until the end of the week, but as this is really a minor issue, this can wait.
I wanted to first have Tim's comments before doing branches for Moodle 2.3 and moodle 2.2 and adding testing instructions.
Testing will be to verify that I didn't created regressions as there is no new functionnalities only code cleaning.
But we need to ensure I didn't break anything in question bank and lesson import (the fact that the unit tests I did for the blackboard_six import format are still OK is good, but it is not enough to ensure this change is working)

Show
Jean-Michel Vedrine added a comment - Hello Michael, Thanks for looking at this issue. Yes I know Tim is away until the end of the week, but as this is really a minor issue, this can wait. I wanted to first have Tim's comments before doing branches for Moodle 2.3 and moodle 2.2 and adding testing instructions. Testing will be to verify that I didn't created regressions as there is no new functionnalities only code cleaning. But we need to ensure I didn't break anything in question bank and lesson import (the fact that the unit tests I did for the blackboard_six import format are still OK is good, but it is not enough to ensure this change is working)
Hide
Permalink
Tim Hunt added a comment -

+1 from me.

Please cherry-pick and add testing instructions, then this can be submitted for integration.

Show
Tim Hunt added a comment - +1 from me. Please cherry-pick and add testing instructions, then this can be submitted for integration.
Hide
Permalink
Jean-Michel Vedrine added a comment -

Cherry picked to MOODLE_22_STABLE (without the unit tests) and MOODLE_23_STABLE.
I will add some testing instructions. I wonder how much of the testing process of MDL-25492 and MDL-35147 I should ask to redo here ? We sure need to test that image import in question bank is not broken, but this change also has the potential to create regressions in all question bank and lesson import as mod/lesson format.php and question/format.php are changed ? Maybe it's enough if we test that some fixtures files import is still OK in question bank import and in lesson import ?

Show
Jean-Michel Vedrine added a comment - Cherry picked to MOODLE_22_STABLE (without the unit tests) and MOODLE_23_STABLE. I will add some testing instructions. I wonder how much of the testing process of MDL-25492 and MDL-35147 I should ask to redo here ? We sure need to test that image import in question bank is not broken, but this change also has the potential to create regressions in all question bank and lesson import as mod/lesson format.php and question/format.php are changed ? Maybe it's enough if we test that some fixtures files import is still OK in question bank import and in lesson import ?
Hide
Permalink
Tim Hunt added a comment -

Thanks Jean-Michel.

Show
Tim Hunt added a comment - Thanks Jean-Michel.
Hide
Permalink
Jean-Michel Vedrine added a comment -

Rebased all branchs. I am a little worried this didn't get into 2.2.6 and 2.3.3.

Show
Jean-Michel Vedrine added a comment - Rebased all branchs. I am a little worried this didn't get into 2.2.6 and 2.3.3.
Hide
Permalink
Dan Poltawski added a comment -

Hi Jean-Michel/Tim,

Is this improvement safe for back-porting to the stable branches? It looks to me that some non-core format could potentially be relying on this behaviour?

I wonder if it'd be safer to go master only on this to be more cautious. What do you think?

Show
Dan Poltawski added a comment - Hi Jean-Michel/Tim, Is this improvement safe for back-porting to the stable branches? It looks to me that some non-core format could potentially be relying on this behaviour? I wonder if it'd be safer to go master only on this to be more cautious. What do you think?
Hide
Permalink
Jean-Michel Vedrine added a comment -

Hello Dan,
Thanks for having a look at this.
Yes I think it is quite safe because it only correct code I introduced myself a few months ago, so this is really a bugfix not a new feature.
In fact as you can see from the corrections in lesson I am in fact removing (not adding) some changes done when I allowed question formats to import images directly back in July-August 2011.
When I did this changes I allowed the question structure questiontext and generalfeedback fields to be either strings or arrays. As Eloy pointed out this was not very good. This change revert them to be strings in all cases as they have been for years.
The only way I see it could break something is the very unlikely case where somebody has modified a non core format after last summer to use the mechanism I introduced, but :

  • I don't know of any question import format (no one in the plugin database, and no discussion about one in the quiz forum) that import images apart the ones I am currently working on myself : my work to fix the webct import format in MDL-30001 and a "gift with media files" new format see https://github.com/jmvedrine/moodle-qformat_giftmedia. Both are based on the fact that MDL-36243 is integrated, so if it is only integrated in master this will really be a pain for me and Moodle users because no Moodle user would be able to use these formats before June 2013 !!
  • question import formats is (sadly) a very quiet area of Moodle : a lot of formats had been broken for years when I began working on them and very few Moodle developpers work on that area
  • This change in the code was left unpublished and no mention of it was added to the readme file (sorry !) so a developer would have to look in the code to use it. As I said very unlikely
  • If a third party format is using this behaviour, it's only a small job (look at the changes in question/format/blackboard_six/formatqti.php or question/format/blackboard_six/formatpool.php) and a few lines of code to make them use the new behaviour

So please consider integrating this bugfix in all branchs. Thanks a lot.

Show
Jean-Michel Vedrine added a comment - Hello Dan, Thanks for having a look at this. Yes I think it is quite safe because it only correct code I introduced myself a few months ago, so this is really a bugfix not a new feature. In fact as you can see from the corrections in lesson I am in fact removing (not adding) some changes done when I allowed question formats to import images directly back in July-August 2011. When I did this changes I allowed the question structure questiontext and generalfeedback fields to be either strings or arrays. As Eloy pointed out this was not very good. This change revert them to be strings in all cases as they have been for years. The only way I see it could break something is the very unlikely case where somebody has modified a non core format after last summer to use the mechanism I introduced, but : I don't know of any question import format (no one in the plugin database, and no discussion about one in the quiz forum) that import images apart the ones I am currently working on myself : my work to fix the webct import format in MDL-30001 and a "gift with media files" new format see https://github.com/jmvedrine/moodle-qformat_giftmedia . Both are based on the fact that MDL-36243 is integrated, so if it is only integrated in master this will really be a pain for me and Moodle users because no Moodle user would be able to use these formats before June 2013 !! question import formats is (sadly) a very quiet area of Moodle : a lot of formats had been broken for years when I began working on them and very few Moodle developpers work on that area This change in the code was left unpublished and no mention of it was added to the readme file (sorry !) so a developer would have to look in the code to use it. As I said very unlikely If a third party format is using this behaviour, it's only a small job (look at the changes in question/format/blackboard_six/formatqti.php or question/format/blackboard_six/formatpool.php) and a few lines of code to make them use the new behaviour So please consider integrating this bugfix in all branchs. Thanks a lot.
Hide
Permalink
Tim Hunt added a comment -

+1 from me also. Please backport if you are happy to do so.

Show
Tim Hunt added a comment - +1 from me also. Please backport if you are happy to do so.
Hide
Permalink
Dan Poltawski added a comment -

Thanks for clarifying.

I've integrated this to master, 24, 23 and 22.

Show
Dan Poltawski added a comment - Thanks for clarifying. I've integrated this to master, 24, 23 and 22.
Hide
Permalink
Jean-Michel Vedrine added a comment -

Thanks Tim, Thanks Dan

Show
Jean-Michel Vedrine added a comment - Thanks Tim, Thanks Dan
Hide
Permalink
Mark Nelson added a comment -

Works as expected. Passing. Thanks for another great contribution Jean-Michel.

Show
Mark Nelson added a comment - Works as expected. Passing. Thanks for another great contribution Jean-Michel.
Hide
Permalink
Jean-Michel Vedrine added a comment -

Thanks Mark

Show
Jean-Michel Vedrine added a comment - Thanks Mark
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

Closing, ciao

Show
Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: