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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Activity

          Hide
          quen 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
          quen 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
          poltawski Dan Poltawski added a comment -

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

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

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

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

          Thanks Dan Submitting for integration

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

          Rebased (all branches)

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

          Thanks Sam, this has been integrated now

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

          This is working great.

          Thanks for fixing.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This is working great. Thanks for fixing. Test passed.
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                12/Mar/12