Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Calendar
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Log in as admin. Check that the site setting calendar_adminseesall is checked.

      Check that your site has debug messages set to developer and display debug messages checked.

      If it isnt already add a calendar block to your site's front page.

      Go into a course and create two course events within that single course. Make them in the near future. http://docs.moodle.org/20/en/Using_Calendar#Adding_an_event

      Return to your site's home page. The two events should appear in the calendar. No errors should be displayed.

      Show
      Log in as admin. Check that the site setting calendar_adminseesall is checked. Check that your site has debug messages set to developer and display debug messages checked. If it isnt already add a calendar block to your site's front page. Go into a course and create two course events within that single course. Make them in the near future. http://docs.moodle.org/20/en/Using_Calendar#Adding_an_event Return to your site's home page. The two events should appear in the calendar. No errors should be displayed.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-31086
    • Rank:
      37511

      Description

      function calendar_get_default_courses() generates sql like this:

      SELECT c.* , ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance FROM {course} c JOIN {event} e ON e.courseid = c.id LEFT JOIN {context} ctx ON (ctx.instanceid = c.id AND ctx.contextlevel = 50)
      

      Which throws multiple errors like this:

      Did you remember to make the first column something unique in your call to get_records? Duplicate value '4' found in column 'id'.
      
          line 705 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
          line 1359 of /calendar/lib.php: call to pgsql_native_moodle_database->get_records_sql()
          line 40 of /blocks/calendar_upcoming/block_calendar_upcoming.php: call to calendar_get_default_courses()
          line 280 of /blocks/moodleblock.class.php: call to block_calendar_upcoming->get_content()
          line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
          line 926 of /lib/blocklib.php: call to block_base->get_content_for_output()
          line 978 of /lib/blocklib.php: call to block_manager->create_block_contents()
          line 349 of /lib/blocklib.php: call to block_manager->ensure_content_created()
          line 4 of /theme/base/layout/frontpage.php: call to block_manager->region_has_content()
          line 685 of /lib/outputrenderers.php: call to include()
          line 637 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line ? of unknownfile: call to core_renderer->header()
          line 1363 of /lib/setuplib.php: call to call_user_func_array()
          line ? of unknownfile: call to bootstrap_renderer->__call()
          line 91 of /index.php: call to bootstrap_renderer->header()
      

      also reported here:
      http://moodle.org/mod/forum/discuss.php?d=186218
      http://moodle.org/mod/forum/discuss.php?d=182219

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for working on that, Dan.

          The devs have their heads down in docs at the moment, but hopefully we can get this peer reviewed and integrated soon.

          Show
          Michael de Raadt added a comment - Thanks for working on that, Dan. The devs have their heads down in docs at the moment, but hopefully we can get this peer reviewed and integrated soon.
          Hide
          Andrew Davis added a comment -

          That change will prevent the error being displayed but I'm not sure its correct. If there are multiple events per course we probably want the first column to be e.id rather than c.id

          Also, this needs testing instructions. Add a few course events, view the calendar. That sort of thing

          Show
          Andrew Davis added a comment - That change will prevent the error being displayed but I'm not sure its correct. If there are multiple events per course we probably want the first column to be e.id rather than c.id Also, this needs testing instructions. Add a few course events, view the calendar. That sort of thing
          Hide
          Dan Marsden added a comment -

          heh - yeah I didn't look too closely at the sql - makes sense that it should be using e.id but I haven't got any further time to look at this one at the moment sorry - feel free to re-assign to yourself or moodle.com

          -as for testing I haven't spent any time trying to reproduce it either - just noticed it on a clients site - then saw the forum posts.

          I expect you might need to have multiple course events for a single month and a single course?

          Show
          Dan Marsden added a comment - heh - yeah I didn't look too closely at the sql - makes sense that it should be using e.id but I haven't got any further time to look at this one at the moment sorry - feel free to re-assign to yourself or moodle.com -as for testing I haven't spent any time trying to reproduce it either - just noticed it on a clients site - then saw the forum posts. I expect you might need to have multiple course events for a single month and a single course?
          Hide
          Andrew Davis added a comment -

          I've had another look at this and I think your fix is actually correct Dan. I misunderstood the goal of the function.

          I've created some testing instructions and am putting Dan's original fix up for peer review.

          Show
          Andrew Davis added a comment - I've had another look at this and I think your fix is actually correct Dan. I misunderstood the goal of the function. I've created some testing instructions and am putting Dan's original fix up for peer review.
          Hide
          Dan Poltawski added a comment -

          Hmm - in terms of peer review - the fix works and makes sense. +1

          In terms of that function - I haven't reviewed the uses of it but from the look of it I am sure it should be using some other core piece of functionality to generate this list of courses.

          Show
          Dan Poltawski added a comment - Hmm - in terms of peer review - the fix works and makes sense. +1 In terms of that function - I haven't reviewed the uses of it but from the look of it I am sure it should be using some other core piece of functionality to generate this list of courses.
          Hide
          Dan Marsden added a comment -

          Hi Dan - definitely agree with you there - I don't think the calendar has had much love for a while... there seems to be general resistance to accepting improvements to the calendar as there have been noises about it being replaced at some point - would be nice to see this on the roadmap at some point.

          Show
          Dan Marsden added a comment - Hi Dan - definitely agree with you there - I don't think the calendar has had much love for a while... there seems to be general resistance to accepting improvements to the calendar as there have been noises about it being replaced at some point - would be nice to see this on the roadmap at some point.
          Hide
          Andrew Davis added a comment -

          Adding branches for 2.2 and 2.1.

          Show
          Andrew Davis added a comment - Adding branches for 2.2 and 2.1.
          Hide
          Andrew Davis added a comment -

          Submitting for integration.

          The calendar is indeed one of the areas of Moodle that receives little love

          Show
          Andrew Davis added a comment - Submitting for integration. The calendar is indeed one of the areas of Moodle that receives little love
          Hide
          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
          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
          Aparup Banerjee added a comment -

          this code looks good to integrate inthe coming cycle.

          note: to add 'Calendar :' to commit message. gotta show off calendar a bit

          Show
          Aparup Banerjee added a comment - this code looks good to integrate inthe coming cycle. note: to add 'Calendar :' to commit message. gotta show off calendar a bit
          Hide
          Aparup Banerjee added a comment -

          integrated and up for testing.

          Show
          Aparup Banerjee added a comment - integrated and up for testing.
          Hide
          Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this Andrew.

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

          Well,

          I wish I said it every time
          you do the things you do.
          You always lend a helping hand,
          and I'm filled with gratitude.

          You are strong and generous
          for each and everyone one of us.
          I am eternally grateful,
          I cannot say thanks enough.

          Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I wish I said it every time you do the things you do. You always lend a helping hand, and I'm filled with gratitude. You are strong and generous for each and everyone one of us. I am eternally grateful, I cannot say thanks enough. Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao
          Hide
          Andrew Davis added a comment -

          Ill raise this in dev chat on Monday but I dont think issue was actually integrated. Two reasons:

          1) I'm still getting the error on my master dev site.

          2) The diff URLs still report a difference between master and the branches. Branches that have been integrated usually say "master is up to date with all commits from (branch name)" after integration when you go to the diff URL.

          Show
          Andrew Davis added a comment - Ill raise this in dev chat on Monday but I dont think issue was actually integrated. Two reasons: 1) I'm still getting the error on my master dev site. 2) The diff URLs still report a difference between master and the branches. Branches that have been integrated usually say "master is up to date with all commits from (branch name)" after integration when you go to the diff URL.
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          Have you checked git.moodle.org ?

          Diff url can report difference if your master branch is not updated, you might want to check your git branches.

          In addition to this, I checked this on my integration branches (master, 22, 21) and patch is there.

          Show
          Rajesh Taneja added a comment - Hello Andrew, Have you checked git.moodle.org ? Diff url can report difference if your master branch is not updated, you might want to check your git branches. In addition to this, I checked this on my integration branches (master, 22, 21) and patch is there.
          Hide
          Dan Poltawski added a comment -

          Andrew, its definitely there:

          (MOODLE_21_STABLE=8e35d ?7) danp@marge ~/git/moodle> git checkout master
          Switched to branch 'master'
          (M=668a4 ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086
          commit 0ec75d9ffee0bbe3ab7f20718ac769bed1f3014f
          Author: Dan Marsden <dan@danmarsden.com>
          Date:   Tue Jan 10 13:57:44 2012 +1300
          
              MDL-31086 Calendar : fix sql to get distinct list of courses
          (M=668a4 ?7) danp@marge ~/git/moodle> git checkout MOODLE_22_STABLE
          Switched to branch 'MOODLE_22_STABLE'
          (MOODLE_22_STABLE=668a4 ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086
          commit 0ec75d9ffee0bbe3ab7f20718ac769bed1f3014f
          Author: Dan Marsden <dan@danmarsden.com>
          Date:   Tue Jan 10 13:57:44 2012 +1300
          
              MDL-31086 Calendar : fix sql to get distinct list of courses
          (MOODLE_22_STABLE=668a4 ?7) danp@marge ~/git/moodle> git checkout MOODLE_21_STABLE
          Switched to branch 'MOODLE_21_STABLE'
          (MOODLE_21_STABLE=8e35d ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086
          commit 72307d9a9f8858c2b3752a21e5ad5a0e4312fd34
          Author: Dan Marsden <dan@danmarsden.com>
          Date:   Tue Jan 10 13:57:44 2012 +1300
          
              MDL-31086 Calendar : fix sql to get distinct list of courses
          
          Show
          Dan Poltawski added a comment - Andrew, its definitely there: (MOODLE_21_STABLE=8e35d ?7) danp@marge ~/git/moodle> git checkout master Switched to branch 'master' (M=668a4 ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086 commit 0ec75d9ffee0bbe3ab7f20718ac769bed1f3014f Author: Dan Marsden <dan@danmarsden.com> Date: Tue Jan 10 13:57:44 2012 +1300 MDL-31086 Calendar : fix sql to get distinct list of courses (M=668a4 ?7) danp@marge ~/git/moodle> git checkout MOODLE_22_STABLE Switched to branch 'MOODLE_22_STABLE' (MOODLE_22_STABLE=668a4 ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086 commit 0ec75d9ffee0bbe3ab7f20718ac769bed1f3014f Author: Dan Marsden <dan@danmarsden.com> Date: Tue Jan 10 13:57:44 2012 +1300 MDL-31086 Calendar : fix sql to get distinct list of courses (MOODLE_22_STABLE=668a4 ?7) danp@marge ~/git/moodle> git checkout MOODLE_21_STABLE Switched to branch 'MOODLE_21_STABLE' (MOODLE_21_STABLE=8e35d ?7) danp@marge ~/git/moodle> git log --grep=MDL-31086 commit 72307d9a9f8858c2b3752a21e5ad5a0e4312fd34 Author: Dan Marsden <dan@danmarsden.com> Date: Tue Jan 10 13:57:44 2012 +1300 MDL-31086 Calendar : fix sql to get distinct list of courses
          Hide
          Andrew Davis added a comment -

          Must just be something screwy with my repo. Never mind

          Show
          Andrew Davis added a comment - Must just be something screwy with my repo. Never mind
          Hide
          Michael de Raadt added a comment -

          Oracle and SQL Server have issues with the use of DISTINCT on text fields. This should be resolved in MDL-32340.

          Show
          Michael de Raadt added a comment - Oracle and SQL Server have issues with the use of DISTINCT on text fields. This should be resolved in MDL-32340 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: