Moodle
  1. Moodle
  2. MDL-38390

Blackboard ZIP import with multiple pool files should treat each file as a separate category

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Current behaviour:

      1. Import the bb-test.zip sample as Blackboard format into a course ensuring the "Get category from file" option is selected and the "Import category" is "Default for Course Name Here".
      2. Go to Course administration > Question bank > Categories.
      3. Observe no new categories have been created underneath "Default for Course Name Here".
      4. Go to Course administration > Question bank > Questions.
      5. Observe that four questions named "res0000x qy" have been imported to the "Default for Course Name Here".

      New behaviour:

      1. Import the bb-test.zip sample as Blackboard format into a course ensuring the "Get category from file" option is selected and the "Import category" is "Default for Course Name Here"..
      2. Go to Course administration > Question bank > Categories.
      3. Observe that two categories have been created under "Default for Course Name Here": "res00000 title" and "res00001 title".
      4. Go to Course administration > Question bank > Questions.
      5. Select the "res00000 title" category and observe that two new questions named "res00000 q1" and "res00000 q2" have been imported.
      6. Select the "res00001 title" category and observe that two new questions named "res00001 q1" and "res00001 q2" have been imported.
      Show
      Current behaviour: Import the bb-test.zip sample as Blackboard format into a course ensuring the "Get category from file" option is selected and the "Import category" is "Default for Course Name Here ". Go to Course administration > Question bank > Categories. Observe no new categories have been created underneath "Default for Course Name Here ". Go to Course administration > Question bank > Questions. Observe that four questions named "res0000x qy" have been imported to the "Default for Course Name Here ". New behaviour: Import the bb-test.zip sample as Blackboard format into a course ensuring the "Get category from file" option is selected and the "Import category" is "Default for Course Name Here ".. Go to Course administration > Question bank > Categories. Observe that two categories have been created under "Default for Course Name Here ": "res00000 title" and "res00001 title". Go to Course administration > Question bank > Questions. Select the "res00000 title" category and observe that two new questions named "res00000 q1" and "res00000 q2" have been imported. Select the "res00001 title" category and observe that two new questions named "res00001 q1" and "res00001 q2" have been imported.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      48308

      Description

      For Zipped Blackboard 6+ format question files having multiple res000xx.dat pool files within, currently all questions get imported into the one category. As each pool file may represent a separate category, my patch interprets each pool as an individual category for importing. The title of the category comes from the name of the pool. The existing behaviour of importing into the one category can still be achieved by disabling the "Get category from file" import option.

      Attached example ZIP has two pools each with two questions to demonstrate.

        Activity

        Hide
        Jean-Michel Vedrine added a comment -

        Assigning to Jonathon

        Show
        Jean-Michel Vedrine added a comment - Assigning to Jonathon
        Hide
        Jonathon Fowler added a comment -

        Unit tests fixed, and I added an extra one to test the category.

        Show
        Jonathon Fowler added a comment - Unit tests fixed, and I added an extra one to test the category.
        Hide
        Michael de Raadt added a comment -

        Thanks for working on that, Jono.

        Show
        Michael de Raadt added a comment - Thanks for working on that, Jono.
        Hide
        Tim Hunt added a comment -

        What state is this in? If you want peer review, please make that clear.

        Show
        Tim Hunt added a comment - What state is this in? If you want peer review, please make that clear.
        Hide
        Jean-Michel Vedrine added a comment -

        Starting peer review

        Show
        Jean-Michel Vedrine added a comment - Starting peer review
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Jonathon,
        This is a nice addition and as you say it will be usefull to teachers when importing question pools from publishers.
        The code is working. Codechecker reports no problem and phpunit tests are OK too.
        I think the code to create the fake question in process_category could be improved.
        We currently have 2 questions import formats able to import categories: GIFT and Moodle XML.

        • GIFT is using something equivalent to:
                      $question = $this->defaultquestion();
          
                      // build fake question to contain category
                      $question->qtype = 'category';
                      $question->category = $newcategory;
          
        • Moodle XML is using:
                  $qo = new stdClass();
                  $qo->qtype = 'category';
                  $qo->category = $this->import_text($question['#']['category'][0]['#']['text']);
          

          If we look at the code in question/format.php lines 364-372 it seems to me that only qtype and category are used so I think you should simplify your code to mimic what Moodle XML do and only set qtype and category using some code like

                  $dummyquestion = new stdClass();
                  $dummyquestion->qtype = 'category';
                  $dummyquestion->category = $title;
          
                  $questions[] = $dummyquestion;
          

          and not try to set the other fields.

        You should use cleaninput($title) to ensure $title don't contain any entity (I don't know for authentic blackboard files but some ExamView and TestGen generated files contains a lot of unwanted entities)
        Maybe it would also be useful not to create a category with an empty name (I have not tested this case so I don't really know what happen ?)

        Show
        Jean-Michel Vedrine added a comment - Hello Jonathon, This is a nice addition and as you say it will be usefull to teachers when importing question pools from publishers. The code is working. Codechecker reports no problem and phpunit tests are OK too. I think the code to create the fake question in process_category could be improved. We currently have 2 questions import formats able to import categories: GIFT and Moodle XML. GIFT is using something equivalent to: $question = $ this ->defaultquestion(); // build fake question to contain category $question->qtype = 'category'; $question->category = $newcategory; Moodle XML is using: $qo = new stdClass(); $qo->qtype = 'category'; $qo->category = $ this ->import_text($question['#']['category'][0]['#']['text']); If we look at the code in question/format.php lines 364-372 it seems to me that only qtype and category are used so I think you should simplify your code to mimic what Moodle XML do and only set qtype and category using some code like $dummyquestion = new stdClass(); $dummyquestion->qtype = 'category'; $dummyquestion->category = $title; $questions[] = $dummyquestion; and not try to set the other fields. You should use cleaninput($title) to ensure $title don't contain any entity (I don't know for authentic blackboard files but some ExamView and TestGen generated files contains a lot of unwanted entities) Maybe it would also be useful not to create a category with an empty name (I have not tested this case so I don't really know what happen ?)
        Hide
        Jean-Michel Vedrine added a comment -

        Finishing peer review.

        Show
        Jean-Michel Vedrine added a comment - Finishing peer review.
        Hide
        Jonathon Fowler added a comment -

        Thanks. I'll make those improvements.

        Show
        Jonathon Fowler added a comment - Thanks. I'll make those improvements.
        Hide
        Jonathon Fowler added a comment -

        Maybe it would also be useful not to create a category with an empty name

        How do we feel about this: https://github.com/jonof/moodle/compare/MDL-38388...MDL-38390#L0R119

        If a pool has no title then no category entry gets generated, but there's a catch: if the untitled pool comes after a named pool, the questions of the untitled pool get added into the category of the previous named pool in the sequence. The options as I see them are:

        1. accept the quirky behaviour.
        2. deposit questions from pools with empty titles into the category chosen by the user on the import form. To do this we'll need to preserve the initial category whereas now it gets overwritten within qformat_default::importprocess().
        3. have the importer create its own category to deposit the questions into, eg. "Imported Blackboard questions"

        Personally I'm inclined towards the second option.

        Show
        Jonathon Fowler added a comment - Maybe it would also be useful not to create a category with an empty name How do we feel about this: https://github.com/jonof/moodle/compare/MDL-38388...MDL-38390#L0R119 If a pool has no title then no category entry gets generated, but there's a catch: if the untitled pool comes after a named pool, the questions of the untitled pool get added into the category of the previous named pool in the sequence. The options as I see them are: accept the quirky behaviour. deposit questions from pools with empty titles into the category chosen by the user on the import form. To do this we'll need to preserve the initial category whereas now it gets overwritten within qformat_default::importprocess(). have the importer create its own category to deposit the questions into, eg. "Imported Blackboard questions" Personally I'm inclined towards the second option.
        Hide
        Jean-Michel Vedrine added a comment -

        hello Jonathon,
        You are right, I think that option 2. is definitely what most users would expect. Unfortunately this need modifications to question/format.php, so I think we need Tim's approval to go this way. Tim, do you think this is OK ? If Tim don't agree this change, maybe we can choose option 3. ?
        Another though (just a detail) : maybe it would be better to use this->cleaninput($this->clean_question_name($title)) to match what the question category form does and to be sure the string is valid and not too long ?

        Show
        Jean-Michel Vedrine added a comment - hello Jonathon, You are right, I think that option 2. is definitely what most users would expect. Unfortunately this need modifications to question/format.php, so I think we need Tim's approval to go this way. Tim, do you think this is OK ? If Tim don't agree this change, maybe we can choose option 3. ? Another though (just a detail) : maybe it would be better to use this->cleaninput($this->clean_question_name($title)) to match what the question category form does and to be sure the string is valid and not too long ?
        Hide
        Tim Hunt added a comment -

        I think 2. requires too much re-architecting for very little gain, so is not worth it.

        I think 3. is the best option. Since the filenames within the zip seem to be meaningless (res0001, ...) I think you should generate names like "Imported category 1", "Imported category 2", ... (This will need a new lang string.)

        Show
        Tim Hunt added a comment - I think 2. requires too much re-architecting for very little gain, so is not worth it. I think 3. is the best option. Since the filenames within the zip seem to be meaningless (res0001, ...) I think you should generate names like "Imported category 1", "Imported category 2", ... (This will need a new lang string.)
        Hide
        Tim Hunt added a comment -

        Can I add:

        1. Please remember to verify import into lesson module is not broken.

        2. Please check that when you import one of the old example files, without multiple categories, we have not changed the behaviour in a bad way.

        Show
        Tim Hunt added a comment - Can I add: 1. Please remember to verify import into lesson module is not broken. 2. Please check that when you import one of the old example files, without multiple categories, we have not changed the behaviour in a bad way.
        Hide
        Jean-Michel Vedrine added a comment -

        Jonathon,
        If you want I can take care of verifying that lesson import is not broken if you are unfamiliar with the awful way lesson import format is stealing using question import formats (I know it: I have already broken it )

        Show
        Jean-Michel Vedrine added a comment - Jonathon, If you want I can take care of verifying that lesson import is not broken if you are unfamiliar with the awful way lesson import format is stealing using question import formats (I know it: I have already broken it )
        Hide
        Jonathon Fowler added a comment -

        I've implemented name generation for categories with empty titles. I chose to do it as a final pass over the questions array once all the pool files have been processed. It seemed the simplest approach.

        Also, I took your hint, Jean-Michel, and improved the cleaning of the category name.

        I did a quick check of importing questions into a Lesson using a couple of different Blackboard files I have at my disposal. One had multiple pools in the one ZIP, and another had only one pool. Both imported without error, though I did notice that question pictures in the lesson pages are showing as broken images where the same pictures show fine if imported into the question bank. I'll look into that issue next week when I'm back in the office. Hopefully it's not something I broke in MDL-38388.

        Show
        Jonathon Fowler added a comment - I've implemented name generation for categories with empty titles. I chose to do it as a final pass over the questions array once all the pool files have been processed. It seemed the simplest approach. Also, I took your hint, Jean-Michel, and improved the cleaning of the category name. I did a quick check of importing questions into a Lesson using a couple of different Blackboard files I have at my disposal. One had multiple pools in the one ZIP, and another had only one pool. Both imported without error, though I did notice that question pictures in the lesson pages are showing as broken images where the same pictures show fine if imported into the question bank. I'll look into that issue next week when I'm back in the office. Hopefully it's not something I broke in MDL-38388 .
        Hide
        Jean-Michel Vedrine added a comment -

        Don't worry for broken images when importing questions in lessons, this is not something you have broken, it has never been implemented .
        I have proposed a patch (MDL-38505 created yesterday) to Rossiani Wijaya (lesson component lead) to make it work at least for images in question text. Hopefully it will be accepted and integrated before 2.5 code freeze.
        And I have verified that if MDL-38505 is integrated everything is fine with your work in MDL-38390 and MDL-38388.
        Unfortunately making import of images work for multichoice answers would need important changes in lesson code as currently nothing is implemented for images in that area at all

        Show
        Jean-Michel Vedrine added a comment - Don't worry for broken images when importing questions in lessons, this is not something you have broken, it has never been implemented . I have proposed a patch ( MDL-38505 created yesterday) to Rossiani Wijaya (lesson component lead) to make it work at least for images in question text. Hopefully it will be accepted and integrated before 2.5 code freeze. And I have verified that if MDL-38505 is integrated everything is fine with your work in MDL-38390 and MDL-38388 . Unfortunately making import of images work for multichoice answers would need important changes in lesson code as currently nothing is implemented for images in that area at all
        Hide
        Jean-Michel Vedrine added a comment -

        Peer reviewing latest version

        Show
        Jean-Michel Vedrine added a comment - Peer reviewing latest version
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Jonathon,
        [N] Syntax
        [-] Output
        [Y] Whitespace
        [Y] Language
        [-] Databases
        [Y] Testing
        [-] Security
        [N] Documentation
        [N] Git
        [Y] Sanity check
        I think we are nearly done

        • The comment Give any unnamed categories generated names is missing the final dot
        • IMHO the lang string should be named importedcategory without the final x
        • The commit message is too long and should be split in several lines

        And I think we should update documentation, but looking at it documentation about blackboard v6+ format is really outdated (this format is still mentioned as being broken ), so I have added the doc_required label to this issue and I will take care of reviewing all the documentation about import of Blackboard question. Don't bother to do it yourself. This is entirely my fault and I should have done that last year when I fixed blackboard_six.

        Show
        Jean-Michel Vedrine added a comment - Hello Jonathon, [N] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [N] Documentation [N] Git [Y] Sanity check I think we are nearly done The comment Give any unnamed categories generated names is missing the final dot IMHO the lang string should be named importedcategory without the final x The commit message is too long and should be split in several lines And I think we should update documentation, but looking at it documentation about blackboard v6+ format is really outdated (this format is still mentioned as being broken ), so I have added the doc_required label to this issue and I will take care of reviewing all the documentation about import of Blackboard question. Don't bother to do it yourself. This is entirely my fault and I should have done that last year when I fixed blackboard_six.
        Hide
        Jean-Michel Vedrine added a comment -

        Finishing peer review

        Show
        Jean-Michel Vedrine added a comment - Finishing peer review
        Hide
        Jonathon Fowler added a comment -

        All done. Comments now have full-stops, the language string is sans-'x', and the new commit message is 16% shorter (now 65 chars).

        Show
        Jonathon Fowler added a comment - All done. Comments now have full-stops, the language string is sans-'x', and the new commit message is 16% shorter (now 65 chars).
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Jonathon,
        Perfect .
        Hello Tim,
        I think this is ready for integration. It will need to be rebased after MDL-38388 is integrated this week.
        I will take care of updating documentation after this is integrated.

        Show
        Jean-Michel Vedrine added a comment - Hello Jonathon, Perfect . Hello Tim, I think this is ready for integration. It will need to be rebased after MDL-38388 is integrated this week. I will take care of updating documentation after this is integrated.
        Hide
        Tim Hunt added a comment -

        Great. Thanks Jonathon and Jean-Michel. I have clicked the 'Submit for integration' button now, and I trust you will rebase once the weeklies are out.

        Show
        Tim Hunt added a comment - Great. Thanks Jonathon and Jean-Michel. I have clicked the 'Submit for integration' button now, and I trust you will rebase once the weeklies are out.
        Hide
        Damyon Wiese added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        Thanks!

        Show
        Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
        Hide
        Jean-Michel Vedrine added a comment -

        Hi Jonathon,
        Congratulations, MDL-38388 is now part of Moodle. Don't forget to rebase this one. Have a nice week-end.

        Show
        Jean-Michel Vedrine added a comment - Hi Jonathon, Congratulations, MDL-38388 is now part of Moodle. Don't forget to rebase this one. Have a nice week-end.
        Hide
        Jonathon Fowler added a comment -

        Rebase done.

        Show
        Jonathon Fowler added a comment - Rebase done.
        Hide
        Jonathon Fowler added a comment -

        By the way, I've just this morning been given a Blackboard file in the QTI variant of the supported formats, and it doesn't handle categories currently. I'm going to implement support for that, so do we feel it's worth holding this back until that's complete and then integrate it all in one go?

        Show
        Jonathon Fowler added a comment - By the way, I've just this morning been given a Blackboard file in the QTI variant of the supported formats, and it doesn't handle categories currently. I'm going to implement support for that, so do we feel it's worth holding this back until that's complete and then integrate it all in one go?
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Jonathon,
        Better to wait for Tim's decision. My personal preference would be to let this issue been integrated as it is and create a new tracker issue linked to that one.

        Show
        Jean-Michel Vedrine added a comment - Hello Jonathon, Better to wait for Tim's decision. My personal preference would be to let this issue been integrated as it is and create a new tracker issue linked to that one.
        Hide
        Tim Hunt added a comment -

        I agree with Jean-Michel. It is better to move Moodle forwards one step at a time. In particular, here, where we have a reviewed patch ready for integration, let's allow that to be integrated, and work on the next step in a separate issue.

        Show
        Tim Hunt added a comment - I agree with Jean-Michel. It is better to move Moodle forwards one step at a time. In particular, here, where we have a reviewed patch ready for integration, let's allow that to be integrated, and work on the next step in a separate issue.
        Hide
        Jonathon Fowler added a comment -

        Excellent. MDL-38705 has the changes to handle QTI.

        Show
        Jonathon Fowler added a comment - Excellent. MDL-38705 has the changes to handle QTI.
        Hide
        Damyon Wiese added a comment -

        Thanks Jonathon, Jean-Michel, and Tim for working on/reviewing this issue.

        The code looks nice and the extra unit tests are good.

        I've integrated to master now.

        Show
        Damyon Wiese added a comment - Thanks Jonathon, Jean-Michel, and Tim for working on/reviewing this issue. The code looks nice and the extra unit tests are good. I've integrated to master now.
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        In master the new behaviour is shown as expected. In earlier versions the behaviour remains as described.

        Thanks for sharing this improvement, Jono.

        Show
        Michael de Raadt added a comment - Test result: Success! In master the new behaviour is shown as expected. In earlier versions the behaviour remains as described. Thanks for sharing this improvement, Jono.
        Hide
        Damyon Wiese added a comment -

        Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

        Show
        Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
        Hide
        Mary Cooch added a comment -

        (Housekeeping)Just asking re the docs_required label - does this need documenting or is it covered in the updated documentation on Blackboard questions? I don't know anything about Blackboard questions to do this, sorry.

        Show
        Mary Cooch added a comment - (Housekeeping)Just asking re the docs_required label - does this need documenting or is it covered in the updated documentation on Blackboard questions? I don't know anything about Blackboard questions to do this, sorry.
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Mary,
        Thanks a lot for reminding me of this.
        Not this is not currently documented. I will do this on the page http://docs.moodle.org/25/en/Import_questions and post here when this is done.

        Show
        Jean-Michel Vedrine added a comment - Hello Mary, Thanks a lot for reminding me of this. Not this is not currently documented. I will do this on the page http://docs.moodle.org/25/en/Import_questions and post here when this is done.
        Hide
        Mary Cooch added a comment -

        Excellent thankyou

        Show
        Mary Cooch added a comment - Excellent thankyou
        Hide
        Jean-Michel Vedrine added a comment -

        I have edited the doc page (and somewhat refactored it for clarity) and the new feature is tagged with the "New feature in Moodle 2.5" label.
        But even if you know nothing about Blackboard questions, maybe it would be best if you review my changes to ensure my English is correct before removing the docs_required label.
        Thanks a lot for all your work.

        Show
        Jean-Michel Vedrine added a comment - I have edited the doc page (and somewhat refactored it for clarity) and the new feature is tagged with the "New feature in Moodle 2.5" label. But even if you know nothing about Blackboard questions, maybe it would be best if you review my changes to ensure my English is correct before removing the docs_required label. Thanks a lot for all your work.
        Hide
        Mary Cooch added a comment -

        Removing docs_required label - merci Jean-Michel - j'ai vérifié - c'est parfait

        Show
        Mary Cooch added a comment - Removing docs_required label - merci Jean-Michel - j'ai vérifié - c'est parfait
        Hide
        Jean-Michel Vedrine added a comment -

        Thanks Mary

        Show
        Jean-Michel Vedrine added a comment - Thanks Mary

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: