Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Testing Instructions

      Basic Functionality

      • Open a course;
      • turn editing on;
      • ensure that both the activities and resources dropdowns are shown; and
      • confirm that all expected modules appear in the lists.

      Disabling and re-enabling plugins

      • navigate to Settings -> Site Administration -> Plugins -> Activity modules;
      • disable the assignment plugin, another activity (e.g. Chat), and a resource (e.g. Page);
      • refresh the course page and confirm that the assignment activity and all subtypes have disappeared, and the other activity and resource you disabled; and
      • re-enable the plugins and refresh your course page to confirm that they're back.

      Correct Functionality

      • try clicking through to each plugin page in turn to ensure that the you are redirected to the module editing page for the correct plugins.
        This is particularly important for assignment subtypes.
      Show
      Testing Instructions Basic Functionality Open a course; turn editing on; ensure that both the activities and resources dropdowns are shown; and confirm that all expected modules appear in the lists. Disabling and re-enabling plugins navigate to Settings -> Site Administration -> Plugins -> Activity modules; disable the assignment plugin, another activity (e.g. Chat), and a resource (e.g. Page); refresh the course page and confirm that the assignment activity and all subtypes have disappeared, and the other activity and resource you disabled; and re-enable the plugins and refresh your course page to confirm that they're back. Correct Functionality try clicking through to each plugin page in turn to ensure that the you are redirected to the module editing page for the correct plugins. This is particularly important for assignment subtypes.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30616-master-7

      Description

      In course/lib.php, the print_section_add_menus function does a lot of work to retrieve module metadata (e.g. name and a URL for creating a new instance of the module).

      To create the module chooser in the most efficient manner, we should split out the retrieval of this metadata into a separate function that other functions can use.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Andrew,

            Looks like a nice clean abstraction thanks.
            The only thing I noted was the spacing in the foreach within get_module_metadata, totally trivial. http://docs.moodle.org/dev/Coding_style#Foreach

            Feel free to put this up for integration when you are ready.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Andrew, Looks like a nice clean abstraction thanks. The only thing I noted was the spacing in the foreach within get_module_metadata, totally trivial. http://docs.moodle.org/dev/Coding_style#Foreach Feel free to put this up for integration when you are ready. Cheers Sam
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Sam,

            I've corrected that foreach spacing - IIRC it was in the original and I preserved that section of code.

            I've also rebased on this weeks master.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Sam, I've corrected that foreach spacing - IIRC it was in the original and I preserved that section of code. I've also rebased on this weeks master.
            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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Uhm... I bet this will conflict with some changes accepted last week about MOD_ARCHETYPE_SYSTEM... checking...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Uhm... I bet this will conflict with some changes accepted last week about MOD_ARCHETYPE_SYSTEM... checking...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yeah, confirmed.

            Although the conflicts are easily solvable, I think it's better to move this to next week, so you can review if the new archetype needs also to be handled by your new get_module_metadata() function.

            So reopening, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yeah, confirmed. Although the conflicts are easily solvable, I think it's better to move this to next week, so you can review if the new archetype needs also to be handled by your new get_module_metadata() function. So reopening, ciao
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've updated my patch to resolve the merge conflict and tested against the existing test instructions for both bugs

            Show
            dobedobedoh Andrew Nicols added a comment - I've updated my patch to resolve the merge conflict and tested against the existing test instructions for both bugs
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Updated on latest master

            Show
            dobedobedoh Andrew Nicols added a comment - Updated on latest master
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Rebased on weekly master - ready for peer review again

            Show
            dobedobedoh Andrew Nicols added a comment - Rebased on weekly master - ready for peer review again
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Updated on latest master

            Show
            dobedobedoh Andrew Nicols added a comment - Updated on latest master
            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
            dobedobedoh Andrew Nicols added a comment -

            Rebases cleanly onto latest master

            Show
            dobedobedoh Andrew Nicols added a comment - Rebases cleanly onto latest master
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday, with point releases happening next week based on current status.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday, with point releases happening next week based on current status. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Interesting one. Let's break master! Yay! I like it. (joking, but can imagine at least 2 ongoing developments that will need to perform 2 adjustments here). Luckily, you arrived first.

            Integrated, thanks! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Interesting one. Let's break master! Yay! I like it. (joking, but can imagine at least 2 ongoing developments that will need to perform 2 adjustments here). Luckily, you arrived first. Integrated, thanks! Ciao
            Hide
            samhemelryk Sam Hemelryk added a comment -

            It appears there is a bug with this presently that is preventing a user from creating a new assignment.
            I'm looking into this now.

            Show
            samhemelryk Sam Hemelryk added a comment - It appears there is a bug with this presently that is preventing a user from creating a new assignment. I'm looking into this now.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            here "Let's break master" becomes reality. Better I keep my mouth closed next time.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - here "Let's break master" becomes reality. Better I keep my mouth closed next time.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Problem found, I've applied a patch now for it:

            commit 772685e9830abfeb8dc6c0a9fb4ee104804eb45d
            Author: Sam Hemelryk <sam@moodle.com>
            Date:   Wed Mar 14 13:52:18 2012 +1300
             
                MDl-30616 course: Fixed bug with module subtype links
             
            diff --git a/course/lib.php b/course/lib.php
            index 4a31b80..6282112 100644
            --- a/course/lib.php
            +++ b/course/lib.php
            @@ -1964,7 +1964,7 @@ function get_module_metadata($course, $modnames) {
                                 if (get_string_manager()->string_exists('help' . $subtype->name, $modname)) {
                                     $subtype->help = get_string('help' . $subtype->name, $modname);
                                 }
            -                    $subtype->link = $urlbase . $type->type;
            +                    $subtype->link = $urlbase . $subtype->type;
                                 $group->types[] = $subtype;
                             }
                             $modlist[$course->id][$modname] = $group;

            Changes look real good btw Andrew!

            Show
            samhemelryk Sam Hemelryk added a comment - Problem found, I've applied a patch now for it: commit 772685e9830abfeb8dc6c0a9fb4ee104804eb45d Author: Sam Hemelryk <sam@moodle.com> Date: Wed Mar 14 13:52:18 2012 +1300   MDl-30616 course: Fixed bug with module subtype links   diff --git a/course/lib.php b/course/lib.php index 4a31b80..6282112 100644 --- a/course/lib.php +++ b/course/lib.php @@ -1964,7 +1964,7 @@ function get_module_metadata($course, $modnames) { if (get_string_manager()->string_exists('help' . $subtype->name, $modname)) { $subtype->help = get_string('help' . $subtype->name, $modname); } - $subtype->link = $urlbase . $type->type; + $subtype->link = $urlbase . $subtype->type; $group->types[] = $subtype; } $modlist[$course->id][$modname] = $group; Changes look real good btw Andrew!
            Hide
            phalacee Jason Fowler added a comment -

            All good

            Show
            phalacee Jason Fowler added a comment - All good
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12