Moodle
  1. Moodle
  2. MDL-32002

No 'manage plagiarism plugins'. It appears to be impossible to un-install a plagiarism plugin

    Details

    • Testing Instructions:
      Hide

      install a couple of plagiarism plugins - Urkund and turnitin are good examples. - you don't need to configure these for testing.
      https://github.com/danmarsden/moodle-plagiarism_turnitin
      https://github.com/danmarsden/moodle-plagiarism_urkund

      Make sure the advanced feature to enable plagiarism plugins is enabled.

      after install - try using the new delete option in admin > plugins > plagiarism > manage plagiarism plugins

      • confirm that the custom plagiarism_plugin tables have been removed.

      important to note that this will only delete the tables created by the plugins - the plugins both use the config_plugins table to store settings information and this information will currently stay in the db - we need to add uninstall.php to the plugins to remove this data as well (will do this at some point.)

      Show
      install a couple of plagiarism plugins - Urkund and turnitin are good examples. - you don't need to configure these for testing. https://github.com/danmarsden/moodle-plagiarism_turnitin https://github.com/danmarsden/moodle-plagiarism_urkund Make sure the advanced feature to enable plagiarism plugins is enabled. after install - try using the new delete option in admin > plugins > plagiarism > manage plagiarism plugins confirm that the custom plagiarism_plugin tables have been removed. important to note that this will only delete the tables created by the plugins - the plugins both use the config_plugins table to store settings information and this information will currently stay in the db - we need to add uninstall.php to the plugins to remove this data as well (will do this at some point.)
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      master_MDL-32002
    • Rank:
      38673

      Description

      All (most?) other plugin types have a 'manage' screen that allows them to be deleted. Plagiarism plugins do not for some reason.

        Activity

        Hide
        Michael de Raadt added a comment -

        Good point.

        Feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

        Show
        Michael de Raadt added a comment - Good point. Feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
        Hide
        Dan Poltawski added a comment -

        Hi Dan,

        Looks good to me, +1 for integration. Two small comments:

        • I'd prefer the default of this to be false rather than an empty string:
          optional_param('confirm', '', PARAM_BOOL);
          
        • This could be a html_writer::link & moodle_url (I see its existing code):
          $settings = "<a href=\"$CFG->wwwroot/plagiarism/$plugin/settings.php\">{$txt->settings}</a>";
          
        Show
        Dan Poltawski added a comment - Hi Dan, Looks good to me, +1 for integration. Two small comments: I'd prefer the default of this to be false rather than an empty string: optional_param('confirm', '', PARAM_BOOL); This could be a html_writer::link & moodle_url (I see its existing code): $settings = "<a href=\" $CFG->wwwroot/plagiarism/$plugin/settings.php\ ">{$txt->settings}</a>" ;
        Hide
        Dan Marsden added a comment -

        thanks Dan - yeah, those were both copy/paste of code that I didn't look at too closely - tidied that up now thanks!

        Show
        Dan Marsden added a comment - thanks Dan - yeah, those were both copy/paste of code that I didn't look at too closely - tidied that up now thanks!
        Hide
        Aparup Banerjee 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.

        TIA and ciao

        Show
        Aparup Banerjee 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. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Hehe reopening this at the moment sorry, certainly the code looks 99% perfect.
        However there is a return in the page if there are no plagiarism plugins installed.
        Also just a thought but perhaps it would be worth adding a notification to that page when enableplagiarism is off notifying the user of that. Just something simple like "The plagiarism detection system is currently turned off."
        Then again perhaps it is not needed.

        Also it'd be friendlier to use $table->colclasses (mdl-left, mdl-align) rather than $table->align. Just makes things styleable.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hehe reopening this at the moment sorry, certainly the code looks 99% perfect. However there is a return in the page if there are no plagiarism plugins installed. Also just a thought but perhaps it would be worth adding a notification to that page when enableplagiarism is off notifying the user of that. Just something simple like "The plagiarism detection system is currently turned off." Then again perhaps it is not needed. Also it'd be friendlier to use $table->colclasses (mdl-left, mdl-align) rather than $table->align. Just makes things styleable. Cheers Sam
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Dan Marsden added a comment -

        hehe - whoops! - I've fixed that return and now using colclasses as suggested - thanks.

        I'm not sure if we need to add a check to see if plagiarism is enabled as this page won't appear in the nav when plagiarism is disabled - but feel free to bounce back if you think I need to add it for people that type the url in manually?

        Show
        Dan Marsden added a comment - hehe - whoops! - I've fixed that return and now using colclasses as suggested - thanks. I'm not sure if we need to add a check to see if plagiarism is enabled as this page won't appear in the nav when plagiarism is disabled - but feel free to bounce back if you think I need to add it for people that type the url in manually?
        Hide
        Dan Poltawski added a comment -

        Submitting for integration - as I think thats the right state

        Show
        Dan Poltawski added a comment - Submitting for integration - as I think thats the right state
        Hide
        Dan Marsden added a comment -

        thanks Dan!

        Show
        Dan Marsden added a comment - thanks Dan!
        Hide
        Sam Hemelryk added a comment -

        Thanks Dan, code is now 100% perfect! (and has been integrated btw)

        Show
        Sam Hemelryk added a comment - Thanks Dan, code is now 100% perfect! (and has been integrated btw)
        Hide
        Frédéric Massart added a comment -

        Test successful! \o/

        Show
        Frédéric Massart added a comment - Test successful! \o/
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Fixed STOP Closed STOP Thanks STOP

        Yay, imagination! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: