Moodle
  1. Moodle
  2. MDL-20131

Glossary_random block does not list and use global glossaries

    Details

    • Testing Instructions:
      Hide
      1. Login as admin
      2. Create glossary in a course and make sure Is this glossary global? is checked.
      3. Add few entries in glossary.
      4. Go to front page and add Random glossary entry block
      5. Configure Random glossary entry block and make sure you can see created glossary name in "Take entries from this glossary"
      6. Save changes to configuration with "Take entries from this glossary" as global glossary.
      7. Check Random glossary block and make sure you can see entries.

      Test 2:

      1. Login as guest and make sure you can see Random global glossary with some entries.
      2. As admin remove 'mod/glossary:view' capability for guest on course level (Course ► Users ► Permissions).
      3. login as guest and make sure you can't see Random glossary entries.

      Test 3:

      1. Log in as admin and hide glossary activity
      2. Go to front page and you should see "(to be continued)" in Random glossary entry block.

      Test 4:

      1. Log in as admin and uncheck Is this glossary global? on glossary
      2. Go to frontpage and you should see "Please configure this block using the edit icon."
      Show
      Login as admin Create glossary in a course and make sure Is this glossary global? is checked. Add few entries in glossary. Go to front page and add Random glossary entry block Configure Random glossary entry block and make sure you can see created glossary name in "Take entries from this glossary" Save changes to configuration with "Take entries from this glossary" as global glossary. Check Random glossary block and make sure you can see entries. Test 2: Login as guest and make sure you can see Random global glossary with some entries. As admin remove 'mod/glossary:view' capability for guest on course level (Course ► Users ► Permissions). login as guest and make sure you can't see Random glossary entries. Test 3: Log in as admin and hide glossary activity Go to front page and you should see "(to be continued)" in Random glossary entry block. Test 4: Log in as admin and uncheck Is this glossary global? on glossary Go to frontpage and you should see "Please configure this block using the edit icon."
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-20131
    • Rank:
      5500

      Description

      If a site admin creates a global glossary in a course and then wants to use the glossary_random block to display random entries on the frontpage it is not possible because currently only glossaries of that course are displayed; however, I would expect a global glossary to be visible and usable as well. This issue was raised in http://moodle.org/mod/forum/discuss.php?d=108686 but never responded to. I now have a client that wants to be able to do this so I am looking into it. Peace - Anthony

      1. MDL-20131.patch
        4 kB
        Anthony Borrow
      2. MDL-20131.patch
        2 kB
        Anthony Borrow

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Here is a possible patch - essentially I opted to lookup the course information rather than assuming that it is the current course. This makes it possible to select glossaries that are not in the course.

          Show
          Anthony Borrow added a comment - Here is a possible patch - essentially I opted to lookup the course information rather than assuming that it is the current course. This makes it possible to select glossaries that are not in the course.
          Hide
          Anthony Borrow added a comment -

          Re-assigning to moodle.com - I have attached a patch file and would appreciate a review of it. If it seems OK to apply to core then it can be re-assigned to me and I will take care of it. Peace - Anthony

          Show
          Anthony Borrow added a comment - Re-assigning to moodle.com - I have attached a patch file and would appreciate a review of it. If it seems OK to apply to core then it can be re-assigned to me and I will take care of it. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Attaching what I consider a cleaner version (using existing $course rather than creating a new $glossarycourse since $course is not used elsewhere) also better checking to reduce PHP notices when the chosen glossary is deleted. Peace - Anthony

          Show
          Anthony Borrow added a comment - Attaching what I consider a cleaner version (using existing $course rather than creating a new $glossarycourse since $course is not used elsewhere) also better checking to reduce PHP notices when the chosen glossary is deleted. Peace - Anthony
          Hide
          John White added a comment -

          This problem persists into Moodle 2.0, so based on Anthony's work I have proposed a fix for this in 2.0.1
          See http://moodle.org/mod/forum/discuss.php?d=89732#p732166
          Regards, John

          Show
          John White added a comment - This problem persists into Moodle 2.0, so based on Anthony's work I have proposed a fix for this in 2.0.1 See http://moodle.org/mod/forum/discuss.php?d=89732#p732166 Regards, John
          Hide
          Anthony Borrow added a comment -

          Per John White's post at http://moodle.org/mod/forum/discuss.php?d=140973#p732519, I am adding 2.0 as an affect version and changing fix version to STABLE backlog so that the patch I provided and the one John mentions at http://moodle.org/mod/forum/discuss.php?d=89732 can be evaluated and tweaked as necessary to help resolve this issue. Peace - Anthony

          Show
          Anthony Borrow added a comment - Per John White's post at http://moodle.org/mod/forum/discuss.php?d=140973#p732519 , I am adding 2.0 as an affect version and changing fix version to STABLE backlog so that the patch I provided and the one John mentions at http://moodle.org/mod/forum/discuss.php?d=89732 can be evaluated and tweaked as necessary to help resolve this issue. Peace - Anthony
          Hide
          Chris Baldwin added a comment -

          Still seems to be a problem in 2.2. Any news on getting it fixed?

          Thanks

          Chris

          Show
          Chris Baldwin added a comment - Still seems to be a problem in 2.2. Any news on getting it fixed? Thanks Chris
          Hide
          Joseph Rézeau added a comment -

          Still not fixed.

          Show
          Joseph Rézeau added a comment - Still not fixed.
          Hide
          Martin Dougiamas added a comment -

          bumping up the tree

          Show
          Martin Dougiamas added a comment - bumping up the tree
          Hide
          Helen Foster added a comment -

          Adding 2.3.4 as affects version, as reported by Pavel Gonzalez Garcia.

          Show
          Helen Foster added a comment - Adding 2.3.4 as affects version, as reported by Pavel Gonzalez Garcia.
          Hide
          Rajesh Taneja added a comment -

          Taking this in next sprint, so I can have a look at it.

          Show
          Rajesh Taneja added a comment - Taking this in next sprint, so I can have a look at it.
          Hide
          Andrew Davis added a comment -

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

          It was incorrect when you got here but would you mind fixing up the indenting in instance_allow_multiple()?

          At line 160
          if (!$glossary = $DB->get_record('glossary', array('id' => $this->config->glossary)))

          { return ''; }

          Is it worth putting up an error in this situation? Will it ever be the case that you'll wind up with a bad glossary ID and it isn't a sign that something is very wrong?

          // Set globalglossary flag in config.
          $this->config->globalglossary = $glossary->globalglossary;

          Try and avoid comments like this that are saying what is being done. I can read the code and see what is being done. Comments that explain why are better (although in this case I think its obvious so the comment isn't necessary at all).

          "$glossaryid = $this->config->glossary;"
          Can you double check and, if possible, remote the $glossaryid variable entirely. There doesn't seem to be any good reason to have two variables instead of just using $this->config->glossary everywhere.

          Would it be possible to get some unit tests covering this?

          Show
          Andrew Davis added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [Y] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check It was incorrect when you got here but would you mind fixing up the indenting in instance_allow_multiple()? At line 160 if (!$glossary = $DB->get_record('glossary', array('id' => $this->config->glossary))) { return ''; } Is it worth putting up an error in this situation? Will it ever be the case that you'll wind up with a bad glossary ID and it isn't a sign that something is very wrong? // Set globalglossary flag in config. $this->config->globalglossary = $glossary->globalglossary; Try and avoid comments like this that are saying what is being done. I can read the code and see what is being done. Comments that explain why are better (although in this case I think its obvious so the comment isn't necessary at all). "$glossaryid = $this->config->glossary;" Can you double check and, if possible, remote the $glossaryid variable entirely. There doesn't seem to be any good reason to have two variables instead of just using $this->config->glossary everywhere. Would it be possible to get some unit tests covering this?
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Andrew,

          1. Not sure if error is a good option here, as global glossary is created in course and it's a block which stores glossay id and can be placed outside course. There can be a case where block is placed with some global glossary and global glossary is deleted. In such case we should unset globaglossay id and ask user to reconfigure.
          2. I will remove comment.
          3. $glossaryid is easy to read though code, so will remove double usage of $this->config->glossary;
          4. Good point, will add unit test for this.
          Show
          Rajesh Taneja added a comment - Thanks for the review Andrew, Not sure if error is a good option here, as global glossary is created in course and it's a block which stores glossay id and can be placed outside course. There can be a case where block is placed with some global glossary and global glossary is deleted. In such case we should unset globaglossay id and ask user to reconfigure. I will remove comment. $glossaryid is easy to read though code, so will remove double usage of $this->config->glossary; Good point, will add unit test for this.
          Hide
          Rajesh Taneja added a comment -

          I have fixed comments and other stuff as suggested by Andrew.

          As there is no phpunit/generator for glossary and block_glossary_random needs glossary to be available, I have created MDL-37938, so it can be handled separately.

          Submitting for integration review.

          Show
          Rajesh Taneja added a comment - I have fixed comments and other stuff as suggested by Andrew. As there is no phpunit/generator for glossary and block_glossary_random needs glossary to be available, I have created MDL-37938 , so it can be handled separately. Submitting for integration review.
          Hide
          Damyon Wiese added a comment -

          Hi Raj,

          I'm getting debugging messages with this patch. I added a global glossary, then added the random glossary entry block to my front page. When I try and configure the block I get:

          Notice: Undefined property: block_glossary_random::$course in /home/damyonw/Documents/Moodle/integration/master/moodle/blocks/glossary_random/edit_form.php on line 44 Notice: Trying to get property of non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/blocks/glossary_random/edit_form.php on line 44

          Can you add a fix for this and I'll take another look?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Raj, I'm getting debugging messages with this patch. I added a global glossary, then added the random glossary entry block to my front page. When I try and configure the block I get: Notice: Undefined property: block_glossary_random::$course in /home/damyonw/Documents/Moodle/integration/master/moodle/blocks/glossary_random/edit_form.php on line 44 Notice: Trying to get property of non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/blocks/glossary_random/edit_form.php on line 44 Can you add a fix for this and I'll take another look? Thanks, Damyon
          Hide
          Rajesh Taneja added a comment -

          Thanks for finding this Damyon.
          Not sure why I didn't see this at first place.

          Have put a fix now, back for your review.

          Show
          Rajesh Taneja added a comment - Thanks for finding this Damyon. Not sure why I didn't see this at first place. Have put a fix now, back for your review.
          Hide
          Damyon Wiese added a comment -

          Hi Raj,

          I took another look and it's almost perfect

          The one thing I did find is that if I have an existing random glossary block on my front page configured to point to a glossary on my front page, after installing your patch it needs to be configured again.

          Can you add a fix for this?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Raj, I took another look and it's almost perfect The one thing I did find is that if I have an existing random glossary block on my front page configured to point to a glossary on my front page, after installing your patch it needs to be configured again. Can you add a fix for this? Thanks, Damyon
          Hide
          Rajesh Taneja added a comment -

          Grrrr..
          Thanks for spotting that Damyon, there was a typo for courseid.

          I have fixed this now.
          FYI: added a new commit so you can see the changes.

          Show
          Rajesh Taneja added a comment - Grrrr.. Thanks for spotting that Damyon, there was a typo for courseid. I have fixed this now. FYI: added a new commit so you can see the changes.
          Hide
          Damyon Wiese added a comment -

          Thanks Raj, this looks good now.

          Pushed to master only.

          Show
          Damyon Wiese added a comment - Thanks Raj, this looks good now. Pushed to master only.
          Hide
          Andrew Davis added a comment -

          Works as described. Well done covering all of these different scenarios. Passing.

          Show
          Andrew Davis added a comment - Works as described. Well done covering all of these different scenarios. Passing.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is now documented here http://docs.moodle.org/25/en/Random_glossary_entry_block

          Show
          Mary Cooch added a comment - Removing docs_required label as this is now documented here http://docs.moodle.org/25/en/Random_glossary_entry_block

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: