Moodle
  1. Moodle
  2. MDL-30316

Pagination bar does not display all options when maxdisplay is lower than 18

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: None
    • Component/s: Libraries, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. set public $maxdisplay = 18; in lib/outputcomponents.php to 5
      2. Create database activity.
      3. Add more than 5 entries
      4. Go to View single option
      5. Click on page 5 and try to access the rest of the entries without changing manually the url.

      p.s. it can be tested with any other interface with pagination that has more than 5 entries.

      Show
      set public $maxdisplay = 18; in lib/outputcomponents.php to 5 Create database activity. Add more than 5 entries Go to View single option Click on page 5 and try to access the rest of the entries without changing manually the url. p.s. it can be tested with any other interface with pagination that has more than 5 entries.
    • Workaround:
      Hide

      file lib/outputcomponents.php
      Class paging_bar
      function: prepare(renderer_base $output, moodle_page $page, $target)
      line: 2038

      Before:

      ...
                  if ($this->page > 15) {
                      $startpage = $this->page - 10;
      

      After:

      ...
                  if ($this->page > round($this->maxdisplay/3*2)) {
                      $startpage = $this->page - round($this->maxdisplay/3);
      
      Show
      file lib/outputcomponents.php Class paging_bar function: prepare(renderer_base $output, moodle_page $page, $target) line: 2038 Before: ... if ($ this ->page > 15) { $startpage = $ this ->page - 10; After: ... if ($ this ->page > round($ this ->maxdisplay/3*2)) { $startpage = $ this ->page - round($ this ->maxdisplay/3);
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30316-master
    • Rank:
      32665

      Description

      I changed the maxdisplay to 5 because the section I have to show the pagination is not big enough but I found a problem. The values to calculate which pages to display are hardcoded instead of being relative, for example:

      When viewing an interface with pagination, it only shows the first 5 pages and the user can't access the rest.
      I created a database activity with more than 5 entries and when I click on "View single" and navigate to page number 5, I can't access the rest unless I change the URL manually but then I wouldn't see the current page.

      I think this is something very specific, but I think the suggested solution can help everyone in a similar situation.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for reporting that and providing a solution.
          Hide
          Jason Fowler added a comment -

          Martha, I have tried to reproduce this without creating theme renderers.php, but can't seem to do it, can you please provide more details on how this can be tested?

          Show
          Jason Fowler added a comment - Martha, I have tried to reproduce this without creating theme renderers.php, but can't seem to do it, can you please provide more details on how this can be tested?
          Hide
          Adrian Greeve added a comment -

          The code is fine, but this isn't a bug.

          The maximum number of pages that we have in the pagination bar is 18.
          If the current page is higher than 15 then it alters the start of the pagination bar to display something like this: [1....8 9 10

          I see no way that this could have caused the error that is in the attached image.

          I guess not having the figures hard coded is a good idea, just in case for some weird reason we decided to change the number of pages at the top of a page.

          Show
          Adrian Greeve added a comment - The code is fine, but this isn't a bug. The maximum number of pages that we have in the pagination bar is 18. If the current page is higher than 15 then it alters the start of the pagination bar to display something like this: [1....8 9 10 I see no way that this could have caused the error that is in the attached image. I guess not having the figures hard coded is a good idea, just in case for some weird reason we decided to change the number of pages at the top of a page.
          Hide
          Jason Fowler added a comment -

          No this isn't a bug, its an improvement I forgot to change that, will do it as soon as I get in to work tomorrow.

          This code change is to allow greater flexibility for theme designers is all.

          Show
          Jason Fowler added a comment - No this isn't a bug, its an improvement I forgot to change that, will do it as soon as I get in to work tomorrow. This code change is to allow greater flexibility for theme designers is all.
          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
          Jason Fowler added a comment -

          rebased

          Show
          Jason Fowler added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Jason Fowler added a comment -

          Rebased, again

          Show
          Jason Fowler added a comment - Rebased, again
          Hide
          Aparup Banerjee added a comment -

          Hi Jason,
          sorry it took awhile to get to reviewing this, here's a few notes:

          • the test here only tests for what is being fixed, i think it would be wise to add in some regression test to the test instructions here too. oh and even better..
          • have you considered adding to lib/tests/outputcomponents_test.php, perhaps a test for variations of pagination bar output? i think pagination could do with some test in here.
          • where git commits are from contributed code, we should celebrate it in the commit message

          so mainly reopening here for unit testing additions here.

          Show
          Aparup Banerjee added a comment - Hi Jason, sorry it took awhile to get to reviewing this, here's a few notes: the test here only tests for what is being fixed, i think it would be wise to add in some regression test to the test instructions here too. oh and even better.. have you considered adding to lib/tests/outputcomponents_test.php, perhaps a test for variations of pagination bar output? i think pagination could do with some test in here. where git commits are from contributed code, we should celebrate it in the commit message so mainly reopening here for unit testing additions here.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Hey Aparup, I was talking this over with Ankit, and he raised the point that for this to have a unit test, the part of the code I modified would have to be put into a function on its own, and then called by the unit test. The consensus in the scrum this morning was that this isn't necessary. Does this really need a unit test seeing as it's a hard-coded calculation?

          Show
          Jason Fowler added a comment - Hey Aparup, I was talking this over with Ankit, and he raised the point that for this to have a unit test, the part of the code I modified would have to be put into a function on its own, and then called by the unit test. The consensus in the scrum this morning was that this isn't necessary. Does this really need a unit test seeing as it's a hard-coded calculation?
          Hide
          Aparup Banerjee added a comment -

          Hi Jason,

          • i was thinking of this unit test as a way to lock in this output from this method/class, it would be useful to prevent future regression when some one comes along and tries to develop this area next time. I'm not sure why the part of the code would have to be extracted to have it tested. it is the one class and if you pass in the right constructors (.. a contructed $page , $perpage = something > round($this->maxdisplay/3*2), ... ) you can trigger the code area and validate the output by checking say the class of $this->firstlink (assuming its changed - test it too)?

          perhaps i'm not seeing the problem so i'll discuss this further to get my head straight with integrators

          • i also realised that $currpage and $startpage are redundant variables of each other within paging_bar::prepare()

          ps: still need to assign credit in commit message.

          Show
          Aparup Banerjee added a comment - Hi Jason, i was thinking of this unit test as a way to lock in this output from this method/class, it would be useful to prevent future regression when some one comes along and tries to develop this area next time. I'm not sure why the part of the code would have to be extracted to have it tested. it is the one class and if you pass in the right constructors (.. a contructed $page , $perpage = something > round($this->maxdisplay/3*2), ... ) you can trigger the code area and validate the output by checking say the class of $this->firstlink (assuming its changed - test it too)? perhaps i'm not seeing the problem so i'll discuss this further to get my head straight with integrators i also realised that $currpage and $startpage are redundant variables of each other within paging_bar::prepare() ps: still need to assign credit in commit message.
          Hide
          Jason Fowler added a comment -

          Created a separate issue for the creation of the unit test. Will get to work on that in the next sprint.

          Show
          Jason Fowler added a comment - Created a separate issue for the creation of the unit test. Will get to work on that in the next sprint.
          Hide
          Jason Fowler added a comment -

          I've also updated the commit message to reflect Martha's contribution of the original code.

          Show
          Jason Fowler added a comment - I've also updated the commit message to reflect Martha's contribution of the original code.
          Hide
          Jason Fowler added a comment -

          fixed the redundant variable and pushed

          Show
          Jason Fowler added a comment - fixed the redundant variable and pushed
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Hi,

          I don't understand why this is round($this->maxdisplay/3*2)? Particularly I don't understand what the 3 and 2 are for? Is there some parenthesis missing?

          In any case, if I don't understand it I can't integrate it - could you clarify the code?

          Show
          Dan Poltawski added a comment - Hi, I don't understand why this is round($this->maxdisplay/3*2)? Particularly I don't understand what the 3 and 2 are for? Is there some parenthesis missing? In any case, if I don't understand it I can't integrate it - could you clarify the code?
          Hide
          Jason Fowler added a comment -

          Will see what I can do to improve that.

          Show
          Jason Fowler added a comment - Will see what I can do to improve that.
          Hide
          Jason Fowler added a comment -

          Ah, the reason is to make it easier to understand what is actually happening with the math....

          if ($this->page > round($this->maxdisplay/3*2)) {
          $startpage = $this->page - round($this->maxdisplay/3);
          

          The round($this->maxdisplay/3*2) is to show that it is double the round($this->maxdisplay/3) used in the line below.

          Show
          Jason Fowler added a comment - Ah, the reason is to make it easier to understand what is actually happening with the math.... if ($this->page > round($this->maxdisplay/3*2)) { $startpage = $this->page - round($this->maxdisplay/3); The round($this->maxdisplay/3*2) is to show that it is double the round($this->maxdisplay/3) used in the line below.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          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
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23 (as its a bug).

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23 (as its a bug).
          Hide
          Andrew Davis added a comment -

          Seems to be working as described. Passing.

          Show
          Andrew Davis added a comment - Seems to be working as described. Passing.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!
          Hide
          Ankit Agarwal added a comment -

          Is fix versions correct for this issue?

          Show
          Ankit Agarwal added a comment - Is fix versions correct for this issue?
          Hide
          Jason Fowler added a comment -

          Fix version was master only

          Show
          Jason Fowler added a comment - Fix version was master only

            People

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

              Dates

              • Created:
                Updated:
                Resolved: