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
    • Rank:
      39279

      Description

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

        Activity

        Hide
        Dongsheng Cai added a comment -

        Looks good, thanks!

        Show
        Dongsheng Cai added a comment - Looks good, thanks!
        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
        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
        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
        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
        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
        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
        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 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 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
        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
        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 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 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 Glancy added a comment -

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

        Show
        Marina Glancy added a comment - Eloy, simple cherry-picking to 2.1 and 2.2 did not work. I included links to github
        Hide
        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
        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
        Rossiani Wijaya added a comment -

        This is working great.

        Thanks for fixing.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working great. Thanks for fixing. Test passed.
        Hide
        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
        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: