Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Cohorts, Enrolments
    • Labels:
      None
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Testing cohorts enrolment:
      1. Ensure that you have more than 25 cohorts in the system
      2. Open the course and choose Users > Enrolled Users
      3. Click on the "Enrol Cohort" button
      4. Ensure that search box is working and no more than 25 cohorts are listed
      5. Click on "More...", more cohorts should be listed

      Testing cohorts management:
      1. Ensure that you have more than 25 cohorts in the system
      2. Open Home ► Site administration ► Users ► Accounts ► Cohorts
      3. Ensure that the search and pagination is working. Only 25 items are listed per page.

      Show
      Testing cohorts enrolment: 1. Ensure that you have more than 25 cohorts in the system 2. Open the course and choose Users > Enrolled Users 3. Click on the "Enrol Cohort" button 4. Ensure that search box is working and no more than 25 cohorts are listed 5. Click on "More...", more cohorts should be listed Testing cohorts management: 1. Ensure that you have more than 25 cohorts in the system 2. Open Home ► Site administration ► Users ► Accounts ► Cohorts 3. Ensure that the search and pagination is working. Only 25 items are listed per page.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29695-squashed
    • Rank:
      19205

      Description

      It would be useful to add pagination and search functionality to both cohorts management page and cohorts enrolment interface.

        Activity

        Show
        Ruslan Kabalin added a comment - - edited Diffs for inspection: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=51b6fe045bf7b25450673e08db8e11909b70e3e5 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=3779fabec17009039d7751efa912b8985b7b9b34
        Hide
        Ruslan Kabalin added a comment -

        Patches can be cherry-picked to 2.1 stable with a small conflict in the language file.

        Show
        Ruslan Kabalin added a comment - Patches can be cherry-picked to 2.1 stable with a small conflict in the language file.
        Hide
        Sam Hemelryk added a comment -

        Hi Ruslan,

        The changes look fantastic thank you and certainly after a quick test seem to be working well.
        However before this gets integrated there are a couple of things that need to be tidied up and discussed

        First up there are two larger issues to raise:

        1. Because of the change to functionality to enrol_cohort_get_cohorts (default changes from returning all to returning just the first 25) this would not be suitable for back porting to Moodle 2.1.X.
          While looking at the changes to that function I can't help but think perhaps we should have two functions now - the first enrol_cohort_get_cohorts (in it's original state) and the second enrol_cohort_search_cohorts…. read onto the next issue to find out why this may be a good idea.
        2. There is a potential bug in the search code because of the capability check that is being run on cohorts before returning them. The SQL will return 25 issues however the capability check will for some users lead to less than that being returned, if any. On top of that the total count will also be out which will lead to pagination problems if it is out by more than 25.
          One solution to this problem as well as the above is to us a separate search function that calls enrol_cohort_get_cohorts to get all cohorts and then searches on the already capability checked results. Not great for performance but a tidy way of handling things. Alternatively splitting the search to enrol_cohort_get_cohorts as an optional param without pagination there - and handling pagination in the new function would allow you to use SQL search. Either way I'm not 100% sure the best approach presently - have a think about it Ruslan and see what you come up with - we can always ask Eloy to have a look and get his thoughts to.

        The following are more minor issues - some that would need to be fixed up before integration and some that would just be nice to fix up

        1. cohort/index.php $search_query should be $searchquery as per our coding guidelines
        2. The search param name I think should be just `search` rather than search_query (inline with the what Moodle uses generally and inline with the manual enrolment plugins JS widget.
        3. cohort/lib.php `get_cohorts` should be named cohort_get_cohorts` to make it's name consistent with the other functions in that file.
        4. There is an inconsistency in the search fields between get_cohorts and enrol_cohort_get_cohorts, the first has name and description, the second has idnumber as well.
        5. enrol/cohort/locallib.php within enrol_cohort_get_cohorts the changes to the cohort name when processing the recordset will cause regressions. name and description would need to be formatted separately so that descriptions format could be used. Are you sure you want to be including the description there and then shortening it? 30 characters split less the length of the name won't likely leave much space for the description and I don't know whether it would add real value. This change gets a -1 from me.
        6. Within function definitions there is a whitespace issue $page=0 should be $page = 0.
        7. Double indenting in enrol/cohort/yui/quickenrol/quickenrolment.js round like 56.
        8. Unneeded call to e.preventDefault (e.halt calls that for you) in the above file round line 183.

        Thanks for the great work so far.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Ruslan, The changes look fantastic thank you and certainly after a quick test seem to be working well. However before this gets integrated there are a couple of things that need to be tidied up and discussed First up there are two larger issues to raise: Because of the change to functionality to enrol_cohort_get_cohorts (default changes from returning all to returning just the first 25) this would not be suitable for back porting to Moodle 2.1.X. While looking at the changes to that function I can't help but think perhaps we should have two functions now - the first enrol_cohort_get_cohorts (in it's original state) and the second enrol_cohort_search_cohorts…. read onto the next issue to find out why this may be a good idea. There is a potential bug in the search code because of the capability check that is being run on cohorts before returning them. The SQL will return 25 issues however the capability check will for some users lead to less than that being returned, if any. On top of that the total count will also be out which will lead to pagination problems if it is out by more than 25. One solution to this problem as well as the above is to us a separate search function that calls enrol_cohort_get_cohorts to get all cohorts and then searches on the already capability checked results. Not great for performance but a tidy way of handling things. Alternatively splitting the search to enrol_cohort_get_cohorts as an optional param without pagination there - and handling pagination in the new function would allow you to use SQL search. Either way I'm not 100% sure the best approach presently - have a think about it Ruslan and see what you come up with - we can always ask Eloy to have a look and get his thoughts to. The following are more minor issues - some that would need to be fixed up before integration and some that would just be nice to fix up cohort/index.php $search_query should be $searchquery as per our coding guidelines The search param name I think should be just `search` rather than search_query (inline with the what Moodle uses generally and inline with the manual enrolment plugins JS widget. cohort/lib.php `get_cohorts` should be named cohort_get_cohorts` to make it's name consistent with the other functions in that file. There is an inconsistency in the search fields between get_cohorts and enrol_cohort_get_cohorts, the first has name and description, the second has idnumber as well. enrol/cohort/locallib.php within enrol_cohort_get_cohorts the changes to the cohort name when processing the recordset will cause regressions. name and description would need to be formatted separately so that descriptions format could be used. Are you sure you want to be including the description there and then shortening it? 30 characters split less the length of the name won't likely leave much space for the description and I don't know whether it would add real value. This change gets a -1 from me. Within function definitions there is a whitespace issue $page=0 should be $page = 0. Double indenting in enrol/cohort/yui/quickenrol/quickenrolment.js round like 56. Unneeded call to e.preventDefault (e.halt calls that for you) in the above file round line 183. Thanks for the great work so far. Cheers Sam
        Hide
        Ruslan Kabalin added a comment -

        Hello Sam,

        Thanks a lot for revision. Good job you have spotted a potential issue with capabilities/pagination. I have updated the branch with some fixes for further revision, once you are fine with everything, I will merge them all in the single initial commit.

        I have decided to preserve the initial function for compatibility purposes as you suggested (https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=f7b20ccd97eb8a112339eb32c93bb470fd405ec7).

        For the permission check I have chosen the different approach. I got rid of the counter which allowed me to make a capability check for some items only. Basically I get all cohorts for required contexts and then check capabilities until I have 25 plus one items, then these 25 are displayed and based on existence of 26th one I decide if "More..." link needs to be displayed at the bottom. On the consequent queries the offset is used and the same approach is used until there will be no more items. The reason for this is that capability check is pretty heavy, and with more than 1000 cohorts, the page load takes more than 5 sec which is slow. You may examine the change here: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=db29892542d914145edeffed127d024a1be756b7 For the same reason I have changed the single cohort validation approach (see https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=9d84f47261c26cfe39f82c714b3b1da0514e75fb)

        Now regarding minor issues. I have fixed them all, thanks for spotting them. The one that requires commenting is no. 5. I think having a part of the description there is a good idea - the 'Name' might not be intuitive and partial description will give more info to the user. See attached image for example. Though, I have extended it slightly and changed the formatting approach.

        Looking forward for further comments,
        Thanks,
        Ruslan

        Show
        Ruslan Kabalin added a comment - Hello Sam, Thanks a lot for revision. Good job you have spotted a potential issue with capabilities/pagination. I have updated the branch with some fixes for further revision, once you are fine with everything, I will merge them all in the single initial commit. I have decided to preserve the initial function for compatibility purposes as you suggested ( https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=f7b20ccd97eb8a112339eb32c93bb470fd405ec7 ). For the permission check I have chosen the different approach. I got rid of the counter which allowed me to make a capability check for some items only. Basically I get all cohorts for required contexts and then check capabilities until I have 25 plus one items, then these 25 are displayed and based on existence of 26th one I decide if "More..." link needs to be displayed at the bottom. On the consequent queries the offset is used and the same approach is used until there will be no more items. The reason for this is that capability check is pretty heavy, and with more than 1000 cohorts, the page load takes more than 5 sec which is slow. You may examine the change here: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=db29892542d914145edeffed127d024a1be756b7 For the same reason I have changed the single cohort validation approach (see https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=9d84f47261c26cfe39f82c714b3b1da0514e75fb ) Now regarding minor issues. I have fixed them all, thanks for spotting them. The one that requires commenting is no. 5. I think having a part of the description there is a good idea - the 'Name' might not be intuitive and partial description will give more info to the user. See attached image for example. Though, I have extended it slightly and changed the formatting approach. Looking forward for further comments, Thanks, Ruslan
        Hide
        Dan Poltawski added a comment -

        Adding Sam, and giving him a ping to see if we can get comments before the next integration cycle

        Show
        Dan Poltawski added a comment - Adding Sam, and giving him a ping to see if we can get comments before the next integration cycle
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        I have been meaning to get to this issue to have a look at but have run out of time sorry.
        Unfortunately I wont' be near a computer this weekend as well so it will have to wait till Monday sorry. However I will make it #1 on my hitlist and will either get it in or give you feedback ASAP.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, I have been meaning to get to this issue to have a look at but have run out of time sorry. Unfortunately I wont' be near a computer this weekend as well so it will have to wait till Monday sorry. However I will make it #1 on my hitlist and will either get it in or give you feedback ASAP. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        Thanks for making the changes Ruslan - things are really starting to look spot on.
        I've just been looking at this now and have noted the following things that really should be tidied up before this gets integrated:

        • In Firefox 7 (perhaps earlier versions I havn't tested) there is a display bug with the Cohort AJAX widget, I'll attach a screenshot. I did note that in Chrome it looked fine and I found removing the float:right from .qce-panel .qce-cohort .qce-cohort-users fixed the problem although changed the alignment slightly.
        • enrol_cohort_search_cohorts The $output argument I think should be renamed to something like $limit. $output isn't descriptive of what the argument is really doing and we commonly use $output internally for building HTML output.
        • The generation of $name within enrol_cohort_search_cohorts (name + description then all shortened) is wrong - each part needs to be individually formatted using the context of the cohort, then joined, and shortened. However I think that it should not be including description in here anyway for two reasons a) enrol_cohort_get_cohorts uses just the name so it would be more consistent to do the same, and b) when you shorten it so much it's not likely you will get anything meaningful from description. Of course the second point depends upon how it is used however 35 chars is pretty short.
        • The new method enrol_cohort_can_access_cohort I think should be renamed to enrol_cohort_can_view_cohort. Again I think this is more descriptive of what the function is doing and more consistent with other areas of Moodle.

        I've left this in integration at the moment - I think the above four points need to be addressed before integration. Let me know if you are happy with the above changes and either let me know when you've made them, or if you prefer as they are all pretty minor I can make them during integration.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, Thanks for making the changes Ruslan - things are really starting to look spot on. I've just been looking at this now and have noted the following things that really should be tidied up before this gets integrated: In Firefox 7 (perhaps earlier versions I havn't tested) there is a display bug with the Cohort AJAX widget, I'll attach a screenshot. I did note that in Chrome it looked fine and I found removing the float:right from .qce-panel .qce-cohort .qce-cohort-users fixed the problem although changed the alignment slightly. enrol_cohort_search_cohorts The $output argument I think should be renamed to something like $limit. $output isn't descriptive of what the argument is really doing and we commonly use $output internally for building HTML output. The generation of $name within enrol_cohort_search_cohorts (name + description then all shortened) is wrong - each part needs to be individually formatted using the context of the cohort, then joined, and shortened. However I think that it should not be including description in here anyway for two reasons a) enrol_cohort_get_cohorts uses just the name so it would be more consistent to do the same, and b) when you shorten it so much it's not likely you will get anything meaningful from description. Of course the second point depends upon how it is used however 35 chars is pretty short. The new method enrol_cohort_can_access_cohort I think should be renamed to enrol_cohort_can_view_cohort. Again I think this is more descriptive of what the function is doing and more consistent with other areas of Moodle. I've left this in integration at the moment - I think the above four points need to be addressed before integration. Let me know if you are happy with the above changes and either let me know when you've made them, or if you prefer as they are all pretty minor I can make them during integration. Cheers Sam
        Hide
        Ruslan Kabalin added a comment -

        Hi Sam,

        Thanks a lot for further revision. I have fixed all your four points. Thanks.

        Ruslan

        Show
        Ruslan Kabalin added a comment - Hi Sam, Thanks a lot for further revision. I have fixed all your four points. Thanks. Ruslan
        Hide
        Ruslan Kabalin added a comment -

        I have created a squashed branch that contains all revision commits squashed into original two.

        Show
        Ruslan Kabalin added a comment - I have created a squashed branch that contains all revision commits squashed into original two.
        Hide
        Sam Hemelryk added a comment -

        Awesome thanks Ruslan - I'll look to integrate this tomorrow (in about 9 hours now lol)

        Show
        Sam Hemelryk added a comment - Awesome thanks Ruslan - I'll look to integrate this tomorrow (in about 9 hours now lol)
        Hide
        Sam Hemelryk added a comment -

        Thanks Ruslan - this has been integrated now

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Ruslan - this has been integrated now Cheers Sam
        Hide
        Ruslan Kabalin added a comment -

        My pleasure. Thanks a lot Sam for the constructive comments )) Also, thanks Dan for useful advises.

        Show
        Ruslan Kabalin added a comment - My pleasure. Thanks a lot Sam for the constructive comments )) Also, thanks Dan for useful advises.
        Hide
        Rossiani Wijaya added a comment -

        Thanks Ruslan for working on this issue.

        This is working great.

        Test passed.

        Show
        Rossiani Wijaya added a comment - Thanks Ruslan for working on this issue. This is working great. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: