Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34654

Ampersand breaks auto-linking in Glossary

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4.6, 2.5.2
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Filters, Glossary
    • Labels:
    • Testing Instructions:
      Hide

      Note: For format_string text it generally is supposed to work whether or not you type the &

      1. Create new course or use existing. Go to Filter settings and check that 'Glossary auto-linking' is set to On. (This might also require a change to system settings if it isn't enabled at site level.)
      2. Create new Glossary. Type a name and description and select 'Main glossary' for 'Glossary type', then hit 'Save and display'.
      3. Add a new entry. Under Concept type 'D&D'; under Definition type 'Dungeons and Dragons'. Turn on 'This entry should be automatically linked' and save.
      4. Add another new entry. Under Concept type 'T&C'; under Definition type 'Terms and conditions'. Turn on the autolink option and save.
      4. Back on course page, create a new Page activity. Type a name and description and under Content, type 'What is D&D? How about T&C?'. Save and display.

      EXPECTED: Both terms should be highlighted as links. The tooltips for both terms should display correctly (without '&' HTML code). When you ctrl-click on both terms to open in a new tab, the relevant glossary entry should display.

      (NOTE: Due to what appears to be a separate bug in current master, normal left clicks on glossary links do not work on my system anyhow even if they don't have ampersands in, so I'm not including that in this issue.)

      Show
      Note: For format_string text it generally is supposed to work whether or not you type the & 1. Create new course or use existing. Go to Filter settings and check that 'Glossary auto-linking' is set to On. (This might also require a change to system settings if it isn't enabled at site level.) 2. Create new Glossary. Type a name and description and select 'Main glossary' for 'Glossary type', then hit 'Save and display'. 3. Add a new entry. Under Concept type 'D&D'; under Definition type 'Dungeons and Dragons'. Turn on 'This entry should be automatically linked' and save. 4. Add another new entry. Under Concept type 'T&C'; under Definition type 'Terms and conditions'. Turn on the autolink option and save. 4. Back on course page, create a new Page activity. Type a name and description and under Content, type 'What is D&D? How about T&C?'. Save and display. EXPECTED: Both terms should be highlighted as links. The tooltips for both terms should display correctly (without '&' HTML code). When you ctrl-click on both terms to open in a new tab, the relevant glossary entry should display. (NOTE: Due to what appears to be a separate bug in current master, normal left clicks on glossary links do not work on my system anyhow even if they don't have ampersands in, so I'm not including that in this issue.)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-34654-master

      Description

      Problem: A Glossary item containing an ampersand (&) character breaks auto-linking.

      Tested by removing and then adding the ampersand again, and behaviour is consistent with the & being the problem.

      We use APA style and are trying to use a glossary for a list of references. APA style uses this format for articles with multiple authors: eg. "Cooper, Orrell, & Bowden, 2003"

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put it on our backlog.

            In the meantime adding more information, such as screenshots, replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog. In the meantime adding more information, such as screenshots, replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
            Hide
            johannes.albert Johannes Albert added a comment -

            Workaround: Replace every "&" in the entry's name with "&". Downside: an ugly link hint.

            Show
            johannes.albert Johannes Albert added a comment - Workaround: Replace every "&" in the entry's name with "&". Downside: an ugly link hint.
            Hide
            quen Sam Marshall added a comment -

            For info, I'm taking a look at this issue - if it's reasonably easy I intend to fix it, as it's come up for us. (Otherwise will assign back to nobody.)

            Show
            quen Sam Marshall added a comment - For info, I'm taking a look at this issue - if it's reasonably easy I intend to fix it, as it's come up for us. (Otherwise will assign back to nobody.)
            Hide
            quen Sam Marshall added a comment -

            I've added a detailed test script about how I think it should work - probably. The test script currently fails.

            Show
            quen Sam Marshall added a comment - I've added a detailed test script about how I think it should work - probably. The test script currently fails.
            Hide
            quen Sam Marshall added a comment -

            In trying to write a unit test for this issue before fixing it, I discovered that there is currently no generator for glossary in unit tests, so I made one (MDL-42499) and have submitted that for inclusion.

            Probably the new generator will only be included in 2.6, so for the backports of this fix, I'll leave out the unit tests.

            Show
            quen Sam Marshall added a comment - In trying to write a unit test for this issue before fixing it, I discovered that there is currently no generator for glossary in unit tests, so I made one ( MDL-42499 ) and have submitted that for inclusion. Probably the new generator will only be included in 2.6, so for the backports of this fix, I'll leave out the unit tests.
            Hide
            quen Sam Marshall added a comment - - edited

            Code done, getting ready for review.

            NOTE 1: Unit tests are not included in the 2.4/2.5 branches because the test requires MDL-42499 which I think will be 2.6-only.

            NOTE 2: My solution to the problem is analagous to code in format_string (using same function format_string uses). I believe this to be correct, because the glossary concept is a TYPE_TEXT form field which is normally displayed with format_string.

            NOTE 3: While doing this code I noticed that the multilang filter can't work properly in the glossary auto-linking. This is a more fundamental problem so if anybody wants to fix it, please file a new issue.

            NOTE 4: I added a unit test to filter_glossary which only covers this issue (well, plus a sneaky 'normal link'). This commit is not supposed to be a complete unit test for filter_glossary but it's better than nothing; if full unit tests are desired, this should be a separate issue too.

            Show
            quen Sam Marshall added a comment - - edited Code done, getting ready for review. NOTE 1: Unit tests are not included in the 2.4/2.5 branches because the test requires MDL-42499 which I think will be 2.6-only. NOTE 2: My solution to the problem is analagous to code in format_string (using same function format_string uses). I believe this to be correct, because the glossary concept is a TYPE_TEXT form field which is normally displayed with format_string. NOTE 3: While doing this code I noticed that the multilang filter can't work properly in the glossary auto-linking. This is a more fundamental problem so if anybody wants to fix it, please file a new issue. NOTE 4: I added a unit test to filter_glossary which only covers this issue (well, plus a sneaky 'normal link'). This commit is not supposed to be a complete unit test for filter_glossary but it's better than nothing; if full unit tests are desired, this should be a separate issue too.
            Hide
            quen Sam Marshall added a comment -

            Think this is ready for review now.

            Show
            quen Sam Marshall added a comment - Think this is ready for review now.
            Hide
            quen Sam Marshall added a comment -

            There was already a glossary generator developed, so marking as blocking that instead.

            Show
            quen Sam Marshall added a comment - There was already a glossary generator developed, so marking as blocking that instead.
            Hide
            phalacee Jason Fowler added a comment -

            Thanks for the patch Sam, looks good, and thanks for the unit test.

            Show
            phalacee Jason Fowler added a comment - Thanks for the patch Sam, looks good, and thanks for the unit test.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            quen Sam Marshall added a comment -

            Thanks for review. I've updated the branch - and also updated my unit test (it didn't work properly against Marina's version of the glossary generator; passes now).

            Haven't updated the 24/25 branches as no change there.

            Show
            quen Sam Marshall added a comment - Thanks for review. I've updated the branch - and also updated my unit test (it didn't work properly against Marina's version of the glossary generator; passes now). Haven't updated the 24/25 branches as no change there.
            Hide
            damyon Damyon Wiese added a comment -

            The code immediately above this change looks suspicious of exactly the same bug. Raising a new issue to investigate.

            Show
            damyon Damyon Wiese added a comment - The code immediately above this change looks suspicious of exactly the same bug. Raising a new issue to investigate.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Sam,

            Integrated to 24, 25 and master.

            Note: I added a new issue for the suspect linking to category code immediately above your patch - I'm 99% sure it has the same issue.

            Show
            damyon Damyon Wiese added a comment - Thanks Sam, Integrated to 24, 25 and master. Note: I added a new issue for the suspect linking to category code immediately above your patch - I'm 99% sure it has the same issue.
            Hide
            skodak Petr Skoda added a comment -

            works as described, thanks

            Show
            skodak Petr Skoda added a comment - works as described, thanks
            Hide
            damyon Damyon Wiese added a comment -

            Here lies 52 bugs.
            All fixed or swept under a rug.
            If they come back one day,
            To our dismay,
            We all will feel quite un-smug.

            Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            Show
            damyon Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13