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

          Attachments

            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