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

      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
      

        Gliffy Diagrams

          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: