Course import selector lists only 10 courses, and no pagination

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.

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

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

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

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.

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.

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

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

Added periods at the end to conform to code checker.

[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.

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

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

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.

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

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

Looking again...

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

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

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

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

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!

Rebased!

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()

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

Passing test based on new test instructions.
Thanks

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()