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

          Attachments

            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