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

Course import selector lists only 10 courses, and no pagination

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.3.2
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      From a fresh install build 10 courses total.

      1. Testing with default config value of 10 courses
      2. Login as user with access to all newly created courses (e.g. Site manager)
      3. Go to one of the courses course and go to Course administration > Import
        1. Verify that form says "Total courses: 10"
      4. Built one more course and verify that form says:
        1. "More than 10 courses found, showing first 10 results"
        2. "There are too many results, enter a more specific search."
      1. Testing with config value changed (Master only)
      2. Now go to Site administration > Courses > Backups > General import defaults
      3. Change the value for "Maximum number of courses listed for import backup" to 5
      4. Go back to the course import screen and reload and verify form says:
        1. "More than 5 courses found, showing first 5 results"
        2. "There are too many results, enter a more specific search."
      5. Now go to Site administration > Courses > Backups > General import defaults
      6. Change the value for "Maximum number of courses listed for import backup" to 20
      7. Go back to the course import screen and reload and verify form says:
        1. "Total courses: 11"
      Show
      From a fresh install build 10 courses total. Testing with default config value of 10 courses Login as user with access to all newly created courses (e.g. Site manager) Go to one of the courses course and go to Course administration > Import Verify that form says "Total courses: 10" Built one more course and verify that form says: "More than 10 courses found, showing first 10 results" "There are too many results, enter a more specific search." Testing with config value changed (Master only) Now go to Site administration > Courses > Backups > General import defaults Change the value for "Maximum number of courses listed for import backup" to 5 Go back to the course import screen and reload and verify form says: "More than 5 courses found, showing first 5 results" "There are too many results, enter a more specific search." Now go to Site administration > Courses > Backups > General import defaults Change the value for "Maximum number of courses listed for import backup" to 20 Go back to the course import screen and reload and verify form says: "Total courses: 11"
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35603-import-limit

      Description

      When choosing to import material from another course, a maximum of 10 courses are presented, with no way to seek to additional courses.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mcampbell Matt Campbell added a comment -

            Reported previously as MDL-27154 and closed as a duplicate of MDL-26037, but issue still exists.

            Create at least 11 courses with some sort of similarity with the naming. Use that similarity in the search criteria. Only 10 results will be shown, with no indication that more exist or a way to view them.

            Result set of 10 is defined by $MAXRESULTS, /backup/util/ui/restore_ui_components.php, line 42.

            Need to be able to either detect additional results and offer pagination, or allow admin to define $MAXRESULTS.

            Show
            mcampbell Matt Campbell added a comment - Reported previously as MDL-27154 and closed as a duplicate of MDL-26037 , but issue still exists. Create at least 11 courses with some sort of similarity with the naming. Use that similarity in the search criteria. Only 10 results will be shown, with no indication that more exist or a way to view them. Result set of 10 is defined by $MAXRESULTS, /backup/util/ui/restore_ui_components.php, line 42. Need to be able to either detect additional results and offer pagination, or allow admin to define $MAXRESULTS.
            Hide
            rex Rex Lorenzo added a comment - - edited

            Added a branch to help fix this issue.

            • added new config option to determine length of courses returned by import
            • added text indicator if there are more than X number of courses, similar to how the restore course list currently works

            Some questions:

            • Is it okay to add a new config page for just one config option? Maybe it is better to rename the "General backup settings" to "General backup/restore settings"?
            • For the screen that list the courses to import, it shows "Total courses: X", but the course selector screen for the restore process doesn't indicate the number of courses. Maybe these two should be made consistent?

            Would appreciate a code review/feedback

            Show
            rex Rex Lorenzo added a comment - - edited Added a branch to help fix this issue. added new config option to determine length of courses returned by import added text indicator if there are more than X number of courses, similar to how the restore course list currently works Some questions: Is it okay to add a new config page for just one config option? Maybe it is better to rename the "General backup settings" to "General backup/restore settings"? For the screen that list the courses to import, it shows "Total courses: X", but the course selector screen for the restore process doesn't indicate the number of courses. Maybe these two should be made consistent? Would appreciate a code review/feedback
            Hide
            rex Rex Lorenzo added a comment -

            Eloy, can I request you as a peer reviewer? If not, please feel free to just assign it back to moodle.org.

            Show
            rex Rex Lorenzo added a comment - Eloy, can I request you as a peer reviewer? If not, please feel free to just assign it back to moodle.org.
            Hide
            tsala Helen Foster added a comment -

            Increasing priority as it seems pretty bad if you have more than 10 courses.

            Show
            tsala Helen Foster added a comment - Increasing priority as it seems pretty bad if you have more than 10 courses.
            Hide
            gb2048 Gareth J Barnard added a comment -

            I could not find anything wrong bar code checker would report lack of periods and no capital at the start of the single line comments. I don't think I'm worthy enough to peer review yet / allowed to. I've two issues that have failed peer review, so still learning the Moodle way.

            Show
            gb2048 Gareth J Barnard added a comment - I could not find anything wrong bar code checker would report lack of periods and no capital at the start of the single line comments. I don't think I'm worthy enough to peer review yet / allowed to. I've two issues that have failed peer review, so still learning the Moodle way.
            Hide
            rex Rex Lorenzo added a comment -

            Gareth, thanks for some feedback. I made my comments start with an uppercase. But I don't know what you mean by the lack of periods. I looked at the other comments for those files and they don't end their comments with periods, so I am just following the existing code style.

            I have rebased and pushed out my new version of the branch.

            Show
            rex Rex Lorenzo added a comment - Gareth, thanks for some feedback. I made my comments start with an uppercase. But I don't know what you mean by the lack of periods. I looked at the other comments for those files and they don't end their comments with periods, so I am just following the existing code style. I have rebased and pushed out my new version of the branch.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Rex,

            No problem. With the periods thing, I'm being encouraged to use code checker local plugin to tidy up code, so perhaps its capturing bygone things in an efficiency drive to clean code in general.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Rex, No problem. With the periods thing, I'm being encouraged to use code checker local plugin to tidy up code, so perhaps its capturing bygone things in an efficiency drive to clean code in general. Cheers, Gareth
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Stealing the Peer Review off Eloy as Helen posted this in the chat earlier looking for a PR.

            Show
            dobedobedoh Andrew Nicols added a comment - Stealing the Peer Review off Eloy as Helen posted this in the chat earlier looking for a PR.
            Hide
            rex Rex Lorenzo added a comment -

            Added periods at the end to conform to code checker.

            Show
            rex Rex Lorenzo added a comment - Added periods at the end to conform to code checker.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            [Y] Syntax
            [Y] Output
            [N] Whitespace
            [Y] Language
            [-] Databases
            [N] Testing
            [Y] Security
            [Y] Documentation
            [N] Git
            [Y] Sanity check

            Whitespace

            As discussed, comment should start capitalised, and end with appropriate punctuation
            Also, trailing whitespace on the empty line after $this->maxresults = ... in backup/util/ui/restore_ui_components.php

            Testing

            Could you provide some testing instructions before putting up for IR

            Git

            If you could provide the component on the first line of the commit, and change the first line of the message to summarise the change ('Add search to course import selector' or something)

            Otherwise looks awesome. Feel free to submit for Integration when ready.

            Show
            dobedobedoh Andrew Nicols added a comment - [Y] Syntax [Y] Output [N] Whitespace [Y] Language [-] Databases [N] Testing [Y] Security [Y] Documentation [N] Git [Y] Sanity check Whitespace As discussed, comment should start capitalised, and end with appropriate punctuation Also, trailing whitespace on the empty line after $this->maxresults = ... in backup/util/ui/restore_ui_components.php Testing Could you provide some testing instructions before putting up for IR Git If you could provide the component on the first line of the commit, and change the first line of the message to summarise the change ('Add search to course import selector' or something) Otherwise looks awesome. Feel free to submit for Integration when ready.
            Hide
            rex Rex Lorenzo added a comment -

            Fixed trailing white space and commit message. Added testing instructions.

            Show
            rex Rex Lorenzo added a comment - Fixed trailing white space and commit message. Added testing instructions.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry - I hadn't seen that you'd put this up for Peer Review again. Submitting for integration now.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry - I hadn't seen that you'd put this up for Peer Review again. Submitting for integration now.
            Hide
            rex Rex Lorenzo added a comment -

            As a note for the integrator, please let me know about my question regarding the config setting. I can pull out the config option and just leave it as 10 courses and a hard-coded variable.

            Show
            rex Rex Lorenzo added a comment - As a note for the integrator, please let me know about my question regarding the config setting. I can pull out the config option and just leave it as 10 courses and a hard-coded variable.
            Hide
            stronk7 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
            stronk7 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
            damyon Damyon Wiese added a comment -

            Got to head off now and didn't finish looking at this - will pick it up again tomorrow if still in list.

            Show
            damyon Damyon Wiese added a comment - Got to head off now and didn't finish looking at this - will pick it up again tomorrow if still in list.
            Hide
            damyon Damyon Wiese added a comment -

            Looking again...

            Show
            damyon Damyon Wiese added a comment - Looking again...
            Hide
            damyon Damyon Wiese added a comment -

            This is a good improvement Rex (and Matt) thanks for the patch.

            Responses to your questions:
            Is it okay to add a new config page for just one config option? Maybe it is better to rename the "General backup settings" to "General backup/restore settings"?

            I think "General backup/restore settings" is getting too long for the page name in the navigation block so a separate page is fine. I think that the page should be "General restore defaults" though as this setting affects both restore and import and import is really a special case of restore and not the other way around. The language for the setting needs to state that it applies to both restore and import.

            For the screen that list the courses to import, it shows "Total courses: X", but the course selector screen for the restore process doesn't indicate the number of courses. Maybe these two should be made consistent?

            Yes - I would like to see them consistent - if you can fix this it would be great.

            Finally - if this is to be backported it needs stable branches which should not contain the extra admin setting - just leave it hardcoded at 10 but just add the "More than X..." message for the stables.

            Thanks again, Damyon

            Show
            damyon Damyon Wiese added a comment - This is a good improvement Rex (and Matt) thanks for the patch. Responses to your questions: Is it okay to add a new config page for just one config option? Maybe it is better to rename the "General backup settings" to "General backup/restore settings"? I think "General backup/restore settings" is getting too long for the page name in the navigation block so a separate page is fine. I think that the page should be "General restore defaults" though as this setting affects both restore and import and import is really a special case of restore and not the other way around. The language for the setting needs to state that it applies to both restore and import. For the screen that list the courses to import, it shows "Total courses: X", but the course selector screen for the restore process doesn't indicate the number of courses. Maybe these two should be made consistent? Yes - I would like to see them consistent - if you can fix this it would be great. Finally - if this is to be backported it needs stable branches which should not contain the extra admin setting - just leave it hardcoded at 10 but just add the "More than X..." message for the stables. Thanks again, Damyon
            Hide
            damyon Damyon Wiese added a comment -

            Taking this issue out of integration for this week - feel free to resubmit it when you think it's ready.

            Show
            damyon Damyon Wiese added a comment - Taking this issue out of integration for this week - feel free to resubmit it when you think it's ready.
            Hide
            cibot CiBoT added a comment -

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

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

            Requesting this for integration consideration again so that I can hopefully get this patch in before the code freeze.

            Damyon, I made the changes as requested:

            1. Renamed new settings page to "General restore defaults"
            2. Added the "Total courses: X" to the restore UI as well
            3. Added patches for the 23 and 24 stable branches that don't include the new config setting and just use the existing hard-coded value and kept the new "More than X..." string
            Show
            rex Rex Lorenzo added a comment - Requesting this for integration consideration again so that I can hopefully get this patch in before the code freeze. Damyon, I made the changes as requested: Renamed new settings page to "General restore defaults" Added the "Total courses: X" to the restore UI as well Added patches for the 23 and 24 stable branches that don't include the new config setting and just use the existing hard-coded value and kept the new "More than X..." string
            Hide
            damyon Damyon Wiese 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.

            Thanks!

            Show
            damyon Damyon Wiese 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. Thanks!
            Hide
            rex Rex Lorenzo added a comment -

            Rebased!

            Show
            rex Rex Lorenzo added a comment - Rebased!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for this Rex.

            Note that you had variable names with underscore seperators, which is against Moodle coding guidelines: http://docs.moodle.org/dev/Coding_style#Variables

            I fixed this in integration in order to avoid going round another iteration. It might sound a bit picky, but I'm actually a big fan of this coding style guidance as I find it helps more easily distinguish between variable names and function names.

            In fact in this bit of code in particular there could be instances of it, e.g. between:

             
            $this->has_more_results and $this->has_more_results()
            

            Show
            poltawski Dan Poltawski added a comment - Thanks for this Rex. Note that you had variable names with underscore seperators, which is against Moodle coding guidelines: http://docs.moodle.org/dev/Coding_style#Variables I fixed this in integration in order to avoid going round another iteration. It might sound a bit picky, but I'm actually a big fan of this coding style guidance as I find it helps more easily distinguish between variable names and function names. In fact in this bit of code in particular there could be instances of it, e.g. between: $this->has_more_results and $this->has_more_results()
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Updating testing instruction to say that the new config is only in master

            Show
            ankit_frenz Ankit Agarwal added a comment - Updating testing instruction to say that the new config is only in master
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Passing test based on new test instructions.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Passing test based on new test instructions. Thanks
            Hide
            poltawski Dan Poltawski added a comment -

            Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

            line 1289 of \lib\changes.php: call to debugging()
            line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
            line 202 of \lib\now.php: call to moodleform->_is_poor_form()
            line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            Show
            poltawski Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/May/13