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

System mods shown in activity chooser

    Details

    • Testing Instructions:
      Hide
      1. Install the attached plugin with archetype MOD_ARCHETYPE_SYSTEM
      2. Launch the activity chooser
      3. The module shouldn't appear
      4. VERIFY: all existing resources appear
      5. VERIFY: all existing activities appear
      Show
      Install the attached plugin with archetype MOD_ARCHETYPE_SYSTEM Launch the activity chooser The module shouldn't appear VERIFY: all existing resources appear VERIFY: all existing activities appear
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-34762-master

      Description

      mods with archetype MOD_ARCHETYPE_SYSTEM aren't shown in the old-style dropdown menus but are shown in the activity chooser.

      To replicate bug:

      1. Install a mod with archetype MOD_ARCHETYPE_SYSTEM (see attached gettypestest.zip)
      2. Launch the activity chooser.
      3. You will see the mod (but shouldn't)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            maherne Michael Aherne added a comment -

            Attached a patch and a test mod.

            Show
            maherne Michael Aherne added a comment - Attached a patch and a test mod.
            Hide
            maherne Michael Aherne added a comment -

            Updated pull requests.

            Show
            maherne Michael Aherne added a comment - Updated pull requests.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for spotting that and providing a fix, Michael.

            Andrew: could you please peer review this and push it to integration if you deem it ready?

            Show
            salvetore Michael de Raadt added a comment - Thanks for spotting that and providing a fix, Michael. Andrew: could you please peer review this and push it to integration if you deem it ready?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Michael A,

            The patch looks fine on the whole, just a few comments/questions below. Apologies for my delay in reviewing it.

            [N] Syntax
            [Y] Output
            [N] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [Y] Security
            [-] Documentation
            [N] Git
            [Y] Sanity check

            Syntax

            Just wondering why you've switched from create_function to function - is there a specific reason?

            Whitespace

            Ideally the function($mod) should be on the same line now that you've changed it to a function from create_function which was all on one line

            Git

            Could you add the component (Course) to the commit message.

            If you can rebase on the latest master and address the above, I'll submit it for integration.

            Thanks,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Michael A, The patch looks fine on the whole, just a few comments/questions below. Apologies for my delay in reviewing it. [N] Syntax [Y] Output [N] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [N] Git [Y] Sanity check Syntax Just wondering why you've switched from create_function to function - is there a specific reason? Whitespace Ideally the function($mod) should be on the same line now that you've changed it to a function from create_function which was all on one line Git Could you add the component (Course) to the commit message. If you can rebase on the latest master and address the above, I'll submit it for integration. Thanks, Andrew
            Hide
            maherne Michael Aherne added a comment -

            Hi Andrew, thanks for the feedback! I just changed from create_function as I was under the impression it was an older and less reliable way to do it, and that this was one of the things that Moodle would benefit from requiring PHP 5.3 for. I could be wrong about that though - happy to change it back again if you'd prefer.

            I've reformatted the patch and rebased to the latest master / MOODLE_23_STABLE without issues, so I'm happy for it to go to integration.

            Show
            maherne Michael Aherne added a comment - Hi Andrew, thanks for the feedback! I just changed from create_function as I was under the impression it was an older and less reliable way to do it, and that this was one of the things that Moodle would benefit from requiring PHP 5.3 for. I could be wrong about that though - happy to change it back again if you'd prefer. I've reformatted the patch and rebased to the latest master / MOODLE_23_STABLE without issues, so I'm happy for it to go to integration.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Michael,

            Sorry - I meant that the function() line shoudl be on the same way as array_filter, e.g.:

            array_filter($modules, function($mod) {
              ...
            });

            Otherwise looks good. Looking into the create_function() side of things, it appears that they're pretty inefficient too so probably good that we move away from them.

            Cheers

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Michael, Sorry - I meant that the function() line shoudl be on the same way as array_filter, e.g.: array_filter($modules, function($mod) { ... }); Otherwise looks good. Looking into the create_function() side of things, it appears that they're pretty inefficient too so probably good that we move away from them. Cheers
            Hide
            maherne Michael Aherne added a comment -

            Ah, I see! I've made the change now - cheers.

            Show
            maherne Michael Aherne added a comment - Ah, I see! I've made the change now - cheers.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Looks good to me. Submitting

            Show
            dobedobedoh Andrew Nicols added a comment - Looks good to me. Submitting
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski added a comment -

            Hi Michael,

            I've integrated this to 23 and master, thanks.

            Your commit author details didn't seem to be correctly formed: "Author: Michael Aherne <Michael Aherne>", so i corrected you're email address before pushing.

            cheers,
            Dan

            Show
            poltawski Dan Poltawski added a comment - Hi Michael, I've integrated this to 23 and master, thanks. Your commit author details didn't seem to be correctly formed: "Author: Michael Aherne <Michael Aherne>", so i corrected you're email address before pushing. cheers, Dan
            Hide
            dmonllao David Monllaó added a comment -

            Tested in 24 and 23. It passes

            Show
            dmonllao David Monllaó added a comment - Tested in 24 and 23. It passes
            Hide
            maherne Michael Aherne added a comment -

            Thanks, everyone, and thanks for pointing out the git email problem, Dan. I changed my git client a few weeks ago and seem to have set the global email wrongly, so most of my recent pull requests may have this problem.

            Show
            maherne Michael Aherne added a comment - Thanks, everyone, and thanks for pointing out the git email problem, Dan. I changed my git client a few weeks ago and seem to have set the global email wrongly, so most of my recent pull requests may have this problem.
            Hide
            maherne Michael Aherne added a comment -

            Should this issue have 2.4 as a fix version as well as 2.3.4?

            Show
            maherne Michael Aherne added a comment - Should this issue have 2.4 as a fix version as well as 2.3.4?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            We don't include unreleased versions in the fix version unless it's the only version. It's been fixed since 2.3.4.

            I agree - it is confusing :\

            Show
            dobedobedoh Andrew Nicols added a comment - We don't include unreleased versions in the fix version unless it's the only version. It's been fixed since 2.3.4. I agree - it is confusing :\
            Hide
            maherne Michael Aherne added a comment -

            Ah, I get it now

            Show
            maherne Michael Aherne added a comment - Ah, I get it now
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many, many thanks for your effort!

            Millions of people will enjoy the results of your work, yay!

            Closing as fixed. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13