Moodle
  1. Moodle
  2. MDL-9073

Glossary printer-friendly view does not work for uncategorised entry view

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8, 2.3
    • Fix Version/s: 2.1.7, 2.2.4, 2.3.1
    • Component/s: Glossary
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Set up new glossary.
      2. From the main page of the new glossary, click on the tab 'Browse by category', click on 'Edit categories'.
      3. Add a new category called 'My Category'.
      4. Now, add a new entry called 'My categorised concept' and select the category 'My Category'.
      5. Add another entry called 'Concept without category' and select the category 'Not categorised'.
      6. Click on 'Browse by category' tab. Make sure the only entry is 'My categorised concept'.
      7. Click on the print icon and make sure only 'My categorised concept' appears.
      8. Back on the previous screen, select 'Not categorised' from the dropdown menu. Make sure the only entry is 'Concept without category'.
      9. Click on the print icon and make sure only 'Concept without category' appears.
      10. Back on the previous screen, select 'My Category' from the dropdown menu. Make sure the only entry is 'My categorised concept'.
      11. Click on the print icon and make sure only 'My categorised concept' appears.

      Show
      1. Set up new glossary. 2. From the main page of the new glossary, click on the tab 'Browse by category', click on 'Edit categories'. 3. Add a new category called 'My Category'. 4. Now, add a new entry called 'My categorised concept' and select the category 'My Category'. 5. Add another entry called 'Concept without category' and select the category 'Not categorised'. 6. Click on 'Browse by category' tab. Make sure the only entry is 'My categorised concept'. 7. Click on the print icon and make sure only 'My categorised concept' appears. 8. Back on the previous screen, select 'Not categorised' from the dropdown menu. Make sure the only entry is 'Concept without category'. 9. Click on the print icon and make sure only 'Concept without category' appears. 10. Back on the previous screen, select 'My Category' from the dropdown menu. Make sure the only entry is 'My categorised concept'. 11. Click on the print icon and make sure only 'My categorised concept' appears.
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
      git@github.com:FMCorz/moodle.git
    • Pull Master Branch:
      MDL-9073-master
    • Rank:
      3533

      Description

      (Reported by one of our testers.)

      To reproduce:

      1. Set up new glossary.
      2. Add new uncategorised entry.
      3. Click on 'Browse by category' tab. Initially, no entries are shown.
      4. Click on 'Not categorised'. Entry appears.
      5. Click on the 'Printable version' icon at top by header.

      The printable version does not show any entries even though one is shown on the screen entry.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          lqjjLKA0p6

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
          Hide
          Sam Marshall added a comment -

          Confirmed still occurs in current master.

          Show
          Sam Marshall added a comment - Confirmed still occurs in current master.
          Hide
          Andrew Davis added a comment -

          Code change looks fine. Could you add a brief description of what the problem actually was and why this change fixes it.

          This looks like it will require versions for 2.2 and 2.1.

          It also needs testing instructions.

          Show
          Andrew Davis added a comment - Code change looks fine. Could you add a brief description of what the problem actually was and why this change fixes it. This looks like it will require versions for 2.2 and 2.1. It also needs testing instructions.
          Hide
          Frédéric Massart added a comment -

          What happened is that the uncategorised entry is identified by '-1', but the dash was filtered by optional_param(), and therefore the category was incorrect. I used the same cleaning parameter than on the other pages to fix this.

          I have created the branches for 2.1 and 2.2 and wrote the test instructions. I should have done that in the first place.

          Thanks!

          Show
          Frédéric Massart added a comment - What happened is that the uncategorised entry is identified by '-1', but the dash was filtered by optional_param(), and therefore the category was incorrect. I used the same cleaning parameter than on the other pages to fix this. I have created the branches for 2.1 and 2.2 and wrote the test instructions. I should have done that in the first place. Thanks!
          Hide
          Andrew Davis added a comment -

          You are go for integration

          Show
          Andrew Davis added a comment - You are go for integration
          Hide
          Dan Poltawski added a comment -

          Hi Fredm

          In general we shouldn't be converting to use PARAM_CLEAN as its marked deprecated and does a generic cleaning and probably isn't that safe.

          I had a look around and I can't for the life of me understand what the hook parameter it supposed to be, and in fact its not consistent:

          approve.php:$hook = optional_param('hook', 'ALL', PARAM_CLEAN);
          deleteentry.php:$hook     = optional_param('hook', '', PARAM_CLEAN);
          editcategories.php:$hook   = optional_param('hook', '', PARAM_ALPHANUM); // category ID
          export.php:$hook= optional_param('hook', '', PARAM_CLEAN);           // the term, entry, cat, etc... to look for based on mode
          exportentry.php:$hook     = optional_param('hook', '', PARAM_CLEAN);
          import.php:$hook     = optional_param('hook', 'ALL', PARAM_ALPHANUM);
          lib.php:    $hook = optional_param('hook', 'ALL', PARAM_CLEAN);
          print.php:$hook    = optional_param('hook','ALL', PARAM_ALPHANUM);   // what to show
          view.php:$hook       = optional_param('hook', '', PARAM_CLEAN);           // the term, entry, cat, etc... to look for based on mode
          view.php:$show       = optional_param('show', '', PARAM_ALPHA);           // [ concept | alias ] => mode=term hook=$show
          

          So, i'm not really keen on this solution. Should it in fact be PARAM_INT? Or what other values can hook be?

          Show
          Dan Poltawski added a comment - Hi Fredm In general we shouldn't be converting to use PARAM_CLEAN as its marked deprecated and does a generic cleaning and probably isn't that safe. I had a look around and I can't for the life of me understand what the hook parameter it supposed to be, and in fact its not consistent: approve.php:$hook = optional_param('hook', 'ALL', PARAM_CLEAN); deleteentry.php:$hook = optional_param('hook', '', PARAM_CLEAN); editcategories.php:$hook = optional_param('hook', '', PARAM_ALPHANUM); // category ID export.php:$hook= optional_param('hook', '', PARAM_CLEAN); // the term, entry, cat, etc... to look for based on mode exportentry.php:$hook = optional_param('hook', '', PARAM_CLEAN); import .php:$hook = optional_param('hook', 'ALL', PARAM_ALPHANUM); lib.php: $hook = optional_param('hook', 'ALL', PARAM_CLEAN); print.php:$hook = optional_param('hook','ALL', PARAM_ALPHANUM); // what to show view.php:$hook = optional_param('hook', '', PARAM_CLEAN); // the term, entry, cat, etc... to look for based on mode view.php:$show = optional_param('show', '', PARAM_ALPHA); // [ concept | alias ] => mode=term hook=$show So, i'm not really keen on this solution. Should it in fact be PARAM_INT? Or what other values can hook be?
          Hide
          Frédéric Massart added a comment -

          Well, PARAM_INT would be the best idea except that hook can contain letters too, like 'all'. This parameter contains different type of data from one place to another in the glossary. I did not pay attention to the fact that PARAM_CLEAN was deprecated, but having a look at clean_param() now, I guess PARAM_ALPHANUMEXT would be good.

          Show
          Frédéric Massart added a comment - Well, PARAM_INT would be the best idea except that hook can contain letters too, like 'all'. This parameter contains different type of data from one place to another in the glossary. I did not pay attention to the fact that PARAM_CLEAN was deprecated, but having a look at clean_param() now, I guess PARAM_ALPHANUMEXT would be good.
          Hide
          Dan Poltawski added a comment -

          OK, well i'm going to reopen this if you could took at that, thanks

          Show
          Dan Poltawski added a comment - OK, well i'm going to reopen this if you could took at that, thanks
          Hide
          Dan Poltawski added a comment -

          (Note the real solution should be for that crazy parameter to use individual params for different things, but..)

          Show
          Dan Poltawski added a comment - (Note the real solution should be for that crazy parameter to use individual params for different things, but..)
          Hide
          Frédéric Massart added a comment -

          There are the updated patches!

          Show
          Frédéric Massart added a comment - There are the updated patches!
          Hide
          Frédéric Massart added a comment -

          I accidentally reopened this issue. Sorry!

          Show
          Frédéric Massart added a comment - I accidentally reopened this issue. Sorry!
          Hide
          Andrew Davis added a comment -

          Submit for integration

          Show
          Andrew Davis added a comment - Submit for integration
          Hide
          Petr Škoda added a comment - - edited

          The hook is first letter of the concept (unicode char), or a hardcoded constant - that means both current code (PARAM_ALPHANUM) and the patch (PARAM_ALPHANUMEXT) are not correct. There is no simple way to clean it, it might be best to whitelist those special words and use s() and p() for page output.

          Show
          Petr Škoda added a comment - - edited The hook is first letter of the concept (unicode char), or a hardcoded constant - that means both current code (PARAM_ALPHANUM) and the patch (PARAM_ALPHANUMEXT) are not correct. There is no simple way to clean it, it might be best to whitelist those special words and use s() and p() for page output.
          Hide
          Dan Poltawski added a comment -

          Petr. How about we go one step closer to fixing this problem by accepting this incremental patch, then create a new issue for the utf8 problem?

          (that sucks, but it already sucks anyway so is at least an improvement).

          Show
          Dan Poltawski added a comment - Petr. How about we go one step closer to fixing this problem by accepting this incremental patch, then create a new issue for the utf8 problem? (that sucks, but it already sucks anyway so is at least an improvement).
          Hide
          Petr Škoda added a comment -

          hmm, if you want a hacky patch then use PARAM_CLEAN, it is already used in other glossary scripts, the PARAM_ALPHA* is wrong

          Show
          Petr Škoda added a comment - hmm, if you want a hacky patch then use PARAM_CLEAN, it is already used in other glossary scripts, the PARAM_ALPHA* is wrong
          Hide
          Frédéric Massart added a comment -

          There I have updated the branchs with the hack. I guess we should create a new issue to have a correct sanitization of the params.

          Show
          Frédéric Massart added a comment - There I have updated the branchs with the hack. I guess we should create a new issue to have a correct sanitization of the params.
          Hide
          Andrew Davis added a comment -

          Raise a new MDL for the additional work and link it to this one. Submit for integration when you're ready.

          Show
          Andrew Davis added a comment - Raise a new MDL for the additional work and link it to this one. Submit for integration when you're ready.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 23, 22 and 21 - thanks.

          Show
          Dan Poltawski added a comment - Integrated to master, 23, 22 and 21 - thanks.
          Hide
          David Monllaó added a comment -

          Tested on integration 2.1, 2.2 and 2.3 and passes in all the branches. Thanks Fred for the detailed steps to test

          PD: I pick that one cause I've begun testing before the SCRUM (it's the first one ordering by KEY)

          Show
          David Monllaó added a comment - Tested on integration 2.1, 2.2 and 2.3 and passes in all the branches. Thanks Fred for the detailed steps to test PD: I pick that one cause I've begun testing before the SCRUM (it's the first one ordering by KEY)
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: