Moodle
  1. Moodle
  2. MDL-42767

Notes for deleted courses causes error

    Details

    • Testing Instructions:
      Hide

      This test requires making changes before running upgrade
      This test needs all databases for testing

      Test 1 (After upgrade)

      1. Enable user notes on your site.
      2. Browse your list of users for a user that is enrolled in a course.
      3. In the navigation menu, under users, choose Notes.
      4. Under one of the courses, choose "Add a new note".
      5. Add a note, leaving it at the course context and save.
      6. Delete the course you added the note for.
      7. Go back to the user's note page, make sure no error is presented.
      8. Make sure the note is deleted from database (table 'post').

      Test 2 (Before upgrade)

      1. Please repeat this step for all supported databases atleast in master.
      2. To test if upgrade is working properly we need to make some changes in database. Create a few course level notes (same process as in test 1) and go to database and edit the courseid field for those notes (located in table 'post') and change it to a non-existent courseid.
      3. Run the upgrade.
      4. Visit the notes page user and make sure no error is shown.
      5. Go to the database and make sure those corrupt notes are deleted.

      Test 3 (Master only)
      Run phpunit lib/tests/moodlelib_test.php and make sure it passes

      Show
      This test requires making changes before running upgrade This test needs all databases for testing Test 1 (After upgrade) Enable user notes on your site. Browse your list of users for a user that is enrolled in a course. In the navigation menu, under users, choose Notes. Under one of the courses, choose "Add a new note". Add a note, leaving it at the course context and save. Delete the course you added the note for. Go back to the user's note page, make sure no error is presented. Make sure the note is deleted from database (table 'post'). Test 2 (Before upgrade) Please repeat this step for all supported databases atleast in master. To test if upgrade is working properly we need to make some changes in database. Create a few course level notes (same process as in test 1) and go to database and edit the courseid field for those notes (located in table 'post') and change it to a non-existent courseid. Run the upgrade. Visit the notes page user and make sure no error is shown. Go to the database and make sure those corrupt notes are deleted. Test 3 (Master only) Run phpunit lib/tests/moodlelib_test.php and make sure it passes
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-42767-master
    • Story Points (Obsolete):
      13
    • Sprint:
      BACKEND Sprint 7

      Description

      When a note is added for a user at the course level on the user's notes page and then that course is deleted, an error results on the note's page that the deleted course can not be found.

      Steps to replicate ->

      Enable user notes on your site.
      Browse your list of users for a user that is enrolled in a class.
      In the navigation menu, under users, choose Notes.
      Under one of the courses, choose "Add a new note".
      Add a note, leaving it at the course context and save.
      Delete the course you added the note for.
      Go back to the user's note page:

      Can not find data record in database table course.

      More information about this error
      Debug info: SELECT * FROM

      {course}

      WHERE id = ?
      [array (
      0 => 42,
      )]
      Error code: invalidrecord
      Stack trace:

      line 1372 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
      line 1348 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
      line 42 of /notes/index.php: call to moodle_database->get_record()

        Gliffy Diagrams

          Activity

          Hide
          Heather Williams added a comment -

          Here is another debugging error I am seeing:

          Can not find data record in database table course.

          More information about this error
          Debug info: SELECT id,category FROM

          {course}

          WHERE id = ?
          [array (
          0 => '447',
          )]
          Error code: invalidrecord
          Stack trace:

          line 1357 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
          line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 6599 of /lib/accesslib.php: call to moodle_database->get_record()
          line 174 of /notes/lib.php: call to context_course::instance()
          line 228 of /notes/lib.php: call to note_print()
          line 261 of /notes/lib.php: call to note_print_list()
          line 120 of /notes/index.php: call to note_print_notes()

          Show
          Heather Williams added a comment - Here is another debugging error I am seeing: Can not find data record in database table course. More information about this error Debug info: SELECT id,category FROM {course} WHERE id = ? [array ( 0 => '447', )] Error code: invalidrecord Stack trace: line 1357 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 6599 of /lib/accesslib.php: call to moodle_database->get_record() line 174 of /notes/lib.php: call to context_course::instance() line 228 of /notes/lib.php: call to note_print() line 261 of /notes/lib.php: call to note_print_list() line 120 of /notes/index.php: call to note_print_notes()
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks for the reporting this issue.

          I was able to replicate it.
          Edit:- The error is shown only when you visit the course note page. i.e with params courseid and userid(index.php?course=5&user=59). Which can not be considered as a bug since the course doesn't exist any more. However the underlying issue is present anyway since the note should be deleted and is not deleted.

          The notes should be deleted when the course is deleted.

          Show
          Ankit Agarwal added a comment - - edited Thanks for the reporting this issue. I was able to replicate it. Edit:- The error is shown only when you visit the course note page. i.e with params courseid and userid(index.php?course=5&user=59). Which can not be considered as a bug since the course doesn't exist any more. However the underlying issue is present anyway since the note should be deleted and is not deleted. The notes should be deleted when the course is deleted.
          Hide
          Ankit Agarwal added a comment -

          Requesting a review, will backport after review.

          Show
          Ankit Agarwal added a comment - Requesting a review, will backport after review.
          Hide
          Petr Skoda added a comment -

          Hello, this does not seem ok, the problem is that there might be thousands of courses and SQL is not able to handle this high number of parameters. I suppose you should either do the delete in SQL only (with subselect) or use recordset to look deleted course id that still contain notes (left join with distinct course id).

          Show
          Petr Skoda added a comment - Hello, this does not seem ok, the problem is that there might be thousands of courses and SQL is not able to handle this high number of parameters. I suppose you should either do the delete in SQL only (with subselect) or use recordset to look deleted course id that still contain notes (left join with distinct course id).
          Hide
          Ankit Agarwal added a comment -

          Thanks Petr,
          I have updated the code and added a direct SQL delete. Also back-ported the patch (Unit tests couldn't be back-ported since there are no generators for notes in stables)

          Pushing back for review.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Petr, I have updated the code and added a direct SQL delete. Also back-ported the patch (Unit tests couldn't be back-ported since there are no generators for notes in stables) Pushing back for review. Thanks
          Hide
          Petr Skoda added a comment -

          looks ok now, but please remove the extra global $DB and submit for integration

          Show
          Petr Skoda added a comment - looks ok now, but please remove the extra global $DB and submit for integration
          Hide
          Ankit Agarwal added a comment -

          Thanks Petr,

          Patch updated and rebased. Pushing for integration.

          Show
          Ankit Agarwal added a comment - Thanks Petr, Patch updated and rebased. Pushing for integration.
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          1. This is producing output in the middle of the test execution, please can you prevent that? (if there is not a way to slence it, assert there is some output)
          2. Please assert that the note exists before you delete it, else you could just be asserting nothing
          3. Do you need to include notes/lib.php before using note_load?

          Show
          Dan Poltawski added a comment - Hi Ankit, 1. This is producing output in the middle of the test execution, please can you prevent that? (if there is not a way to slence it, assert there is some output) 2. Please assert that the note exists before you delete it, else you could just be asserting nothing 3. Do you need to include notes/lib.php before using note_load?
          Hide
          Ankit Agarwal added a comment -

          Thanks Dan,

          1. suppressed.
          2. I have added a assert, not sure if it is really needed as we are just asserting here that the generator is working.
          3. It is not required as remove_course_contents() includes notes/lib.php.

          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Dan, suppressed. I have added a assert, not sure if it is really needed as we are just asserting here that the generator is working. It is not required as remove_course_contents() includes notes/lib.php. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          1. I think its much preferable to use something like: $this->expectOutputRegex('/Cleaning up/');
          2. Well, I guess I would suggest a simpler approach than you have gone for, one that verifies that the verification you are doing works.. i.e. don't queyr the db tables, but instead just test note_load() actually returns something before you start. That way you know you are verifying the state before and after correctly.

          Show
          Dan Poltawski added a comment - Hi Ankit, 1. I think its much preferable to use something like: $this->expectOutputRegex('/Cleaning up/'); 2. Well, I guess I would suggest a simpler approach than you have gone for, one that verifies that the verification you are doing works.. i.e. don't queyr the db tables, but instead just test note_load() actually returns something before you start. That way you know you are verifying the state before and after correctly.
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,

          I noticed that remove_course_content() had a flag to supress the output, sorry didn't see it before. Anyway patch updated. I also added one more commit to correct the incorrect usage of remove_course_content with ob_clean in events test. If you prefer this to be separate issue instead, please feel free to ignore the last commit.

          cheers

          Show
          Ankit Agarwal added a comment - Hi Dan, I noticed that remove_course_content() had a flag to supress the output, sorry didn't see it before. Anyway patch updated. I also added one more commit to correct the incorrect usage of remove_course_content with ob_clean in events test. If you prefer this to be separate issue instead, please feel free to ignore the last commit. cheers
          Hide
          Marina Glancy added a comment -

          Dan is away so I will be integrating this. Just running phpunit tests and oracle test first

          Show
          Marina Glancy added a comment - Dan is away so I will be integrating this. Just running phpunit tests and oracle test first
          Hide
          Marina Glancy added a comment -

          Thanks Ankit, integrated in 2.4, 2.5, 2.6 and master

          I followed testing instructions on oracle on master, everything is fine.

          Show
          Marina Glancy added a comment - Thanks Ankit, integrated in 2.4, 2.5, 2.6 and master I followed testing instructions on oracle on master, everything is fine.
          Hide
          Marina Glancy added a comment -

          Ankit, the unittest is also integrated in 2.6 since atm 2.6 must be the same as master.

          Show
          Marina Glancy added a comment - Ankit, the unittest is also integrated in 2.6 since atm 2.6 must be the same as master.
          Hide
          Adrian Greeve added a comment -

          I just checked this on MSSQL and it passed the tests.

          Show
          Adrian Greeve added a comment - I just checked this on MSSQL and it passed the tests.
          Hide
          Marina Glancy added a comment -

          test successfull on all branches. I tested postgres and mysql. Waiting for Adrian to report on mssql.

          To be honest I have no idea why were we fixing this.... The bug as it was reported should have been closed as won't fix. The bug that Ankit found meanwhile about not cleaning up notes it's a different story and to be honest it's much less important than plenty of other issues in the backlog.

          Show
          Marina Glancy added a comment - test successfull on all branches. I tested postgres and mysql. Waiting for Adrian to report on mssql. To be honest I have no idea why were we fixing this.... The bug as it was reported should have been closed as won't fix. The bug that Ankit found meanwhile about not cleaning up notes it's a different story and to be honest it's much less important than plenty of other issues in the backlog.
          Hide
          Marina Glancy added a comment -

          Test passed

          Show
          Marina Glancy added a comment - Test passed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ...
          But still, I thank you, for you made me stronger…

          Stronger as the beast that roar in the wild
          Stronger as the storm across the ocean
          Stronger as the diamond that won’t break
          Stronger enough to take all heart aches….

          I thank you my friend, for you made me stronger…

          ---- Juneah Landicho

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile