Moodle
  1. Moodle
  2. MDL-35603

Course import selector lists only 10 courses, and no pagination

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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 2.4 Branch:
    • Pull Master Branch:
      MDL-35603-import-limit
    • Rank:
      44327

      Description

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

        Issue Links

          Activity

          Hide
          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
          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 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 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 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 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
          Helen Foster added a comment -

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

          Show
          Helen Foster added a comment - Increasing priority as it seems pretty bad if you have more than 10 courses.
          Hide
          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
          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 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 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
          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
          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
          Andrew Nicols added a comment -

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

          Show
          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 Lorenzo added a comment -

          Added periods at the end to conform to code checker.

          Show
          Rex Lorenzo added a comment - Added periods at the end to conform to code checker.
          Hide
          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
          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 Lorenzo added a comment -

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

          Show
          Rex Lorenzo added a comment - Fixed trailing white space and commit message. Added testing instructions.
          Hide
          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
          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 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 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
          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
          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 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 Wiese added a comment -

          Looking again...

          Show
          Damyon Wiese added a comment - Looking again...
          Hide
          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 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 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 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 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
          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 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 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 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 Lorenzo added a comment -

          Rebased!

          Show
          Rex Lorenzo added a comment - Rebased!
          Hide
          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
          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 Agarwal added a comment -

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

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

          Passing test based on new test instructions.
          Thanks

          Show
          Ankit Agarwal added a comment - Passing test based on new test instructions. Thanks
          Hide
          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
          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: