Moodle
  1. Moodle
  2. MDL-32888

Gradebook: Add filter and/or search

    Details

    • Testing Instructions:
      Hide

      For each of the following types of courses;

      • a course with no students
      • a course with suspended students (depending on user permissions, suspended students may or may not be shown in the gradebook)
      • a course with students with grades
      • a course with students with grades and groups (group mode set to separate/visible)

      Complete the following testing on the gradebook;

      • Select Grades from the Course administration menu
      • The Grader report will be displayed with the Student Search by Initials selector option
      • Search students by firstname first initial
      • Search students by surname first initial
      • Search students by firstname and surname first initial
      • Search students by firstname and surname first initials and groups

      In each case confirm the list of students is correct as expected compared with the complete list of students.

      Check that you are able to override student grades as usual both with no filter applied and a filter applied.

      Show
      For each of the following types of courses; a course with no students a course with suspended students (depending on user permissions, suspended students may or may not be shown in the gradebook) a course with students with grades a course with students with grades and groups (group mode set to separate/visible) Complete the following testing on the gradebook; Select Grades from the Course administration menu The Grader report will be displayed with the Student Search by Initials selector option Search students by firstname first initial Search students by surname first initial Search students by firstname and surname first initial Search students by firstname and surname first initials and groups In each case confirm the list of students is correct as expected compared with the complete list of students. Check that you are able to override student grades as usual both with no filter applied and a filter applied.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-32888_master
    • Rank:
      39958

      Description

      I've got a bunch of courses with ~5000 users. Entering grades for a single user involves finding them in the 132 pages of paginated users, which is pretty laborious. Can you add a filter or a search box, please?

        Activity

        Hide
        Julian Ridden added a comment -

        That is a huge +1 for me. An obvious but lacking feature

        Show
        Julian Ridden added a comment - That is a huge +1 for me. An obvious but lacking feature
        Hide
        Mark Drechsler added a comment -

        Same here - numerous large Uni clients have been pointing to this for a while now as a real problem with large subjects, and a search would at least give some relief from this.

        Can this also have the Partner tag added on behalf of NetSpot please?

        Show
        Mark Drechsler added a comment - Same here - numerous large Uni clients have been pointing to this for a while now as a real problem with large subjects, and a search would at least give some relief from this. Can this also have the Partner tag added on behalf of NetSpot please?
        Hide
        Dan Poltawski added a comment -

        Added partner tag for Mark, and also added Andrew Nicols, because he was doing some work with course module add screen, which could be relevant.

        Show
        Dan Poltawski added a comment - Added partner tag for Mark, and also added Andrew Nicols , because he was doing some work with course module add screen, which could be relevant.
        Hide
        Andrew Nicols added a comment -

        Thanks Dan - yeah, I've been trying to conceptualise a new interface to allow dynamic filtering and loading (infinite-scroll style) for course enrolments.

        The same concept should also be reusable here, though the existing JS would probably need to be rewritten to use event delegation instead of individually wired handlers.

        Show
        Andrew Nicols added a comment - Thanks Dan - yeah, I've been trying to conceptualise a new interface to allow dynamic filtering and loading (infinite-scroll style) for course enrolments. The same concept should also be reusable here, though the existing JS would probably need to be rewritten to use event delegation instead of individually wired handlers.
        Hide
        Andrew Nicols added a comment -

        Not adding the triaged tag until Andrew Davis has had a chance to look at this issue too.

        Show
        Andrew Nicols added a comment - Not adding the triaged tag until Andrew Davis has had a chance to look at this issue too.
        Hide
        Andrew Nicols added a comment -

        As a shorter-term workaround, it should be possible to add filters by initials like have recently been added to the course enrolments page.

        Show
        Andrew Nicols added a comment - As a shorter-term workaround, it should be possible to add filters by initials like have recently been added to the course enrolments page.
        Hide
        Andrew Davis added a comment -

        Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

        For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

        If you have any ideas about this issue or a possible solution please post it here

        Show
        Andrew Davis added a comment - Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment If you have any ideas about this issue or a possible solution please post it here
        Hide
        Melissa Aitkin added a comment -

        I've done a possible change for this, at the following branch https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master

        On empty gradebook:
        1. Should we display no gradebook?
        2. Display empty gradebook with no overall average?
        3. Display empty gradebook with overall average of selected students?
        4. Display empty gradebook with overall average for entire subject?

        Show
        Melissa Aitkin added a comment - I've done a possible change for this, at the following branch https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master On empty gradebook: 1. Should we display no gradebook? 2. Display empty gradebook with no overall average? 3. Display empty gradebook with overall average of selected students? 4. Display empty gradebook with overall average for entire subject?
        Hide
        Russell Smith added a comment -

        I have assigned this request to me as Melissa is not currently a developer. Testing instructions are on the way.

        Show
        Russell Smith added a comment - I have assigned this request to me as Melissa is not currently a developer. Testing instructions are on the way.
        Hide
        Russell Smith added a comment -

        Can somebody please peer review what has been developed and offer any relevant comments on the questions Melissa has asked.

        Thanks

        Show
        Russell Smith added a comment - Can somebody please peer review what has been developed and offer any relevant comments on the questions Melissa has asked. Thanks
        Hide
        Mark van Hoek added a comment -

        On empty gradebook:
        I suggest #3 (Display empty gradebook with overall average of selected students), but change the text to "Overall Average (filtered)" any time the results are filtered. Revert the text to "Overall Average" when the filter is removed.

        Show
        Mark van Hoek added a comment - On empty gradebook: I suggest #3 (Display empty gradebook with overall average of selected students), but change the text to "Overall Average (filtered)" any time the results are filtered. Revert the text to "Overall Average" when the filter is removed.
        Hide
        Melissa Aitkin added a comment -

        I've amended the change to move the search filter under user/renderer.php so that it can be re-used by other modules. It doesn't actually make sense to display an average for those students starting with letter 's' so I haven't created a filtered average. I've also removed the average rows if no users have been selected.

        Show
        Melissa Aitkin added a comment - I've amended the change to move the search filter under user/renderer.php so that it can be re-used by other modules. It doesn't actually make sense to display an average for those students starting with letter 's' so I haven't created a filtered average. I've also removed the average rows if no users have been selected.
        Hide
        Andrew Davis added a comment -

        Some initial thoughts.

        Change the testing instructions to use the word "course" instead of "subject". Course is the standard Moodle terminology. A huge proportion of people contributing to Moodle are not native English speakers so it's easier if we limit the use of synonyms.

        I haven't checked but does filtering in combination with paging still function correctly?

        The "reset" button is visible and enabled the first time you go to the gradebook even if you haven't filtered. It's also kind of on its on so its not clear what exactly is being reset. Maybe it should be to the right of the alphabet selection element.

        Does the reset button need to exist at all? As I understand what is going on you can reach the same state by selecting "All" for both first and last name.

        Show
        Andrew Davis added a comment - Some initial thoughts. Change the testing instructions to use the word "course" instead of "subject". Course is the standard Moodle terminology. A huge proportion of people contributing to Moodle are not native English speakers so it's easier if we limit the use of synonyms. I haven't checked but does filtering in combination with paging still function correctly? The "reset" button is visible and enabled the first time you go to the gradebook even if you haven't filtered. It's also kind of on its on so its not clear what exactly is being reset. Maybe it should be to the right of the alphabet selection element. Does the reset button need to exist at all? As I understand what is going on you can reach the same state by selecting "All" for both first and last name.
        Hide
        Melissa Aitkin added a comment -

        The filtering works with the paging.
        The Reset button was suggested by a user to cut down the amount of mouse clicks they perform. Reducing two clicks to one doesn't sound like much, but it adds up when you are doing a lot of filtering.
        I'm happy to remove it because the coding was a little clunky.
        Else I can move it to the right. I will probably have to add some css to get it to sit nicely next to the alphabet section.
        Are you able to update the testing instructions? I don't think I have the access. Else I'll ask a colleague to do it for me.
        My main concern with writing this code was using the $SESSION->graderreportsifirst variables, is there a better way of doing this, that fits into the moodle framework?

        Show
        Melissa Aitkin added a comment - The filtering works with the paging. The Reset button was suggested by a user to cut down the amount of mouse clicks they perform. Reducing two clicks to one doesn't sound like much, but it adds up when you are doing a lot of filtering. I'm happy to remove it because the coding was a little clunky. Else I can move it to the right. I will probably have to add some css to get it to sit nicely next to the alphabet section. Are you able to update the testing instructions? I don't think I have the access. Else I'll ask a colleague to do it for me. My main concern with writing this code was using the $SESSION->graderreportsifirst variables, is there a better way of doing this, that fits into the moodle framework?
        Hide
        Andrew Davis added a comment - - edited

        I'm not sure the reset button does save clicks. It saves you one if you want to apply a filter on both first and surname then remove the filter. If however you want to apply a filter then apply a different filter (say looking for "Bob" then moving on to "Sarah") using the reset button actually adds a click. If you only apply a single name filter then clicking reset is no faster than clicking "all".

        Plus, the first time I saw it a button marked "reset" on the grader report it kind of scared me. I was half it expecting it to throw away grades.

        Using $SESSION like that definitely isn't a good idea (although it appears that we do it elsewhere). If you have the grader report open in multiple tabs confusing things happen as they each overwrite the filter variables.

        As I understand this issue the filtering is only being applied to the grader report, not the user report and overview report. If thats the case the filtering stuff should be in grade/report/grader/lib.php and not in grade/report/lib.php (which is the parent class of all the grade reports).

        Perhaps it would be better to pass the filter parameters to the grade_report_grader constructor then store them at something like $this->filterfirstname and $this->filtersurname (or whatever). Once you've moved setup_users() out of the parent class that should work fine.

        Show
        Andrew Davis added a comment - - edited I'm not sure the reset button does save clicks. It saves you one if you want to apply a filter on both first and surname then remove the filter. If however you want to apply a filter then apply a different filter (say looking for "Bob" then moving on to "Sarah") using the reset button actually adds a click. If you only apply a single name filter then clicking reset is no faster than clicking "all". Plus, the first time I saw it a button marked "reset" on the grader report it kind of scared me. I was half it expecting it to throw away grades. Using $SESSION like that definitely isn't a good idea (although it appears that we do it elsewhere). If you have the grader report open in multiple tabs confusing things happen as they each overwrite the filter variables. As I understand this issue the filtering is only being applied to the grader report, not the user report and overview report. If thats the case the filtering stuff should be in grade/report/grader/lib.php and not in grade/report/lib.php (which is the parent class of all the grade reports). Perhaps it would be better to pass the filter parameters to the grade_report_grader constructor then store them at something like $this->filterfirstname and $this->filtersurname (or whatever). Once you've moved setup_users() out of the parent class that should work fine.
        Hide
        Melissa Aitkin added a comment -

        I will remove the Reset button and look at these suggestions.
        From memory, I thought the report was re-created each time, therefore previously selected filter options were lost...

        Also, despite the filter being used for the grader report, I thought that by adding it to the user module it could be used by other areas in the future.

        Show
        Melissa Aitkin added a comment - I will remove the Reset button and look at these suggestions. From memory, I thought the report was re-created each time, therefore previously selected filter options were lost... Also, despite the filter being used for the grader report, I thought that by adding it to the user module it could be used by other areas in the future.
        Hide
        Andrew Davis added a comment -
        From memory, I thought the report was re-created each time, therefore previously selected filter options were lost...
        

        You're right. I managed to stumble on probably the only situation where this is an issue. I opened the grader report and set a filter then right clicked grader report in the nav breadcrumb thing and opened a second tab. That URL contains no filter information so it didn't overwrite the session variables. That meant that my new grader report inherited the existing filter. Maybe that's not a problem after all.

        Ok, if the filter is (possibly) going into the other grade reports then it should stay in the parent class. It may still be worth shifting from the session variables to using class member variables but I'm not so sure now...

        Show
        Andrew Davis added a comment - From memory, I thought the report was re-created each time, therefore previously selected filter options were lost... You're right. I managed to stumble on probably the only situation where this is an issue. I opened the grader report and set a filter then right clicked grader report in the nav breadcrumb thing and opened a second tab. That URL contains no filter information so it didn't overwrite the session variables. That meant that my new grader report inherited the existing filter. Maybe that's not a problem after all. Ok, if the filter is (possibly) going into the other grade reports then it should stay in the parent class. It may still be worth shifting from the session variables to using class member variables but I'm not so sure now...
        Hide
        Melissa Aitkin added a comment -

        We need to store the filter information somewhere because a user might search initially on surnames starting with 'S', and then want to narrow it down by firstnames starting with 'A'. In the current class framework the first filter will be lost. I await to hear if you can see a better way of storing these values...

        Show
        Melissa Aitkin added a comment - We need to store the filter information somewhere because a user might search initially on surnames starting with 'S', and then want to narrow it down by firstnames starting with 'A'. In the current class framework the first filter will be lost. I await to hear if you can see a better way of storing these values...
        Hide
        Melissa Aitkin added a comment -

        Possible solution - the grader report is using the USER object to store some info - such as $USER->grade_last_report and $USER->gradeediting. It might make sense for me to set filterfirstname and filtersurname to this object?

        Show
        Melissa Aitkin added a comment - Possible solution - the grader report is using the USER object to store some info - such as $USER->grade_last_report and $USER->gradeediting. It might make sense for me to set filterfirstname and filtersurname to this object?
        Hide
        Melissa Aitkin added a comment -

        I have updated my branch https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master

        The reset button has been removed from the grader filter and the USER object is now being used to store grade filter information, rather than the SESSION object

        Show
        Melissa Aitkin added a comment - I have updated my branch https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master The reset button has been removed from the grader filter and the USER object is now being used to store grade filter information, rather than the SESSION object
        Hide
        Russell Smith added a comment -

        Melissa has been the developer on this since the start. She now has developer access so I'm correctly assigning it to her.

        Show
        Russell Smith added a comment - Melissa has been the developer on this since the start. She now has developer access so I'm correctly assigning it to her.
        Hide
        Andrew Davis added a comment -

        Hi. Sorry about the delay. The branch needs to be rebased. Im seeing 250+ commits in the diff.

        Show
        Andrew Davis added a comment - Hi. Sorry about the delay. The branch needs to be rebased. Im seeing 250+ commits in the diff.
        Hide
        Melissa Aitkin added a comment -

        I've done the rebase.

        Show
        Melissa Aitkin added a comment - I've done the rebase.
        Hide
        Andrew Davis added a comment -

        Some very minor things.

        You need to have a space either side of =>. For example, 'gitemid'=>$this->sortitemid. http://docs.moodle.org/dev/Coding_style#Associative_arrays

        To be entirely correct comments like "// Bar of last initials" should have a full-stop. It looks weird when you first start doing it but you get used to it. http://docs.moodle.org/dev/Coding_style#Inline_comments

        Probably expand the testing instructions to include overriding student grades with a filter enabled.

        Otherwise, put this up for integration whenever you're ready.

        Show
        Andrew Davis added a comment - Some very minor things. You need to have a space either side of =>. For example, 'gitemid'=>$this->sortitemid. http://docs.moodle.org/dev/Coding_style#Associative_arrays To be entirely correct comments like "// Bar of last initials" should have a full-stop. It looks weird when you first start doing it but you get used to it. http://docs.moodle.org/dev/Coding_style#Inline_comments Probably expand the testing instructions to include overriding student grades with a filter enabled. Otherwise, put this up for integration whenever you're ready.
        Hide
        Melissa Aitkin added a comment -

        Hi Andrew

        I've made those style changes. And I've also made another fix to cater for suspended students in the gradebook. I discovered that depending on a user's permissions, suspended students may or may not be shown in the gradebook, and this then affects the participants total I display.

        I do not have the authority to put this change up for integration review.

        Could you please perform one last peer review and then, if happy, promote for integration review?

        Thanks

        Show
        Melissa Aitkin added a comment - Hi Andrew I've made those style changes. And I've also made another fix to cater for suspended students in the gradebook. I discovered that depending on a user's permissions, suspended students may or may not be shown in the gradebook, and this then affects the participants total I display. I do not have the authority to put this change up for integration review. Could you please perform one last peer review and then, if happy, promote for integration review? Thanks
        Hide
        Andrew Davis added a comment -

        Submitting for integration.

        Show
        Andrew Davis added a comment - Submitting for integration.
        Hide
        Dan Poltawski added a comment -

        Hi Melissa,

        Thanks for this great feature!

        I'm afraid there are a few things which don't look quite ready yet:

        1. Caching things the $USER global is not permitted. We could potentially use a MUC session cache for this - but not the $USER globa.
        2. I don't think that the parameter name 'sifirst'/'silast' is easy to read and measningful, as per the Moodle coding style. Please consider a longer more verbse variable name http://docs.moodle.org/dev/Coding_style#Variables
        3. It looks like there has been some trailing whitespace introduced in grade/report/grader/lib.php
        4. Use of ILIKE should be converted to use $DB->sql_like() so that it is cross-database compatible.
        5. If you could squash your commits down just to show logical changes and not fixes along the way.

        Thanks a lot!
        Dan

        Show
        Dan Poltawski added a comment - Hi Melissa, Thanks for this great feature! I'm afraid there are a few things which don't look quite ready yet: Caching things the $USER global is not permitted. We could potentially use a MUC session cache for this - but not the $USER globa. I don't think that the parameter name 'sifirst'/'silast' is easy to read and measningful, as per the Moodle coding style. Please consider a longer more verbse variable name http://docs.moodle.org/dev/Coding_style#Variables It looks like there has been some trailing whitespace introduced in grade/report/grader/lib.php Use of ILIKE should be converted to use $DB->sql_like() so that it is cross-database compatible. If you could squash your commits down just to show logical changes and not fixes along the way. Thanks a lot! Dan
        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
        Tim Hunt added a comment -

        To add to two of Dan's points.

        1. However, caching small amounts of data in $SESSION always used to be allowed, and it probably still OK.

        2. lib/tablelib.php uses tifirst/tilast for its initials bars, and that is intentionally the same for all tables in Moodle. I think it also stores them in the same place in the session, so that this settings persists as you move between tables. Therefore, I think this change should slot into the same system. (Although the way it is implemented in tablelib is pretty crazy.)

        Show
        Tim Hunt added a comment - To add to two of Dan's points. 1. However, caching small amounts of data in $SESSION always used to be allowed, and it probably still OK. 2. lib/tablelib.php uses tifirst/tilast for its initials bars, and that is intentionally the same for all tables in Moodle. I think it also stores them in the same place in the session, so that this settings persists as you move between tables. Therefore, I think this change should slot into the same system. (Although the way it is implemented in tablelib is pretty crazy.)
        Hide
        Melissa Aitkin added a comment -

        Dan/Tim,

        Can I get clarity on a couple of points

        1. So it is ok for me to use the $SESSION object in this instance?
        2. 'sifirst/silast' are used in other user and report modules. Do you still want me to change them? Do you want me to change them to tifirst/tilast? If I change them, is firstinitial/lastinitial satisfactory?

        Thanks

        Show
        Melissa Aitkin added a comment - Dan/Tim, Can I get clarity on a couple of points 1. So it is ok for me to use the $SESSION object in this instance? 2. 'sifirst/silast' are used in other user and report modules. Do you still want me to change them? Do you want me to change them to tifirst/tilast? If I change them, is firstinitial/lastinitial satisfactory? Thanks
        Hide
        Melissa Aitkin added a comment -

        I've made the suggested changes: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master
        Please push to integration review and testing

        Show
        Melissa Aitkin added a comment - I've made the suggested changes: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master Please push to integration review and testing
        Hide
        Damyon Wiese added a comment -

        Thanks for working on this. It looks like a popular request. There are a few more things that need fixing in this patch.

        1. Trailing whitespace everywhere (I suggest you setup your editor so you can see these easily)
        2. The changes to public functions need to be documented in the correct upgrade.txt file (e.g. new param for get_grade_table should be reported in grade/report/upgrade.txt)
        3. The small amount of data stored in the session is ok (would be better in a MUC session cache) - but you should encapsulate the data better. $SESSION->filterfirstname is not very specific - and it's not clear the data belongs to the grader report. If it is going in the session it would be better to put it e.g. in $SESSION->grade_report_search['filterfirstname'] - and keep everything in the one array.
        4. Using a custom sql query to get the enrolled users is code duplication and when the logic for enrolled users is changed, this will break. Please use the api functions (get_enrolled_users) - not direct DB queries.
        5. setup_users is not documented
        6. user/renderer.php has comments missing a trailing full stop.

        In general you should use the codechecker and moodlecheck plugins on your code before sending to integration as these will pickup alot of these errors for you.

        I'm reopening this now - please address these errors.

        Show
        Damyon Wiese added a comment - Thanks for working on this. It looks like a popular request. There are a few more things that need fixing in this patch. Trailing whitespace everywhere (I suggest you setup your editor so you can see these easily) The changes to public functions need to be documented in the correct upgrade.txt file (e.g. new param for get_grade_table should be reported in grade/report/upgrade.txt) The small amount of data stored in the session is ok (would be better in a MUC session cache) - but you should encapsulate the data better. $SESSION->filterfirstname is not very specific - and it's not clear the data belongs to the grader report. If it is going in the session it would be better to put it e.g. in $SESSION->grade_report_search ['filterfirstname'] - and keep everything in the one array. Using a custom sql query to get the enrolled users is code duplication and when the logic for enrolled users is changed, this will break. Please use the api functions (get_enrolled_users) - not direct DB queries. setup_users is not documented user/renderer.php has comments missing a trailing full stop. In general you should use the codechecker and moodlecheck plugins on your code before sending to integration as these will pickup alot of these errors for you. I'm reopening this now - please address these errors.
        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
        Melissa Aitkin added a comment -

        I've made those changes, including cleaning up existing code, see: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master
        Please push to integration review and testing

        Show
        Melissa Aitkin added a comment - I've made those changes, including cleaning up existing code, see: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master Please push to integration review and testing
        Hide
        Andrew Nicols added a comment -

        Hi Melissa,

        I've Just noticed that this patch includes some changes to .gitignore which shouldn't be there.
        There are also quite a few whitespace changes in most of the affected files, and a couple of null => NULL (or vice-versa) changes which shouldn't be there.

        Generally we don't make whitespace changes to unaffected lines as this pollutes the history.

        Andrew

        Show
        Andrew Nicols added a comment - Hi Melissa, I've Just noticed that this patch includes some changes to .gitignore which shouldn't be there. There are also quite a few whitespace changes in most of the affected files, and a couple of null => NULL (or vice-versa) changes which shouldn't be there. Generally we don't make whitespace changes to unaffected lines as this pollutes the history. Andrew
        Hide
        Andrew Nicols added a comment -

        Removing the AJAX/JavaScript label as this is not JS related.

        Show
        Andrew Nicols added a comment - Removing the AJAX/JavaScript label as this is not JS related.
        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 -

        Thanks Melissa,

        There are still a few (very minor) things that need fixing in this patch:

        "@var unknown $context" - this needs the correct type - but you can just not add the docs here as it's not affected by your changes anyway.

        +/**
        + * Print private files tree
        + * @copyright  2010 Dongsheng Cai <dongsheng@moodle.com>
        + * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
        + */
         class core_user_renderer extends plugin_renderer_base 
        

        This phpdoc is incorrect (the class does more than just the files tree).

        You still have .gitignore in the patch.

        This is almost there - thanks!

        Show
        Damyon Wiese added a comment - Thanks Melissa, There are still a few (very minor) things that need fixing in this patch: "@var unknown $context" - this needs the correct type - but you can just not add the docs here as it's not affected by your changes anyway. +/** + * Print private files tree + * @copyright 2010 Dongsheng Cai <dongsheng@moodle.com> + * @license http: //www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ class core_user_renderer extends plugin_renderer_base This phpdoc is incorrect (the class does more than just the files tree). You still have .gitignore in the patch. This is almost there - thanks!
        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
        Melissa Aitkin added a comment -

        Changes have been made, see: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master
        Please push to integration review and testing

        Show
        Melissa Aitkin added a comment - Changes have been made, see: https://github.com/maitkin-ltu/moodle/compare/master...MDL-32888_master Please push to integration review and testing
        Hide
        Melissa Aitkin added a comment - - edited

        Could someone push this to integration for final review and testing. I want to complete this issue.

        Show
        Melissa Aitkin added a comment - - edited Could someone push this to integration for final review and testing. I want to complete this issue.
        Hide
        Dan Poltawski added a comment -

        I've added this to integration_held as its a new feature after freeze. Please get Martin Dougiamas to unblock it if we want this to make it for 2.6

        Show
        Dan Poltawski added a comment - I've added this to integration_held as its a new feature after freeze. Please get Martin Dougiamas to unblock it if we want this to make it for 2.6
        Hide
        Dan Poltawski 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
        Dan Poltawski 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 -

        Thanks Melissa,

        Lots of good cleanup in the gradebook there (though that should really have been done in a separate issue).

        This patch works well for me. Integrated to master only.

        Cheers - Damyon

        Show
        Damyon Wiese added a comment - Thanks Melissa, Lots of good cleanup in the gradebook there (though that should really have been done in a separate issue). This patch works well for me. Integrated to master only. Cheers - Damyon
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        Tested in Master only.

        Show
        Michael de Raadt added a comment - Test result: Success! Tested in Master only.
        Hide
        Dan Poltawski added a comment -

        Thanks for your contributions, this change is now upstream!

        “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

        Show
        Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

          People

          • Votes:
            27 Vote for this issue
            Watchers:
            17 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: