Moodle
  1. Moodle
  2. MDL-38388

Blackboard ZIP import with multiple pool files incorrectly associates embedded images

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Current (broken) behaviour:

      1. Import the bb-test.zip sample as Blackboard format into a course.
      2. Go to Course administration > Question bank > Questions.
      3. Preview the "res00000 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00000 q1".
      4. Preview the "res00001 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00001 q1".

      New (fixed) behaviour:

      1. Import the bb-test.zip sample as Blackboard format into a course.
      2. Go to Course administration > Question bank > Questions.
      3. Preview the "res00000 q1" question and observe that the picture reads "res00000/subdir/test1.jpg" and the text below it "res00000 q1".
      4. Preview the "res00001 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00001 q1".
      Show
      Current (broken) behaviour: Import the bb-test.zip sample as Blackboard format into a course. Go to Course administration > Question bank > Questions. Preview the "res00000 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00000 q1". Preview the "res00001 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00001 q1". New (fixed) behaviour: Import the bb-test.zip sample as Blackboard format into a course. Go to Course administration > Question bank > Questions. Preview the "res00000 q1" question and observe that the picture reads "res00000/subdir/test1.jpg" and the text below it "res00000 q1". Preview the "res00001 q1" question and observe that the picture reads "res00001/subdir/test1.jpg" and the text below it "res00001 q1".
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      48306

      Description

      When importing zipped Blackboard 6+ format questions, if there are multiple res000xx.dat pool files within, and they each have image resources associated with them, only the images of the last pool will be used when importing the previous ones. This will mean either broken images for questions from the earlier pool files, or the wrong instance of an image being used if filenames happen to match.

      My patch implements a mechanism for using the correct state for the relevant pool files so that the right images get imported. I've also attached a simple demonstration ZIP to illustrate the problem and the solution.

        Issue Links

          Activity

          Hide
          Jonathon Fowler added a comment -

          I can prepare branches for the other versions if my solution is acceptable.

          Show
          Jonathon Fowler added a comment - I can prepare branches for the other versions if my solution is acceptable.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, this is your area of code. Would you be able to peer-review Jonathon's code?

          Jonathon, are you able to write some testing instructions for this? (If you are not sure of the required style, have a look at some other recently fixed issues: https://tracker.moodle.org/issues/?jql=project%20%3D%20MDL%20AND%20resolution%20%3D%20Fixed%20AND%20component%20%3D%20Questions%20AND%20text%20~%20%22%2Bblackboard%20%2Bimport%22%20ORDER%20BY%20key%20DESC)

          Show
          Tim Hunt added a comment - Jean-Michel, this is your area of code. Would you be able to peer-review Jonathon's code? Jonathon, are you able to write some testing instructions for this? (If you are not sure of the required style, have a look at some other recently fixed issues: https://tracker.moodle.org/issues/?jql=project%20%3D%20MDL%20AND%20resolution%20%3D%20Fixed%20AND%20component%20%3D%20Questions%20AND%20text%20~%20%22%2Bblackboard%20%2Bimport%22%20ORDER%20BY%20key%20DESC )
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Tim, hello Jonathon,
          Thanks Tim for pointing me to this issue, I had missed it.
          I will peer review Jonathon's code.
          Thanks Jonathon for working on this.
          When I worked on the code to permit to import zip files with multiples .dat files my only samples files had no images, so I wrongly assumed the path would be the same for all files. Obviously this is not the case !
          I just saw you opened another issue (MDL-38390), if Tim agree I can peer review it too.
          It's very good we have somebody able to provide us with other blackboard files sample because most of the files submitted by users are in fact files produced with TestGen or Examview and I don't think these software use all blackboard format features. Feel free to open other issues if you find other files that fail to import correctly.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Tim, hello Jonathon, Thanks Tim for pointing me to this issue, I had missed it. I will peer review Jonathon's code. Thanks Jonathon for working on this. When I worked on the code to permit to import zip files with multiples .dat files my only samples files had no images, so I wrongly assumed the path would be the same for all files. Obviously this is not the case ! I just saw you opened another issue ( MDL-38390 ), if Tim agree I can peer review it too. It's very good we have somebody able to provide us with other blackboard files sample because most of the files submitted by users are in fact files produced with TestGen or Examview and I don't think these software use all blackboard format features. Feel free to open other issues if you find other files that fail to import correctly.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, please grab any issues like this that you can peer-review. There are some guidelines on the wiki about what is expected in a peer review: http://docs.moodle.org/dev/Peer_reviewing_checklist

          Show
          Tim Hunt added a comment - Jean-Michel, please grab any issues like this that you can peer-review. There are some guidelines on the wiki about what is expected in a peer review: http://docs.moodle.org/dev/Peer_reviewing_checklist
          Hide
          Jonathon Fowler added a comment -

          > Jonathon, are you able to write some testing instructions for this?

          Certainly. I will update the issues first thing next week.

          > It's very good we have somebody able to provide us with other blackboard files ...

          This test sample is based on the structure of a copyrighted question bank an academic was attempting to import. There's also a patch I'll be submitting next week which deals with non-UTF8-encoded Aiken format files, but it needs a little more polish.

          Show
          Jonathon Fowler added a comment - > Jonathon, are you able to write some testing instructions for this? Certainly. I will update the issues first thing next week. > It's very good we have somebody able to provide us with other blackboard files ... This test sample is based on the structure of a copyrighted question bank an academic was attempting to import. There's also a patch I'll be submitting next week which deals with non-UTF8-encoded Aiken format files, but it needs a little more polish.
          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,
          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check
          Code is good and working as expected.
          But as you have changed readquestions parameter (was an array of strings and is now an array of objects) and suppressed the set_filebase method, you need to also modify the phpunit tests in question/format/blackboard_six/tests because they are currently broken.

          Show
          Jean-Michel Vedrine added a comment - Hello Jonathon, [Y] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Code is good and working as expected. But as you have changed readquestions parameter (was an array of strings and is now an array of objects) and suppressed the set_filebase method, you need to also modify the phpunit tests in question/format/blackboard_six/tests because they are currently broken.
          Hide
          Jonathon Fowler added a comment -

          The unit tests are now fixed. If this change gets integrated, because I removed qformat_blackboard_six::set_filetype() in this patch and moved the property to the file object, there will need to be a further edit to MDL-38390 to remove a call to set_filetype() in the new unit test I added there. I can rebase MDL-38390 off this issue to avoid that if we ensure the other one is integrated first.

          Show
          Jonathon Fowler added a comment - The unit tests are now fixed. If this change gets integrated, because I removed qformat_blackboard_six::set_filetype() in this patch and moved the property to the file object, there will need to be a further edit to MDL-38390 to remove a call to set_filetype() in the new unit test I added there. I can rebase MDL-38390 off this issue to avoid that if we ensure the other one is integrated first.
          Hide
          Tim Hunt added a comment -

          I think you should rebase MDL-38390 off this, then we can submit them both for integration next week.

          Jean-Michel, can you review this and MDL-38390?

          Show
          Tim Hunt added a comment - I think you should rebase MDL-38390 off this, then we can submit them both for integration next week. Jean-Michel, can you review this and MDL-38390 ?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Jonathon and Tim,
          Sorry it took me several days to review this. The tests are now OK and I think it is ready for integration. So +1 for me, Tim you can pres the button if you are OK too. I will now look at MDL-38390.

          Show
          Jean-Michel Vedrine added a comment - Hello Jonathon and Tim, Sorry it took me several days to review this. The tests are now OK and I think it is ready for integration. So +1 for me, Tim you can pres the button if you are OK too. I will now look at MDL-38390 .
          Hide
          Jean-Michel Vedrine added a comment -

          OOps I forgot to say you should rebase master branch on latest weekly release and also create MOODLE_24_STABLE and MOODLE_23_STABLE branchs because as this is a bug fix I think it should be backported.

          Show
          Jean-Michel Vedrine added a comment - OOps I forgot to say you should rebase master branch on latest weekly release and also create MOODLE_24_STABLE and MOODLE_23_STABLE branchs because as this is a bug fix I think it should be backported.
          Hide
          Jean-Michel Vedrine added a comment -

          sorry I also forgot to start peer review !!

          Show
          Jean-Michel Vedrine added a comment - sorry I also forgot to start peer review !!
          Hide
          Jean-Michel Vedrine added a comment -

          Finishing peer review

          Show
          Jean-Michel Vedrine added a comment - Finishing peer review
          Hide
          Tim Hunt added a comment -

          Right, I will want for this and MDL-38390 to be rebase and cherry-picked to stable branches, and then click the button.

          Show
          Tim Hunt added a comment - Right, I will want for this and MDL-38390 to be rebase and cherry-picked to stable branches, and then click the button.
          Hide
          Jonathon Fowler added a comment -

          Rebased master version off current weekly release, created 23_STABLE and 24_STABLE versions too.

          Show
          Jonathon Fowler added a comment - Rebased master version off current weekly release, created 23_STABLE and 24_STABLE versions too.
          Hide
          Tim Hunt added a comment -

          Thanks. Submitting for integration.

          Show
          Tim Hunt added a comment - Thanks. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3, 2.4 and master integration branches.
          The images seem to be associated to the correct questions.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. The images seem to be associated to the correct questions. Test passed.
          Hide
          Damyon Wiese added a comment -

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

          Thanks for your contributions!

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: