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

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

    Details

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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            quen Sam Marshall added a comment -

            Confirmed still occurs in current master.

            Show
            quen Sam Marshall added a comment - Confirmed still occurs in current master.
            Hide
            andyjdavis 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
            andyjdavis 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
            fred 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
            fred 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
            andyjdavis Andrew Davis added a comment -

            You are go for integration

            Show
            andyjdavis Andrew Davis added a comment - You are go for integration
            Hide
            poltawski 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
            poltawski 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
            fred 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
            fred 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
            poltawski Dan Poltawski added a comment -

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

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

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

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

            There are the updated patches!

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

            I accidentally reopened this issue. Sorry!

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

            Submit for integration

            Show
            andyjdavis Andrew Davis added a comment - Submit for integration
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            poltawski 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
            poltawski 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            fred 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
            fred 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
            andyjdavis 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
            andyjdavis 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 23, 22 and 21 - thanks.
            Hide
            dmonllao 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
            dmonllao 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
            samhemelryk 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
            samhemelryk 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:
                  Fix Release Date:
                  9/Jul/12