Moodle
  1. Moodle
  2. MDL-37897

Remove useless Blackboard question import format

    Details

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

      1. Test the upgrade, ensure there are no errors.
      2. After the upgrade, make sure there is not trace of format_blackboard in config_plugins. (There will be information about format_blackboardsix.)
      3. Check that nothing is broken if you export some questoins from the question bank as Moodle XML, and then re-import them into another course.
      4. Check that you can import the example files in question/format/blackboard_six/tests/fixtures

      Show
      1. Test the upgrade, ensure there are no errors. 2. After the upgrade, make sure there is not trace of format_blackboard in config_plugins. (There will be information about format_blackboardsix.) 3. Check that nothing is broken if you export some questoins from the question bank as Moodle XML, and then re-import them into another course. 4. Check that you can import the example files in question/format/blackboard_six/tests/fixtures
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      47651

      Description

      As the Blackboard V6+ (question/format/blackboard_six) is now able to handle all Blackboard files, we should certainly remove the Blackboard format (question/format/blackboard) because its presence just confuse users. Of course this is only doable for Moodle 2.5.

        Activity

        Hide
        Tim Hunt added a comment -

        Will you do a patch for this Jean-Michel?

        Also, while we are at it, shall we rename the remaining plugin to just 'Blackboard format'?

        Show
        Tim Hunt added a comment - Will you do a patch for this Jean-Michel? Also, while we are at it, shall we rename the remaining plugin to just 'Blackboard format'?
        Hide
        Jean-Michel Vedrine added a comment - - edited

        Hello Tim,
        Yes I think I just need to suppress the whole content of the subfolder.
        Calling the remaining format just blackboard makes sense, so after suppressing all files in the blackboard subfolder I should move all the files in blackboard_six subfolder to blackboard and edit all files accordingly.
        But I don't know how to be nice for translators ? Makee an AMOS script to move all strings ? maybe as there are only 9 strings for the current blackboard_six component this would be the best solution and the new strings would be automatically translated. Do we need to ask David about this ?
        Edit OOps reading again your comment maybe you just wanted to let the blackboard_six plugin as it is and just change the string appearing to user when they chhose the format to "Blackboard" ?

        Show
        Jean-Michel Vedrine added a comment - - edited Hello Tim, Yes I think I just need to suppress the whole content of the subfolder. Calling the remaining format just blackboard makes sense, so after suppressing all files in the blackboard subfolder I should move all the files in blackboard_six subfolder to blackboard and edit all files accordingly. But I don't know how to be nice for translators ? Makee an AMOS script to move all strings ? maybe as there are only 9 strings for the current blackboard_six component this would be the best solution and the new strings would be automatically translated. Do we need to ask David about this ? Edit OOps reading again your comment maybe you just wanted to let the blackboard_six plugin as it is and just change the string appearing to user when they chhose the format to "Blackboard" ?
        Hide
        Tim Hunt added a comment -

        Yes. I meant leave the folder called blackboard_six! Just change the name as it appears in the UI.

        Show
        Tim Hunt added a comment - Yes. I meant leave the folder called blackboard_six! Just change the name as it appears in the UI.
        Hide
        Jean-Michel Vedrine added a comment -

        Cool a lot less work

        Show
        Jean-Michel Vedrine added a comment - Cool a lot less work
        Hide
        Tim Hunt added a comment -

        That's the kind of patch I like to see: "Showing 6 changed files with 2 additions and 1,027 deletions."

        Do we need anything else other than testing instructions before this is submitted for integration.

        Show
        Tim Hunt added a comment - That's the kind of patch I like to see: "Showing 6 changed files with 2 additions and 1,027 deletions." Do we need anything else other than testing instructions before this is submitted for integration.
        Hide
        Jean-Michel Vedrine added a comment -

        At the same time all the code I am now removing has been added to blackboard_six a few months ago. But I understand you, this one is a lot easier to peer review .
        I have tried very hard to find sensible testing instructions but don't have very good ideas.
        Surely we need to ask to test import the examples files in question/format/blackboard_six/tests/fixtures (I don't think we need to test that the removed file question/format/blackboard/tests/fixtures/sample_blackboard.dat is still imported because it is identical to question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat). But what else ?
        Searching for "blackboard" everywhere in Moodle's code has revealed a place I must change too I think in lib/pluginlib.php line 511.
        Another though: maybe i should increase version number as I am changing some strings ?

        Show
        Jean-Michel Vedrine added a comment - At the same time all the code I am now removing has been added to blackboard_six a few months ago. But I understand you, this one is a lot easier to peer review . I have tried very hard to find sensible testing instructions but don't have very good ideas. Surely we need to ask to test import the examples files in question/format/blackboard_six/tests/fixtures (I don't think we need to test that the removed file question/format/blackboard/tests/fixtures/sample_blackboard.dat is still imported because it is identical to question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat). But what else ? Searching for "blackboard" everywhere in Moodle's code has revealed a place I must change too I think in lib/pluginlib.php line 511. Another though: maybe i should increase version number as I am changing some strings ?
        Hide
        Jean-Michel Vedrine added a comment - - edited

        The more I look at the code in pluginlib the less I am sure of how to handle deletion of a standard plugin now in Moodle with all the new release notifications stuff. look at is_deleted_standard_plugin function: should I add something here ? Maybe I should ask David or somebody else ?

        Show
        Jean-Michel Vedrine added a comment - - edited The more I look at the code in pluginlib the less I am sure of how to handle deletion of a standard plugin now in Moodle with all the new release notifications stuff. look at is_deleted_standard_plugin function: should I add something here ? Maybe I should ask David or somebody else ?
        Hide
        David Mudrak added a comment -

        Yes, the plugin should be added into the static array there. During the upgrade to 2.5, there is a screen with the overview of plugins displayed before the upgrade actually starts. As qformat_blackboard is not present in the code (the code is updated to 2.5 already), it would be reported as missing from disk. By putting the plugin into this method it will be correctly reported as "to be deleted" and the upgrade code should properly uninstall it (I don't think it does at the moment, looking at the patch).

        For the reference see how is_deleted_standard_plugin() looks at MOODLE_22_STABLE:

        public static function is_deleted_standard_plugin($type, $name) {
            static $plugins = array(
                'block' => array('admin', 'admin_tree', 'loancalc', 'search'),
                'filter' => array('mod_data', 'mod_glossary'),
            );
        
            if (!isset($plugins[$type])) {
                return false;
            }
            return in_array($name, $plugins[$type]);
        }
        

        See MDL-30786 for the background of this feature.

        Please note that you should remove the 'blackboard' from standard_plugins_list() method in lib/pluginlib.php too.

        Also, can you please create a task for me saying that once this is integrated, I should drop qformat_blackboard from 2.5 branch in AMOS? AMOS handles removal of individual strings well. But in case of the whole component disappearing, AMOS does not perform any operation. It makes sense as usually the plugin is just moved to the CONTRIB area but should still be kept in AMOS. This is apparently different case and we want to get rid of this obsolete plugin. Thanks.

        Show
        David Mudrak added a comment - Yes, the plugin should be added into the static array there. During the upgrade to 2.5, there is a screen with the overview of plugins displayed before the upgrade actually starts. As qformat_blackboard is not present in the code (the code is updated to 2.5 already), it would be reported as missing from disk. By putting the plugin into this method it will be correctly reported as "to be deleted" and the upgrade code should properly uninstall it (I don't think it does at the moment, looking at the patch). For the reference see how is_deleted_standard_plugin() looks at MOODLE_22_STABLE: public static function is_deleted_standard_plugin($type, $name) { static $plugins = array( 'block' => array('admin', 'admin_tree', 'loancalc', 'search'), 'filter' => array('mod_data', 'mod_glossary'), ); if (!isset($plugins[$type])) { return false ; } return in_array($name, $plugins[$type]); } See MDL-30786 for the background of this feature. Please note that you should remove the 'blackboard' from standard_plugins_list() method in lib/pluginlib.php too. Also, can you please create a task for me saying that once this is integrated, I should drop qformat_blackboard from 2.5 branch in AMOS? AMOS handles removal of individual strings well. But in case of the whole component disappearing, AMOS does not perform any operation. It makes sense as usually the plugin is just moved to the CONTRIB area but should still be kept in AMOS. This is apparently different case and we want to get rid of this obsolete plugin. Thanks.
        Hide
        Jean-Michel Vedrine added a comment -

        Thanks David, this is exactly the information I was needing.
        You say "the upgrade code should properly uninstall it (I don't think it does at the moment)" but as this plugin has no database table what code should I put and where ?
        Again thanks a lot for your help.

        Show
        Jean-Michel Vedrine added a comment - Thanks David, this is exactly the information I was needing. You say "the upgrade code should properly uninstall it (I don't think it does at the moment)" but as this plugin has no database table what code should I put and where ? Again thanks a lot for your help.
        Hide
        Tim Hunt added a comment -

        I think the least bad option is to create blackboard_six/db/upgrde.php and put the code there.

        Show
        Tim Hunt added a comment - I think the least bad option is to create blackboard_six/db/upgrde.php and put the code there.
        Hide
        Jean-Michel Vedrine added a comment -

        But I don't understand what code I should put in upgrade.php ?
        I have pushed the commit to change pluginlib.php. David if you can have a look at it and tell me if it is Ok, I would be more confident. Thanks.

        Show
        Jean-Michel Vedrine added a comment - But I don't understand what code I should put in upgrade.php ? I have pushed the commit to change pluginlib.php. David if you can have a look at it and tell me if it is Ok, I would be more confident. Thanks.
        Hide
        David Mudrak added a comment -

        OK, there is not much to do then. You should just remove all the plugin data from mdl_config_plugins (which is, as far as I can see, just the information about the version installed). As there does not seem to be any administration/management API for question formats, you will probably want to use unset_all_config_for_plugin() to do the job for you.

        On contrary to what Tim suggests, I think this upgrade step (that removes qformat_blackboard's config data) should be put into the main lib/db/upgrade.php and not in qformat/blackboard_six/db/upgrade.php. It's effectively the same. But conceptually it is the core that should be given the privilege to kill it's plugins. In other words, one plugin should not touch other plugin's data or even uninstall it.

        Show
        David Mudrak added a comment - OK, there is not much to do then. You should just remove all the plugin data from mdl_config_plugins (which is, as far as I can see, just the information about the version installed). As there does not seem to be any administration/management API for question formats, you will probably want to use unset_all_config_for_plugin() to do the job for you. On contrary to what Tim suggests, I think this upgrade step (that removes qformat_blackboard's config data) should be put into the main lib/db/upgrade.php and not in qformat/blackboard_six/db/upgrade.php. It's effectively the same. But conceptually it is the core that should be given the privilege to kill it's plugins. In other words, one plugin should not touch other plugin's data or even uninstall it.
        Hide
        Jean-Michel Vedrine added a comment - - edited

        So I think I must add something like

            if ($oldversion < xxx) {
                // Cleanup qformat blackboard settings.
                unset_all_config_for_plugin('qformat_blackboard');
        
                upgrade_main_savepoint(true,xxx);
            }
        

        in lib/db/upgrade.php, but I don't know what version number I should use here.

        Show
        Jean-Michel Vedrine added a comment - - edited So I think I must add something like if ($oldversion < xxx) { // Cleanup qformat blackboard settings. unset_all_config_for_plugin('qformat_blackboard'); upgrade_main_savepoint( true ,xxx); } in lib/db/upgrade.php, but I don't know what version number I should use here.
        Hide
        David Mudrak added a comment -

        Yes. You just increase the version number in your version.php and put a block like that into lib/db/upgrade.php. So, let us say this is what you have now in version.php:

        $version  = 2013021400.00;
        

        Then you want to modify version.php to:

        $version  = 2013021400.01;
        

        and use that instead of xxx in the upgrade code above. Look at the result of "git show c8b3346c version.php lib/db/upgrade.php" for another example of the similar step.

        As you can imagine, couple of developers may submit a change like that within the same integration window. It's our integration team duty to sort out conflicts then.

        Show
        David Mudrak added a comment - Yes. You just increase the version number in your version.php and put a block like that into lib/db/upgrade.php. So, let us say this is what you have now in version.php: $version = 2013021400.00; Then you want to modify version.php to: $version = 2013021400.01; and use that instead of xxx in the upgrade code above. Look at the result of "git show c8b3346c version.php lib/db/upgrade.php" for another example of the similar step. As you can imagine, couple of developers may submit a change like that within the same integration window. It's our integration team duty to sort out conflicts then.
        Hide
        Jean-Michel Vedrine added a comment -

        Thanks David,
        This is now done in my github branch.
        Please verify I didn't make any mistake because I am really frightened by this kind of change !
        Also I have a question: as this will certainly not be integrated this week, what will I need to do when rebasing after weekly release ? Should I also make a new commit to change the version number I have used and squash this commit ? Or change nothing and integrators will take care of that ?

        Show
        Jean-Michel Vedrine added a comment - Thanks David, This is now done in my github branch. Please verify I didn't make any mistake because I am really frightened by this kind of change ! Also I have a question: as this will certainly not be integrated this week, what will I need to do when rebasing after weekly release ? Should I also make a new commit to change the version number I have used and squash this commit ? Or change nothing and integrators will take care of that ?
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Tim,
        I think this is now ready. So putting in peer review. Thanks.

        Show
        Jean-Michel Vedrine added a comment - Hello Tim, I think this is now ready. So putting in peer review. Thanks.
        Hide
        Tim Hunt added a comment -

        Just noting that we don't have testing instructions yet.

        Show
        Tim Hunt added a comment - Just noting that we don't have testing instructions yet.
        Hide
        Jean-Michel Vedrine added a comment -

        Thanks Tim,
        True, but what should we test ?

        • That in Question bank -> Import the list doesn't contains the old format and the name of the Blackboard V6+ has been updated to Blackboard
        • That in plugin's list there is no leftover of the blackboard format
        • ...
        Show
        Jean-Michel Vedrine added a comment - Thanks Tim, True, but what should we test ? That in Question bank -> Import the list doesn't contains the old format and the name of the Blackboard V6+ has been updated to Blackboard That in plugin's list there is no leftover of the blackboard format ...
        Hide
        Tim Hunt added a comment -

        Probably the main thing to test is the upgrade.

        Show
        Tim Hunt added a comment - Probably the main thing to test is the upgrade.
        Hide
        Tim Hunt added a comment -

        OK, I think those will do as testing instructions. Submitting for integration now.

        Show
        Tim Hunt added a comment - OK, I think those will do as testing instructions. Submitting for integration now.
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Tim,
        Thanks a lot for the testing instructions. I am now on holidays for 1 week so I have some free time and I hope I will be able to work on some tracker issues assigned to me ;

        Show
        Jean-Michel Vedrine added a comment - Hello Tim, Thanks a lot for the testing instructions. I am now on holidays for 1 week so I have some free time and I hope I will be able to work on some tracker issues assigned to me ;
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master only), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
        Hide
        David Mudrak added a comment -

        Sorry Jean-Michel, I did not manage to look at this earlier. I was postponing paying attention to this so long that it got integrated finally! That's a malicious precedent for me to follow, for sure Thanks a lot for your work on this.

        Show
        David Mudrak added a comment - Sorry Jean-Michel, I did not manage to look at this earlier. I was postponing paying attention to this so long that it got integrated finally! That's a malicious precedent for me to follow, for sure Thanks a lot for your work on this.
        Hide
        Ankit Agarwal added a comment -

        Hi guys,
        Everything worked as described. both test files where imported successfully.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi guys, Everything worked as described. both test files where imported successfully. Thanks
        Hide
        Jean-Michel Vedrine added a comment -

        Hello David,
        No problem, you helped a lot fixing this issue and I understand it's quite difficult to follow so many tracker issues.
        When you have some time, if you can have a look at my problems in MDL-33424, it would help me a lot. Thanks.

        Hello Ankit,
        Thanks a lot. I was rather anxious as I never this kind of code before !

        Show
        Jean-Michel Vedrine added a comment - Hello David, No problem, you helped a lot fixing this issue and I understand it's quite difficult to follow so many tracker issues. When you have some time, if you can have a look at my problems in MDL-33424 , it would help me a lot. Thanks. Hello Ankit, Thanks a lot. I was rather anxious as I never this kind of code before !
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
        Hide
        Mary Cooch added a comment -

        Removing docs_required label as I have changed Blackboard v 6 to just Blackboard here http://docs.moodle.org/25/en/Import_questions However, as I have no experience of Blackboard questions at all, I'd be grateful if, when convenient, someone could check the rest of the documentation on it is ok. Thanks.

        Show
        Mary Cooch added a comment - Removing docs_required label as I have changed Blackboard v 6 to just Blackboard here http://docs.moodle.org/25/en/Import_questions However, as I have no experience of Blackboard questions at all, I'd be grateful if, when convenient, someone could check the rest of the documentation on it is ok. Thanks.
        Hide
        Jean-Michel Vedrine added a comment -

        Thanks Mary, I will review the docs and edit them when necessary.

        Show
        Jean-Michel Vedrine added a comment - Thanks Mary, I will review the docs and edit them when necessary.
        Hide
        Jean-Michel Vedrine added a comment -

        In fact I have already updated this page a few days or weeks ago (my memory is really, really bad ), but of course as I have edited the page for Moodle 2.4, I left Blackboard V6+ everywhere so it seems you have done the remaining bit for 2.5 and all is now OK !

        Show
        Jean-Michel Vedrine added a comment - In fact I have already updated this page a few days or weeks ago (my memory is really, really bad ), but of course as I have edited the page for Moodle 2.4, I left Blackboard V6+ everywhere so it seems you have done the remaining bit for 2.5 and all is now OK !
        Hide
        Mary Cooch added a comment -

        Great thanks!

        Show
        Mary Cooch added a comment - Great thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: