Moodle
  1. Moodle
  2. MDL-38997

Notices due to badge config settings not being checked before use

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      Almost every front end page that uses badges is throwing a warning about undefined property if badges haven't been enabled explicitly.

      1. Login as a user with permissions to manage badges (admin or teacher).
      2. Go to the following pages:
      /badges/view.php
      /badges/index.php
      /badges/overview.php
      /badges/mybadges.php
      /badges/mybackpack.php
      /badges/recipients.php
      /badges/newbadge.php
      /badges/criteria.php
      /badges/edit.php
      3. Add badges block to My home page.
      4. There should be no warning messages like "Notice: Undefined property..." associated with badges_allowcoursebadges, badges_allowexternalbackpack, and enablebadges

      Show
      Almost every front end page that uses badges is throwing a warning about undefined property if badges haven't been enabled explicitly. 1. Login as a user with permissions to manage badges (admin or teacher). 2. Go to the following pages: /badges/view.php /badges/index.php /badges/overview.php /badges/mybadges.php /badges/mybackpack.php /badges/recipients.php /badges/newbadge.php /badges/criteria.php /badges/edit.php 3. Add badges block to My home page. 4. There should be no warning messages like "Notice: Undefined property..." associated with badges_allowcoursebadges , badges_allowexternalbackpack , and enablebadges
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
      git@github.com:totara/moodle.git
    • Pull Master Branch:
      integration_MDL-38997
    • Rank:
      49104

      Description

      There are a few examples of code like this:

      if ($CFG->enablebadges)

      { ... }

      Which will lead to warnings if the setting hasn't been set to anything:

      Notice: Undefined property: stdClass::$badges_allowcoursebadges in /lib/badgeslib.php on line 894
      Call Stack
      #	Time	Memory	Function	Location
      1	0.0001	237112	{main}( )	../view.php:0
      2	0.0862	6051656	core_renderer->header( )	../view.php:174
      3	0.1243	6280848	core_renderer->render_page_layout( )	../outputrenderers.php:768
      4	0.1243	6298864	include( '/Users/stronk7/git_moodle/moodle/theme/base/layout/general.php' )	../outputrenderers.php:838
      5	0.1244	6300368	moodle_page->has_navbar( )	../general.php:4
      6	0.1271	6541840	navbar->has_items( )	../pagelib.php:766
      7	0.1273	6550232	global_navigation->initialise( )	../navigationlib.php:2943
      8	0.2474	7950904	local_loginas_extends_navigation( )	../navigationlib.php:1251
      9	0.2474	7951504	moodle_page->__get( )	../lib.php:35
      10	0.2474	7951592	moodle_page->magic_get_settingsnav( )	../pagelib.php:717
      11	0.2475	7954176	settings_navigation->initialise( )	../pagelib.php:701
      12	0.2616	8251528	settings_navigation->load_course_settings( )	../navigationlib.php:3186
      13	0.2783	8731008	badges_add_course_navigation( )	../navigationlib.php:3548
      

      and also:

      Notice: Undefined property: stdClass::$badges_allowexternalbackpack in /lib/badgeslib.php on line 1231
      Call Stack
      #	Time	Memory	Function	Location
      1	0.0001	236176	{main}( )	../profile.php:0
      2	0.8705	16211216	profile_display_badges( )	../profile.php:409
      

        Activity

        Hide
        Simon Coggins added a comment -

        I have pushed a proposed fix to a branch (based off current integration).

        Show
        Simon Coggins added a comment - I have pushed a proposed fix to a branch (based off current integration).
        Hide
        Mary Evans added a comment -

        Good job I spied that ERROR!

        Show
        Mary Evans added a comment - Good job I spied that ERROR!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Plz, when an error happens, add it (with the stack if possible), to the description. It helps a lot when looking for existing issues and prevent the creation of duplicates (from impulsive developers, like me, lol).

        Thanks for working on this! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Plz, when an error happens, add it (with the stack if possible), to the description. It helps a lot when looking for existing issues and prevent the creation of duplicates (from impulsive developers, like me, lol). Thanks for working on this! Ciao
        Hide
        Simon Coggins added a comment -

        Okay, done!

        Show
        Simon Coggins added a comment - Okay, done!
        Hide
        Mary Evans added a comment - - edited

        @Eloy: There was NO Stack Trace otherwise I would have added those too, as I know how important that is, (to impulsive developers, like you! LOL).

        Show
        Mary Evans added a comment - - edited @Eloy: There was NO Stack Trace otherwise I would have added those too, as I know how important that is, (to impulsive developers, like you! LOL).
        Hide
        Jason Fowler added a comment -

        [Y] Syntax
        [-] Output
        [Y] Whitespace
        [-] Language
        [-] Databases
        [N] Testing – We need testing instructions
        [-] Security
        [-] Documentation
        [Y] Git
        [N] Sanity check – there are times you are simply testing a CFG variable exists, but not what it is set to. If I set it to false, these conditions will still trigger where they should only trigger if it is set to true.

        Show
        Jason Fowler added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing – We need testing instructions [-] Security [-] Documentation [Y] Git [N] Sanity check – there are times you are simply testing a CFG variable exists, but not what it is set to. If I set it to false, these conditions will still trigger where they should only trigger if it is set to true.
        Hide
        Yuliya Bozhko added a comment -

        All of these variables (except for badges_defaultissuercontact and badges_defaultissuername) are boolean values. As far as I know, empty() will return true if a variable doesn't exist as well as if it is set to false. So, I don't think we need any additional checks. Is there a specific place where you think an additional check will change output?

        Show
        Yuliya Bozhko added a comment - All of these variables (except for badges_defaultissuercontact and badges_defaultissuername ) are boolean values. As far as I know, empty() will return true if a variable doesn't exist as well as if it is set to false . So, I don't think we need any additional checks. Is there a specific place where you think an additional check will change output?
        Hide
        Jason Fowler added a comment -

        Ah, I see, false is considered empty. that's fine. wasn't aware of that. Thanks.

        If you could write instructions for a tester to follow so we can test the functionality of your code, we can get this through to integration.

        Show
        Jason Fowler added a comment - Ah, I see, false is considered empty. that's fine. wasn't aware of that. Thanks. If you could write instructions for a tester to follow so we can test the functionality of your code, we can get this through to integration.
        Hide
        Yuliya Bozhko added a comment -

        Done

        Show
        Yuliya Bozhko added a comment - Done
        Hide
        Jason Fowler added a comment -

        Great, pushing for integration now.

        Show
        Jason Fowler added a comment - Great, pushing for integration now.
        Hide
        Dan Poltawski added a comment -

        Thanks Simon/Yuliya - looks perfect, i've integrated this now

        Show
        Dan Poltawski added a comment - Thanks Simon/Yuliya - looks perfect, i've integrated this now
        Hide
        Andrew Davis added a comment -

        Seems to be working as described. Passing.

        Show
        Andrew Davis added a comment - Seems to be working as described. Passing.
        Hide
        Dan Poltawski added a comment -

        Blooming Marvelous! It's time for a knees up - your changes are upstream!

        Thanks for making Moodle better!

        Toodle pip

        Show
        Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

          People

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

            Dates

            • Created:
              Updated:
              Resolved: