Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Testing Instructions:
      Hide
      1. Through your console run admin/cli/automated_backups.php
      2. Upgrade your site.
      3. Check your database to make sure the table has been deleted.
      4. Run automated_backups.php again
      5. Log in as an admin
      6. Browse to Settings > Reports > Backups
      7. Make sure you dont' get any errors etc in the report and if you have courses make sure they are listed in the report.
      Show
      Through your console run admin/cli/automated_backups.php Upgrade your site. Check your database to make sure the table has been deleted. Run automated_backups.php again Log in as an admin Browse to Settings > Reports > Backups Make sure you dont' get any errors etc in the report and if you have courses make sure they are listed in the report.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-29185-master
    • Rank:
      18733

      Description

      It should now be possible and safe to remove the backup_log table.

      Please have a read of the comments on MDL-26811 as to the reason this issue was created.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Taking ownership of this issue and bumping up the priority as this should definitely happen before 2.2

          Show
          Sam Hemelryk added a comment - Taking ownership of this issue and bumping up the priority as this should definitely happen before 2.2
          Hide
          Sam Hemelryk added a comment -

          Linking to issue about deciding what to do with this table... it's been decided now.

          Show
          Sam Hemelryk added a comment - Linking to issue about deciding what to do with this table... it's been decided now.
          Hide
          Sam Hemelryk added a comment -

          Up for peer-review.

          This patch does the following thing:

          • Drops the now unused backup_log table
          • Fixed the broken backup admin report
          • Removes the unused + broken backup/backup_scheduled.php
          • Removes unused function from backup/lib.php (which I think will be removed in another issue)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Up for peer-review. This patch does the following thing: Drops the now unused backup_log table Fixed the broken backup admin report Removes the unused + broken backup/backup_scheduled.php Removes unused function from backup/lib.php (which I think will be removed in another issue) Cheers Sam
          Hide
          Andrew Davis added a comment -

          Hi. Can you just tidy up the intending of this in admin/report/backups/index.php

          // Create the row and add it to the table
          $table->data[] = new html_table_row(array(
             format_string($backuprow->fullname, true, array('context' => get_context_instance(CONTEXT_COURSE, $backuprow->courseid))),
             userdate($backuprow->laststarttime, $strftimedatetime),
             '-',
             userdate($backuprow->lastendtime, $strftimedatetime),
             $status,
             userdate($backuprow->nextstarttime, $strftimedatetime)
          ));
          

          Something like this just to make it more obvious what is in the array Vs what is an argument to html_table_row().

          $table->data[] = new html_table_row(
             array(
                format_string(...
          

          Otherwise it looks good. Nice to see code getting removed for once

          Show
          Andrew Davis added a comment - Hi. Can you just tidy up the intending of this in admin/report/backups/index.php // Create the row and add it to the table $table->data[] = new html_table_row(array( format_string($backuprow->fullname, true , array('context' => get_context_instance(CONTEXT_COURSE, $backuprow->courseid))), userdate($backuprow->laststarttime, $strftimedatetime), '-', userdate($backuprow->lastendtime, $strftimedatetime), $status, userdate($backuprow->nextstarttime, $strftimedatetime) )); Something like this just to make it more obvious what is in the array Vs what is an argument to html_table_row(). $table->data[] = new html_table_row( array( format_string(... Otherwise it looks good. Nice to see code getting removed for once
          Hide
          Andrew Davis added a comment -

          Oh, and add testing instructions. Tester can verify that the table is removed and that the report works.

          Show
          Andrew Davis added a comment - Oh, and add testing instructions. Tester can verify that the table is removed and that the report works.
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at that Andrew, I've separated out the generation of the cells to make it more readable and have put it up for integration now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at that Andrew, I've separated out the generation of the cells to make it more readable and have put it up for integration now. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Offtopic: It would be great if you do some cleanup / update of your github repo, lol. Right now https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29185-master is returning way too much. Not a big problem but...

          So I'm looking @ https://github.com/samhemelryk/moodle/commit/89eda3eeaf2cd225345099a08729b81a3d241dbc

          for integration.

          Show
          Eloy Lafuente (stronk7) added a comment - Offtopic: It would be great if you do some cleanup / update of your github repo, lol. Right now https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29185-master is returning way too much. Not a big problem but... So I'm looking @ https://github.com/samhemelryk/moodle/commit/89eda3eeaf2cd225345099a08729b81a3d241dbc for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks. I was slightly reticent to delete the backup_scheduled script (just in case we need to look to something one it). But it's there in other branches so perfect.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks. I was slightly reticent to delete the backup_scheduled script (just in case we need to look to something one it). But it's there in other branches so perfect. Ciao
          Hide
          Aparup Banerjee added a comment -

          This tested fine. backup_log table is gone and no errors seen.
          (backup_logs table is happily around)

          Show
          Aparup Banerjee added a comment - This tested fine. backup_log table is gone and no errors seen. (backup_logs table is happily around)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: