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

error in frompage with format FRONTPAGECOURSELIST when ((frontpagecourselimit > count(courses)) and user is admin

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.6, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.7, 1.7.1, 1.7.2, 1.8, 1.8.1, 1.9.13, 2.0, 2.1, 2.2, 2.3, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Labels:
    • Environment:
      all OS
      all platform
    • Testing Instructions:
      Hide
      1. Login as site admin
      2. Ensure that Frontpage setting is set to Frontpage Course List (Site admin -> Front page -> Front page settings -> frontpageloggedin
      3. Ensure that there are more courses than FRONTPAGECOURSELIMIT constant (defined in course/lib.php, if you don't want to create 201 moodle courses edit the hardcoded value)
      4. Login in as an admin
      5. Go to the frontpage, you SHOULD observe that the site admin also can search for a course at the bottom of the page
      Show
      Login as site admin Ensure that Frontpage setting is set to Frontpage Course List (Site admin -> Front page -> Front page settings -> frontpageloggedin Ensure that there are more courses than FRONTPAGECOURSELIMIT constant (defined in course/lib.php, if you don't want to create 201 moodle courses edit the hardcoded value) Login in as an admin Go to the frontpage, you SHOULD observe that the site admin also can search for a course at the bottom of the page
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-10259_master

      Description

      If admin user is loggin in site, and frompage format is 'FRONTPAGECOURSELIST' , when frontpagecourselimit > count(courses_site) ==> don't display categories, courses,...

      Files affected: /index.php

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            aborrow Anthony Borrow added a comment -

            Adding this to stable backlog and updating to latest versions as this issue came up for me on a production site. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Adding this to stable backlog and updating to latest versions as this issue came up for me on a production site. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment -

            The current logic is a bit defective as it leaves the site administrator with no courses and no search box. The solution that I am proposing, which is different than the existing patch (and for which I will provide a patch) changes the behavior so that if a site admin (someone with moodle/site:config) is loooking at the page and there are too many courses (the number of courses exceeds the number that can be listed on the front page (FRONTPAGECOURSELIMIT), then it will use print_my_moodle to show the courses that the site administrator is enrolled in. print_my_moodle is already setup such that it will show the courses and if there are none it will then show the categories. The other change I made, was to add the search box if it shows the categories so that the site admin consistently has the ability to search for a course on the site without too much effort. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - The current logic is a bit defective as it leaves the site administrator with no courses and no search box. The solution that I am proposing, which is different than the existing patch (and for which I will provide a patch) changes the behavior so that if a site admin (someone with moodle/site:config) is loooking at the page and there are too many courses (the number of courses exceeds the number that can be listed on the front page (FRONTPAGECOURSELIMIT), then it will use print_my_moodle to show the courses that the site administrator is enrolled in. print_my_moodle is already setup such that it will show the courses and if there are none it will then show the categories. The other change I made, was to add the search box if it shows the categories so that the site admin consistently has the ability to search for a course on the site without too much effort. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment -

            Here is what I have implemented for one production site:

            https://github.com/arborrow/moodle/compare/MOODLE_21_STABLE...MDL-10259

            Show
            aborrow Anthony Borrow added a comment - Here is what I have implemented for one production site: https://github.com/arborrow/moodle/compare/MOODLE_21_STABLE...MDL-10259
            Hide
            aborrow Anthony Borrow added a comment -

            Michael - I'm adding the more recent branches as this still appears to be an issue. Also adding the link to my patch which I worked up for 2.1. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Michael - I'm adding the more recent branches as this still appears to be an issue. Also adding the link to my patch which I worked up for 2.1. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment -

            oops, I mixed up the affected and fix versions

            Show
            aborrow Anthony Borrow added a comment - oops, I mixed up the affected and fix versions
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Anthony, pull branches updated on top of your patch.

            Regards, David

            Show
            dmonllao David Monllaó added a comment - Thanks Anthony, pull branches updated on top of your patch. Regards, David
            Hide
            fred Frédéric Massart added a comment -

            Thanks for your work on this Rafa and David. From what I have seen, there are bits of inconsistent things which don't come from your patch but could perhaps be fixed in this issue.

            Regarding the patch itself, the 3rd if statement that you are adding is an exact copy of the first one. I believe there could be a way of not duplicating this code by altering a bit the conditions.

            My +1 for the search box when the user is displayed the whole list of courses.

            And here is the list of behaviour that I found a bit strange:

            • Logged in as a user, if not enrolled in any courses, all the courses are displayed but entitled "My Courses" which is not true. It should be "Available courses" as for logged out users.
            • Not logged in, you see the whole list of available courses on the front page, but logged in as guest you don't see any at all, if the FRONTPAGECOURSELIMIT is exceeded.
            • The FRONTPAGECOURSELIMIT does not seem to be used when viewing the front page as a logged out user.

            Again, I don't know if should be fixed in the scope of this issue, but well... food for thoughts.

            Thanks guys

            Show
            fred Frédéric Massart added a comment - Thanks for your work on this Rafa and David. From what I have seen, there are bits of inconsistent things which don't come from your patch but could perhaps be fixed in this issue. Regarding the patch itself, the 3rd if statement that you are adding is an exact copy of the first one. I believe there could be a way of not duplicating this code by altering a bit the conditions. My +1 for the search box when the user is displayed the whole list of courses. And here is the list of behaviour that I found a bit strange: Logged in as a user, if not enrolled in any courses, all the courses are displayed but entitled "My Courses" which is not true. It should be "Available courses" as for logged out users. Not logged in, you see the whole list of available courses on the front page, but logged in as guest you don't see any at all, if the FRONTPAGECOURSELIMIT is exceeded. The FRONTPAGECOURSELIMIT does not seem to be used when viewing the front page as a logged out user. Again, I don't know if should be fixed in the scope of this issue, but well... food for thoughts. Thanks guys
            Hide
            dmonllao David Monllaó added a comment -

            Hi Frédéric, I'm experiencing the same inconsistencies, this issue will solve the problem logged in as an admin, but IMO the guest users and users without enrolments inconsitencies should be addressed in another issue since it also have to be consistent with the way course/category.php works when FRONTPAGECOURSELIMIT or CFG->coursesperpage is exceeded.

            About the patch comment, I added the new statement to make the code more readable, but it's true that seems ugly, I've joined the 1st and the 3rd one and add a comment about the behaviour.

            Show
            dmonllao David Monllaó added a comment - Hi Frédéric, I'm experiencing the same inconsistencies, this issue will solve the problem logged in as an admin, but IMO the guest users and users without enrolments inconsitencies should be addressed in another issue since it also have to be consistent with the way course/category.php works when FRONTPAGECOURSELIMIT or CFG->coursesperpage is exceeded. About the patch comment, I added the new statement to make the code more readable, but it's true that seems ugly, I've joined the 1st and the 3rd one and add a comment about the behaviour.
            Hide
            fred Frédéric Massart added a comment -

            Hi David. As you suggest to fix the inconsistencies in another issue, could you please raise one and link it with this issue?
            You're right, it does not look nice and it is quite hard to read, but I guess that's the best way of doing it. Minor thing that I noticed, your comment should start with a capital letter and end with a period.
            Thanks !

            Show
            fred Frédéric Massart added a comment - Hi David. As you suggest to fix the inconsistencies in another issue, could you please raise one and link it with this issue? You're right, it does not look nice and it is quite hard to read, but I guess that's the best way of doing it. Minor thing that I noticed, your comment should start with a capital letter and end with a period. Thanks !
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Frédéric

            Show
            dmonllao David Monllaó added a comment - Thanks Frédéric
            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
            samhemelryk Sam Hemelryk added a comment -

            Hi David,

            I've struggled a bit with this issue, hehe taken me ages to review this otherwise small change.
            Presently I think this needs more work, but I've left this in integration to give you a chance to reply in case I have missed something here.

            First up what struck me is that after this change any user who has site:config (pretty much just admins) on a site with more than FRONTPAGECOURSELIMIT courses is going to get sent through the process to print the courses they are enrolled in.
            The first prob with this change is that $CFG->disablemycourses still needs to be adhered to.
            The bigger problem is that this is a fundamental change in the logic here, I say that with an edge of doubt because the code there prior to your change is fricken awful.
            Before this code the my courses list was only printed for users that could be expected to have enrolments. Now it is also printed for privileged users as well which doesn't make overly a lot of sense as its unlikely most admins will have enrolments (once you get out of dev environments for sure).
            On top of that within print_my_moodle if the user has no enrolments then print_courses(0) is going to be called anyway.

            Now, next thing, I'd bet the current code is VERY explicitly not printing anything in the courses list for users without enrolments. Calling print_courses(0) on a site with lots of courses is performance death. It loads every course, it loads the context for every course, and it loads the course contact users for each course as well.
            It is both heavy on the database, and on the PHP processing and output amount.
            Interestingly enough the original code does not factor in users who are not logged in at all, print_courses(0) is still going to be called.
            You could probably bet that most sites with lots of courses will not be using that course list. But that's not you doing.

            Back to the idea behind this issue.
            Nothing is printed for the admin on a large site.
            If we are going to print any sort of course list of admins we had better be very careful about limiting the number of courses displayed within it, not something that happens in the current code by the looks.
            The whole chunk of code responsible for printing the course list needs to be very closely looked at, its a trap presently.
            Printing the search box I think is a smart idea, although I don't think print_my_moodle is the place to put that code either.
            Its a separate component, you could probably expect most users to have a limited number of enrolments (say less than 30) and searching isn't going to be necessary.
            Perhaps a quick solution to this is to leave the current logic as it and introduce an else to print a search box is nothing else at all is being printed. That is going to be more consistent with FRONTPAGECATEGORYNAMES and FRONTPAGECATEGORYCOMBO, although I don't like the idea that potentially you will have multiple search boxes doing the same thing either.
            But either way at least that way you end up with something.

            I think the actual solution to this issue is to take a very close look at the FRONTPAGECOURSELIST, re-evaluate what it is trying to do, and rewrite it.
            Personally I get the impression that it should be doing the following:

            if (the user can be expected to have enrolments) {
            print_my_courses();
            } else if (there are less courses than FRONTPAGECOURSELIMIT) {
            print_courses(0);
            } else {
            print — "There are xxx courses"
            print_course_search();
            }

            Really I think it probably needs discussion at the moment.
            Presently from me this gets a -1 in favour of the above approach.

            In regards to the code you have there a couple of notes:

            1. It would be clearer and easier to read if $courselimitexceeded were true or false, rather than true or unset. That way we can be sure it is set and that it is a bool value. It's what I would expect when dealing with a variable. (also explicit === true | === false checks).
            2. Just noting that ideally we would not run the count_records query unless needed. I understand why you split it out just making mention, the front page is likely to be one of the most hit pages on any site, and we try to keep the queries required for guest|not logged in users to a minimum.

            Many thanks, waiting for you feedback.
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi David, I've struggled a bit with this issue, hehe taken me ages to review this otherwise small change. Presently I think this needs more work, but I've left this in integration to give you a chance to reply in case I have missed something here. First up what struck me is that after this change any user who has site:config (pretty much just admins) on a site with more than FRONTPAGECOURSELIMIT courses is going to get sent through the process to print the courses they are enrolled in. The first prob with this change is that $CFG->disablemycourses still needs to be adhered to. The bigger problem is that this is a fundamental change in the logic here, I say that with an edge of doubt because the code there prior to your change is fricken awful. Before this code the my courses list was only printed for users that could be expected to have enrolments. Now it is also printed for privileged users as well which doesn't make overly a lot of sense as its unlikely most admins will have enrolments (once you get out of dev environments for sure). On top of that within print_my_moodle if the user has no enrolments then print_courses(0) is going to be called anyway. Now, next thing, I'd bet the current code is VERY explicitly not printing anything in the courses list for users without enrolments. Calling print_courses(0) on a site with lots of courses is performance death. It loads every course, it loads the context for every course, and it loads the course contact users for each course as well. It is both heavy on the database, and on the PHP processing and output amount. Interestingly enough the original code does not factor in users who are not logged in at all, print_courses(0) is still going to be called. You could probably bet that most sites with lots of courses will not be using that course list. But that's not you doing. Back to the idea behind this issue. Nothing is printed for the admin on a large site. If we are going to print any sort of course list of admins we had better be very careful about limiting the number of courses displayed within it, not something that happens in the current code by the looks. The whole chunk of code responsible for printing the course list needs to be very closely looked at, its a trap presently. Printing the search box I think is a smart idea, although I don't think print_my_moodle is the place to put that code either. Its a separate component, you could probably expect most users to have a limited number of enrolments (say less than 30) and searching isn't going to be necessary. Perhaps a quick solution to this is to leave the current logic as it and introduce an else to print a search box is nothing else at all is being printed. That is going to be more consistent with FRONTPAGECATEGORYNAMES and FRONTPAGECATEGORYCOMBO, although I don't like the idea that potentially you will have multiple search boxes doing the same thing either. But either way at least that way you end up with something. I think the actual solution to this issue is to take a very close look at the FRONTPAGECOURSELIST, re-evaluate what it is trying to do, and rewrite it. Personally I get the impression that it should be doing the following: if (the user can be expected to have enrolments) { print_my_courses(); } else if (there are less courses than FRONTPAGECOURSELIMIT) { print_courses(0); } else { print — "There are xxx courses" print_course_search(); } Really I think it probably needs discussion at the moment. Presently from me this gets a -1 in favour of the above approach. In regards to the code you have there a couple of notes: It would be clearer and easier to read if $courselimitexceeded were true or false, rather than true or unset. That way we can be sure it is set and that it is a bool value. It's what I would expect when dealing with a variable. (also explicit === true | === false checks). Just noting that ideally we would not run the count_records query unless needed. I understand why you split it out just making mention, the front page is likely to be one of the most hit pages on any site, and we try to keep the queries required for guest|not logged in users to a minimum. Many thanks, waiting for you feedback. Sam
            Hide
            dmonllao David Monllaó added a comment -

            Thank you for the comments Sam; for users without enrolments print_my_moodle() executes print_while_category_list() as long as ($DB->count_records("course_categories") > 1) what I guess should be always in sites with more than FRONTPAGECOURSELIMIT courses, that's why I think it does not represent a significative extra load. Anyway, I've rewritten the patch maintaining the current behaviour and adding the search box in a "else" according to your comments. Pull branches rebased

            Show
            dmonllao David Monllaó added a comment - Thank you for the comments Sam; for users without enrolments print_my_moodle() executes print_while_category_list() as long as ($DB->count_records("course_categories") > 1) what I guess should be always in sites with more than FRONTPAGECOURSELIMIT courses, that's why I think it does not represent a significative extra load. Anyway, I've rewritten the patch maintaining the current behaviour and adding the search box in a "else" according to your comments. Pull branches rebased
            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
            dmonllao David Monllaó added a comment -

            Rebased

            Show
            dmonllao David Monllaó added a comment - Rebased
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks David, has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks David, has been integrated now
            Hide
            dmonllao David Monllaó added a comment -

            Changing the testing instructions according to the last code changes

            Show
            dmonllao David Monllaó added a comment - Changing the testing instructions according to the last code changes
            Hide
            abgreeve Adrian Greeve added a comment -

            Checked on master and then tested on 2.2, 2.3 and master integration branches.
            Everything works as described.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Checked on master and then tested on 2.2, 2.3 and master integration branches. Everything works as described. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!
            Hide
            aborrow Anthony Borrow added a comment -

            I'm not used to seeing Latin in the tracker but well said Eloy! Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - I'm not used to seeing Latin in the tracker but well said Eloy! Peace - Anthony

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12