Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-28690

Meta courses and $CFG->longtimenosee can cause repeated welcome messages

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.13
    • Fix Version/s: 1.9.14
    • Component/s: Course
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      1) Set the $CFG->longtimenosee setting (any small value)
      2) Create a meta course and at least one child course
      3) Enrol a test user onto one of the child courses
      4) Ensure meta course is updated by running cron to update the user's enrolment into the meta course

      5) Wait $CFG->longtimenosee days (or manually change the table mdl_user_lastaccess, so that the record for that user and the metacourse has a timeaccess more than $CFG->longtimenosee days ago)

      6) Test that the test user isn't unenrolled from the metacourse and only from the child course.

      Show
      1) Set the $CFG->longtimenosee setting (any small value) 2) Create a meta course and at least one child course 3) Enrol a test user onto one of the child courses 4) Ensure meta course is updated by running cron to update the user's enrolment into the meta course 5) Wait $CFG->longtimenosee days (or manually change the table mdl_user_lastaccess, so that the record for that user and the metacourse has a timeaccess more than $CFG->longtimenosee days ago) 6) Test that the test user isn't unenrolled from the metacourse and only from the child course.
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      Replication instructions:

      1. Set the $CFG->longtimenosee setting (any value)
      2. Create a meta course and at least one child course
      3. Add a 'welcome' message to the meta course
      4. Enrol a user onto one of the child courses
      5. Run cron to update the user's enrolment into the meta course
      6. Wait $CFG->longtimenosee days (or manually change the table mdl_user_lastaccess, so that the record for that user and the metacourse has a timeaccess more than $CFG->longtimenosee days ago)

      Expected result: nothing happens, as the user is still enroled in the child course, so should stay enroled in the meta course as well.

      Actual result: every time admin/cron.php runs, there is a 20% chance the user will be unenroled from the meta course (as they have not visited it recently enough), they will then immediately be enroled back into it again (as they are still enroled in the child course), at which point they will receive a welcome message. The user will keep receiving 'welcome' emails multiple times an hour until they visit the meta course and reset the 'timeaccess' value.

      Proposed solution:
      In cron.php - check to see if the course is a metacourse before unenroling after $CFG->longtimenosee (the user will already be automatially unenroled, once they have been unenroled from the child courses, so we should not have to worry that they will never leave the meta course)

      $rs = get_recordset_sql ("SELECT id, userid, courseid
                                  FROM {$CFG->prefix}user_lastaccess
                                 WHERE courseid != ".SITEID."
                                   AND timeaccess < $cuttime ");

      Should become:

      $rs = get_recordset_sql ("SELECT la.id, la.userid, la.courseid
                                  FROM {$CFG->prefix}user_lastaccess la
                                  JOIN {$CFG->prefix}course c
                                    ON c.id = la.courseid
                                 WHERE la.courseid != ".SITEID."
                                   AND la.timeaccess < $cuttime 
                                   AND c.metacourse = 0");

      An alternative solution might be to update the 'timeaccess' field in the metacourse when the child course is visited.

      I have not fully investigated, but I suspect that M2.0 may have the same problem.

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Hi, Davo. Good to hear from you.

          Thanks for reporting this and providing a solution.

          Show
          salvetore Michael de Raadt added a comment - Hi, Davo. Good to hear from you. Thanks for reporting this and providing a solution.
          Hide
          nebgor Aparup Banerjee added a comment -

          Hi Davo,
          Thanks for the report!

          The alternative might introduce a few more queries for a statistic that really can be easily mined.

          I'll put up patches with your query fix. (if you're not up to putting one git repo up yourself that is)

          cheers,
          Aparup

          Show
          nebgor Aparup Banerjee added a comment - Hi Davo, Thanks for the report! The alternative might introduce a few more queries for a statistic that really can be easily mined. I'll put up patches with your query fix. (if you're not up to putting one git repo up yourself that is) cheers, Aparup
          Hide
          davosmith Davo Smith added a comment -

          I can put together a git repo, if you prefer, but not until later this evening.

          Show
          davosmith Davo Smith added a comment - I can put together a git repo, if you prefer, but not until later this evening.
          Hide
          nebgor Aparup Banerjee added a comment -

          That'll be great Davo. Its just that the authorship information recorded would be more accurate, especially since we can do this now that we're on git.

          When you have a branch with that commit, i can pick (cherry-pick) your commits across to other needy branches.

          Show
          nebgor Aparup Banerjee added a comment - That'll be great Davo. Its just that the authorship information recorded would be more accurate, especially since we can do this now that we're on git. When you have a branch with that commit, i can pick (cherry-pick) your commits across to other needy branches.
          Hide
          davosmith Davo Smith added a comment -

          Added patch to Github repository

          Show
          davosmith Davo Smith added a comment - Added patch to Github repository
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks Davo!

          just checked and the fix isn't needed for 2.x as that bit was rewritten.

          putting this up for integration.

          Integrators: Davo's fix is also available rebased @
          git://github.com/nebgor/moodle.git
          https://github.com/nebgor/moodle/compare/MOODLE_19_STABLE...MDL-28690_m19

          Show
          nebgor Aparup Banerjee added a comment - Thanks Davo! just checked and the fix isn't needed for 2.x as that bit was rewritten. putting this up for integration. Integrators: Davo's fix is also available rebased @ git://github.com/nebgor/moodle.git https://github.com/nebgor/moodle/compare/MOODLE_19_STABLE...MDL-28690_m19
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          nebgor Aparup Banerjee added a comment -

          thanks, rebased my repo.

          Show
          nebgor Aparup Banerjee added a comment - thanks, rebased my repo.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I've added one more commit adding the "la" prefix to all the fields. Unnecessary right now but better to avoid potential future collisions.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I've added one more commit adding the "la" prefix to all the fields. Unnecessary right now but better to avoid potential future collisions.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Works Great.
          Thanks for fixing this Davo and Aparup

          Show
          rajeshtaneja Rajesh Taneja added a comment - Works Great. Thanks for fixing this Davo and Aparup
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Oct/11