Moodle
  1. Moodle
  2. MDL-30635

q* plugin types should be updated to use cron_execute_plugin_type

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Questions, Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Go to the quiz statistics report, and generate some statistics. These are cached in the quiz_statistics and related tables.

      2. Wait 5+ hours (or go into the quiz_statistics table, and hack the timestamps to be a long way in the past).

      3. Run cron.

      Verify there are no errors relating to q* plugin types.

      Verify that the old cached quiz_statistics and related data has been deleted.

      This needs to be tested on 2.2 and master, since the changes are different in each place.

      Show
      1. Go to the quiz statistics report, and generate some statistics. These are cached in the quiz_statistics and related tables. 2. Wait 5+ hours (or go into the quiz_statistics table, and hack the timestamps to be a long way in the past). 3. Run cron. Verify there are no errors relating to q* plugin types. Verify that the old cached quiz_statistics and related data has been deleted. This needs to be tested on 2.2 and master, since the changes are different in each place.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33446

      Description

      Currently

      • qtypes are fine
      • quiz reports have custom cron, and need to be converted.
      • quizaccess, qformat and qbehaviour plugins cannot have cron. They should be enabled.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          2.2 branch just adds standard cron support to quiz report plugins, so we can drop non-standard support in 2.3.

          The master branch adds support for all plugin types that did not already have it, and then drop the legacy support in quiz reports.

          Show
          Tim Hunt added a comment - 2.2 branch just adds standard cron support to quiz report plugins, so we can drop non-standard support in 2.3. The master branch adds support for all plugin types that did not already have it, and then drop the legacy support in quiz reports.
          Hide
          Tim Hunt added a comment -

          Note that this cannot be submitted for integration until after MDL-30610 has been integrated, and these branches have been rebased.

          Show
          Tim Hunt added a comment - Note that this cannot be submitted for integration until after MDL-30610 has been integrated, and these branches have been rebased.
          Hide
          Petr Škoda added a comment -

          great!

          Show
          Petr Škoda added a comment - great!
          Hide
          Tim Hunt added a comment -

          Finally, this can be submitted for integration.

          I have just rebased the branches.

          Show
          Tim Hunt added a comment - Finally, this can be submitted for integration. I have just rebased the branches.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect, integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect, integrated, thanks!
          Hide
          Ankit Agarwal added a comment -

          Hi Tim,
          As discussed, the cron is not activated atm.
          Failing this for further investigation.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Tim, As discussed, the cron is not activated atm. Failing this for further investigation. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Also, apart from the need to upgrade & change quiz's cron, I've detected that the name of the cron functions in the subplugins are incorrect. For example, the statistics one should be:

          quiz_statistics_cron()

          and currently you're using:

          quiz_report_statistics_cron()

          So it's not detected properly by get_plugin_list_with_function()

          Surely the same happens with other subplugins. (seems accessrule ones have not any cron implementation, edited)

          When I reviewed this, I forgot the annoying name that quiz reports subplugin have ('quiz'). IMO we should move to 'quizreport' or similar asap.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Also, apart from the need to upgrade & change quiz's cron, I've detected that the name of the cron functions in the subplugins are incorrect. For example, the statistics one should be: quiz_statistics_cron() and currently you're using: quiz_report_statistics_cron() So it's not detected properly by get_plugin_list_with_function() Surely the same happens with other subplugins. (seems accessrule ones have not any cron implementation, edited) When I reviewed this, I forgot the annoying name that quiz reports subplugin have ('quiz'). IMO we should move to 'quizreport' or similar asap. Ciao
          Hide
          Tim Hunt added a comment -

          Please re-integrate

          Note that these branches include the fix for MDL-26469 for 2.2 and master, which are required for this to work. I hope that is not too confusing.

          Show
          Tim Hunt added a comment - Please re-integrate https://github.com/timhunt/moodle/compare/master...MDL-30635b https://github.com/timhunt/moodle/compare/MOODLE_22_STABLE...MDL-30635b_22 Note that these branches include the fix for MDL-26469 for 2.2 and master, which are required for this to work. I hope that is not too confusing.
          Hide
          Tim Hunt added a comment -

          ... but note that MDL-26469 also needs to be fixed in 2.1 and 2.0.

          Show
          Tim Hunt added a comment - ... but note that MDL-26469 also needs to be fixed in 2.1 and 2.0.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re integrated, with the 2 new commits (22 and master). Now $module->cron should be considered and upgraded ok (as MDL-26469 fixed that).

          Show
          Eloy Lafuente (stronk7) added a comment - Re integrated, with the 2 new commits (22 and master). Now $module->cron should be considered and upgraded ok (as MDL-26469 fixed that).
          Hide
          Ankit Agarwal added a comment -

          This is working great now!
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - This is working great now! Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding the docs_required to this just in case it needs documentation in upgrade files and release notes/moodle docs.

          Show
          Eloy Lafuente (stronk7) added a comment - Adding the docs_required to this just in case it needs documentation in upgrade files and release notes/moodle docs.
          Hide
          Tim Hunt added a comment -

          This patch already contained changes to upgrade.txt files to document this.

          The documentation for these plugin types already exists, and does not mention cron at all. So the fact that these plugins can now do cron in the standard way is good, and probably does not need to be documented explicitly.

          Show
          Tim Hunt added a comment - This patch already contained changes to upgrade.txt files to document this. The documentation for these plugin types already exists, and does not mention cron at all. So the fact that these plugins can now do cron in the standard way is good, and probably does not need to be documented explicitly.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (agree, thanks!)

          Show
          Eloy Lafuente (stronk7) added a comment - (agree, thanks!)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: