Moodle
  1. Moodle
  2. MDL-10259

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      3693

      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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Anthony Borrow added a comment -

          oops, I mixed up the affected and fix versions

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

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

          Regards, David

          Show
          David Monllaó added a comment - Thanks Anthony, pull branches updated on top of your patch. Regards, David
          Hide
          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
          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
          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
          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
          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
          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
          David Monllaó added a comment -

          Thanks Frédéric

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

          Unknown macro: { print_my_courses(); }

          else if (there are less courses than FRONTPAGECOURSELIMIT)

          Unknown macro: { print_courses(0); }

          else

          Unknown macro: { 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
          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) Unknown macro: { print_my_courses(); } else if (there are less courses than FRONTPAGECOURSELIMIT) Unknown macro: { print_courses(0); } else Unknown macro: { 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
          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
          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
          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
          David Monllaó added a comment -

          Rebased

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

          Thanks David, has been integrated now

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

          Changing the testing instructions according to the last code changes

          Show
          David Monllaó added a comment - Changing the testing instructions according to the last code changes
          Hide
          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
          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
          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
          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
          Anthony Borrow added a comment -

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

          Show
          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: