Moodle
  1. Moodle
  2. MDL-31647

Glossary 'entry list' format: entries load in popups, cause broken links

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Glossary
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a main glossary on a course, default settings. Add one entry with concept/name 'test' set to auto-link.
      2. Create a secondary glossary on a course, default settings except that the format should be 'Entry list'. Add an entry with any name and a value which includes the word 'test'.
      3. Go to the secondary glossary's main page. Click on the entry.

      EXPECTED BEHAVIOUR: Entry correctly opens in the current browser window.

      ACTUAL BEHAVIOUR: The entry unexpectedly opens in a new browser window. (This is terrible usability. You're in the glossary itself, things shouldn't be popping up all over.) You get links to the glossary entry which are normal links, but there's an onclick which takes you to the same link with popup=1 in a new window.

      4. In the popup window which shouldn't have opened, there is a glossary underline (from the main glossary) on the word 'test'. Click this word.

      EXPECTED BEHAVIOUR: Glossary popup for 'test' from main glossary appears (within the same browser window).

      ACTUAL BEHAVIOUR: Spinning cursor effect appears, but nothing happens.

      Show
      1. Create a main glossary on a course, default settings. Add one entry with concept/name 'test' set to auto-link. 2. Create a secondary glossary on a course, default settings except that the format should be 'Entry list'. Add an entry with any name and a value which includes the word 'test'. 3. Go to the secondary glossary's main page. Click on the entry. EXPECTED BEHAVIOUR: Entry correctly opens in the current browser window. ACTUAL BEHAVIOUR: The entry unexpectedly opens in a new browser window. (This is terrible usability. You're in the glossary itself, things shouldn't be popping up all over.) You get links to the glossary entry which are normal links, but there's an onclick which takes you to the same link with popup=1 in a new window. 4. In the popup window which shouldn't have opened, there is a glossary underline (from the main glossary) on the word 'test'. Click this word. EXPECTED BEHAVIOUR: Glossary popup for 'test' from main glossary appears (within the same browser window). ACTUAL BEHAVIOUR: Spinning cursor effect appears, but nothing happens.
    • Workaround:
      Hide

      Change to a different glossary format other than 'entry list'.

      Show
      Change to a different glossary format other than 'entry list'.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31647-master
    • Rank:
      38218

      Description

      Links to a different glossary inside glossary window popup entries (not the current in-page style popup, the old separate-browser-window popup) fail.

      If you click on such a link, you get a spinny cursor and nothing happens. See testing instructions.

      Glossary window popup entries are only used from the 'entry list' glossary format.

      I think the best way to solve this problem would be to ensure that glossary entries do not open in a separate-browser-window popup in the 'entry list' glossary format. If you remove the popup=1 from the URL and have it load as a normal page instead of in a new window, it works fine, and the glossary links inside that window operate correctly.

        Activity

        Hide
        Sam Marshall added a comment -

        Requesting peer review for this.

        Note: I removed the 'popup' parameter from showentry altogether as I don't think it is now used any more (looks like this functionality was replaced by the JavaScript popups, except somebody forgot about entrylist format). I searched mod/glossary and filter/glossary for 'popup'; there are plenty of results but I don't think any of them are putting it into a showentry.php url now.

        Show
        Sam Marshall added a comment - Requesting peer review for this. Note: I removed the 'popup' parameter from showentry altogether as I don't think it is now used any more (looks like this functionality was replaced by the JavaScript popups, except somebody forgot about entrylist format). I searched mod/glossary and filter/glossary for 'popup'; there are plenty of results but I don't think any of them are putting it into a showentry.php url now.
        Hide
        Dan Poltawski added a comment -

        Makes complete sense to me Sam! Please submit for integration!

        Show
        Dan Poltawski added a comment - Makes complete sense to me Sam! Please submit for integration!
        Hide
        Michael de Raadt added a comment -

        I've just triaged this, but don't let that stop you from continuing with this.

        Show
        Michael de Raadt added a comment - I've just triaged this, but don't let that stop you from continuing with this.
        Hide
        Sam Marshall added a comment -

        Thanks Dan Submitting for integration

        Show
        Sam Marshall added a comment - Thanks Dan Submitting for integration
        Hide
        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
        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
        Sam Marshall added a comment -

        Rebased (all branches)

        Show
        Sam Marshall added a comment - Rebased (all branches)
        Hide
        Sam Hemelryk added a comment -

        Thanks Sam, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Sam, this has been integrated now
        Hide
        Rossiani Wijaya added a comment -

        This is working great.

        Thanks for fixing.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working great. Thanks for fixing. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some changes to Moodle should be milestones in the project by themselves.

        This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: