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

      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

        Gliffy Diagrams

        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: