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

Pagination does not work in Flickr repository in search results

    Details

    • Testing Instructions:
      Hide

      Note: This must be tested under 21_STABLE, 22_STABLE and master.

      1. Enable Flickr repository (not Flickr public!)
      2. Add more than 24 images to your Flickr
      3. Access Flickr repository from Filepicker and try search that return more than 24 images (i.e. empty string), make sure you can navigate to second page

      Show
      Note: This must be tested under 21_STABLE, 22_STABLE and master. 1. Enable Flickr repository (not Flickr public!) 2. Add more than 24 images to your Flickr 3. Access Flickr repository from Filepicker and try search that return more than 24 images (i.e. empty string), make sure you can navigate to second page
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-32417-master

      Description

      When there are more than 24 images returned in search, navigation to second page does nothing

        Gliffy Diagrams

          Activity

          Hide
          dongsheng Dongsheng Cai added a comment -

          Looks good, thanks!

          Show
          dongsheng Dongsheng Cai added a comment - Looks good, thanks!
          Hide
          poltawski 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
          poltawski 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
          samhemelryk Sam Hemelryk added a comment -

          Hi Marina, this is a bug right, does it need to be backported to 21, 22?
          If so and it can be done without conflict then I can do it during integration, otherwise can you please produce branches for the required versions for me.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Marina, this is a bug right, does it need to be backported to 21, 22? If so and it can be done without conflict then I can do it during integration, otherwise can you please produce branches for the required versions for me. Cheers Sam
          Hide
          stronk7 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
          stronk7 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
          poltawski 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
          poltawski 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I don't get the "// TODO: This breaks pagination" comment, when the patch is exactly fixing pagination... but here we go!

          Integrated (21, 22 & master, applied clean in all them), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I don't get the "// TODO: This breaks pagination" comment, when the patch is exactly fixing pagination... but here we go! Integrated (21, 22 & master, applied clean in all them), thanks!
          Hide
          marina Marina Glancy added a comment -

          Eloy, regarding this comment. When we apply filetype filter AFTER the result has been retrieved and broken into pages, it may happen that some pages will be smaller than others. Actually some pages may contain no files at all. With lazy loading of the next page in the new filepicker it does not matter very much. But in non-js filepicker when you have to click on page number to go to a particular page it may confuse.

          Show
          marina Marina Glancy added a comment - Eloy, regarding this comment. When we apply filetype filter AFTER the result has been retrieved and broken into pages, it may happen that some pages will be smaller than others. Actually some pages may contain no files at all. With lazy loading of the next page in the new filepicker it does not matter very much. But in non-js filepicker when you have to click on page number to go to a particular page it may confuse.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Marina,

          I'm failing this issue because of regression issue for 2.2 and 2.1. This is the reference issue for $page param in 2.3 (MDL-32095)

          Notice:  Undefined variable: page in /moodle/repository/flickr/lib.php on line 206

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Marina, I'm failing this issue because of regression issue for 2.2 and 2.1. This is the reference issue for $page param in 2.3 ( MDL-32095 ) Notice: Undefined variable: page in /moodle/repository/flickr/lib.php on line 206
          Hide
          marina Marina Glancy added a comment -

          I did not fix it for 2.1 and 2.2 actually. Eloy cherry-picked changes. I'll take a look

          Show
          marina Marina Glancy added a comment - I did not fix it for 2.1 and 2.2 actually. Eloy cherry-picked changes. I'll take a look
          Hide
          marina Marina Glancy added a comment -

          Eloy, simple cherry-picking to 2.1 and 2.2 did not work. I included links to github

          Show
          marina Marina Glancy added a comment - Eloy, simple cherry-picking to 2.1 and 2.2 did not work. I included links to github
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Aha, while cherry-picking I compared the status... but I missed completely that param, apologizes.

          Oki, I've cherry-picked your commits for 21 and 22 on top of the integration.git branches (1-line commits) and amended the message to realize it's the fix for the previous integration.

          Reseting this back to "testable" status, thanks! Ciao

          PS: The commits added are:

          22_STABLE: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=47a6c7ee313b8d44e1d4b1c2b355351c30d7b1ab

          21_STABLE: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=607b4fa3e2e88294e9c823550616235f03e5f7a4

          Edited: to fix commit hashes, grrr

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Aha, while cherry-picking I compared the status... but I missed completely that param, apologizes. Oki, I've cherry-picked your commits for 21 and 22 on top of the integration.git branches (1-line commits) and amended the message to realize it's the fix for the previous integration. Reseting this back to "testable" status, thanks! Ciao PS: The commits added are: 22_STABLE: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=47a6c7ee313b8d44e1d4b1c2b355351c30d7b1ab 21_STABLE: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=607b4fa3e2e88294e9c823550616235f03e5f7a4 Edited: to fix commit hashes, grrr
          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 -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/12