Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.7, 2.3, 2.4
    • Fix Version/s: 2.4
    • Component/s: Gradebook
    • Labels:
    • Environment:
      LAMP
    • Database:
      Any
    • Testing Instructions:
      Hide

      Test pre-requisites

      • 4 user custom fields as follow: firstname, foo, bar, foobar
      • User A is a teacher with capability course:viewhiddenuserfields
      • User B is a teacher without capability course:viewhiddenuserfields
      • Some students with custom fields filled (set firstname to something different than the user firstname)
      • Set setting grade_export_userprofilefields to its default value, but remove 'email' and add 'password,idontexist'
      • Set setting grade_export_customprofilefields to 'foo,bar,firstname'
      • Set setting $CFG->hiddenuserfields to 'bar,lastname' in your config file
      • Have a few grades on a few activities for the students

      Test 1

      1. As user A go to the gradebook
      2. Export the grades to plain text file
      3. In the preview, make sure that:

      • password, idontexist, email DO NOT appear
      • foo, bar, lastname appear
      • firstname appears TWICE with different values

      4. Export the file, and make sure

      • password, idontexist, email DO NOT appear
      • foo, bar, lastname appear
      • firstname appears TWICE with different values

      5. Repeat steps exporting to XLS and ODS.

      Test 2

      1. As user B go to the gradebook
      2. Export the grades to plain text file
      3. In the preview, make sure that:

      • password, idontexist, email, lastname, bar DO NOT appear
      • foo appears
      • firstname appears TWICE with different values

      4. Export the file, and make sure

      • password, idontexist, email, lastname, bar DO NOT appear
      • foo appears
      • firstname appears TWICE with different values

      5. Repeat steps exporting to XLS and ODS.

      Show
      Test pre-requisites 4 user custom fields as follow: firstname, foo, bar, foobar User A is a teacher with capability course:viewhiddenuserfields User B is a teacher without capability course:viewhiddenuserfields Some students with custom fields filled (set firstname to something different than the user firstname) Set setting grade_export_userprofilefields to its default value, but remove 'email' and add 'password,idontexist' Set setting grade_export_customprofilefields to 'foo,bar,firstname' Set setting $CFG->hiddenuserfields to 'bar,lastname' in your config file Have a few grades on a few activities for the students Test 1 1. As user A go to the gradebook 2. Export the grades to plain text file 3. In the preview, make sure that: password, idontexist, email DO NOT appear foo, bar, lastname appear firstname appears TWICE with different values 4. Export the file, and make sure password, idontexist, email DO NOT appear foo, bar, lastname appear firstname appears TWICE with different values 5. Repeat steps exporting to XLS and ODS. Test 2 1. As user B go to the gradebook 2. Export the grades to plain text file 3. In the preview, make sure that: password, idontexist, email, lastname, bar DO NOT appear foo appears firstname appears TWICE with different values 4. Export the file, and make sure password, idontexist, email, lastname, bar DO NOT appear foo appears firstname appears TWICE with different values 5. Repeat steps exporting to XLS and ODS.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-21572-master
    • Rank:
      5818

      Description

      At present, GB allows teacher or admin to export data with grade items and following user related fields: firstname, surname, ID, institution, department and email. It would be useful to be able to export data from other profile fields including city/town and country. The default export contains optional profile fields, yet omits mandatory fields. thank you.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Assigning this to me. There is a forum thread discussing this at http://moodle.org/mod/forum/discuss.php?d=156230

          Show
          Andrew Davis added a comment - Assigning this to me. There is a forum thread discussing this at http://moodle.org/mod/forum/discuss.php?d=156230
          Hide
          Matt Ladwig added a comment -

          I would like about the same thing. For example. I would like to be able to use a field that I created to be exported as an excel doc from the gradebook.

          Show
          Matt Ladwig added a comment - I would like about the same thing. For example. I would like to be able to use a field that I created to be exported as an excel doc from the gradebook.
          Hide
          Charles Fulton added a comment -

          Hi Andrew, I've written a patch for 2.3 which makes these fields user-configurable. It's very similar to the patch proposed in MDL-17346 although I didn't discover that issue until I was 2/3 of the way through writing it. Could you please take a look?

          Show
          Charles Fulton added a comment - Hi Andrew, I've written a patch for 2.3 which makes these fields user-configurable. It's very similar to the patch proposed in MDL-17346 although I didn't discover that issue until I was 2/3 of the way through writing it. Could you please take a look?
          Hide
          Andrew Davis added a comment - - edited

          Hi Charles. I've had a look and it looks good. I've added a second commit which you can find at https://github.com/andyjdavis/moodle/compare/master...MDL-21572-master It just cleans up some white space and indenting stuff. No actual code changes.

          Some things we'll have to think about. I'm listing these here both for you and to remind me when I come back to this

          Should get_user_profile_fields() be in /grade/lib.php (where it is now) or in /grade/export/lib.php

          This code modifies the init() method of the graded_users_iterator class. That class is mostly just used by the grade export but it is used elsewhere. The user report (/grade/report/user) uses it from memory. Most likely contrib code somewhere out there also uses it. We'll have to make sure we don't break it or make it perform significantly worse for anywhere outside of the grade export that uses it.

          Update: one more thing. It would be highly desirable if the export works the same by default as it did before. Perhaps thats as simple as changing the default for "grade_export_userprofilefields". If anyone is exporting grades from Moodle then piping that data into a piece of custom software (or something) ideally their system will continue to work.

          Show
          Andrew Davis added a comment - - edited Hi Charles. I've had a look and it looks good. I've added a second commit which you can find at https://github.com/andyjdavis/moodle/compare/master...MDL-21572-master It just cleans up some white space and indenting stuff. No actual code changes. Some things we'll have to think about. I'm listing these here both for you and to remind me when I come back to this Should get_user_profile_fields() be in /grade/lib.php (where it is now) or in /grade/export/lib.php This code modifies the init() method of the graded_users_iterator class. That class is mostly just used by the grade export but it is used elsewhere. The user report (/grade/report/user) uses it from memory. Most likely contrib code somewhere out there also uses it. We'll have to make sure we don't break it or make it perform significantly worse for anywhere outside of the grade export that uses it. Update: one more thing. It would be highly desirable if the export works the same by default as it did before. Perhaps thats as simple as changing the default for "grade_export_userprofilefields". If anyone is exporting grades from Moodle then piping that data into a piece of custom software (or something) ideally their system will continue to work.
          Hide
          Charles Fulton added a comment -

          I've pulled that commit in, squashed and rebased. No code changes from me either. IIRC my rationale for putting get_user_profile_fields() there was to avoid having to change which libraries were included where. I know I considered (and tried) both locations you mentioned. For the settings the defaults are the same as the hardcoded ones so the change should be transparent to existing users.

          Show
          Charles Fulton added a comment - I've pulled that commit in, squashed and rebased. No code changes from me either. IIRC my rationale for putting get_user_profile_fields() there was to avoid having to change which libraries were included where. I know I considered (and tried) both locations you mentioned. For the settings the defaults are the same as the hardcoded ones so the change should be transparent to existing users.
          Hide
          Andrew Davis added a comment -

          I am attempting to estimate my future work so that I can manage my time better in future. My current estimate for this issue is another 4 hours of time to get this issue through integration and testing.

          Show
          Andrew Davis added a comment - I am attempting to estimate my future work so that I can manage my time better in future. My current estimate for this issue is another 4 hours of time to get this issue through integration and testing.
          Hide
          Andrew Davis added a comment -

          Added some testing instructions.

          Show
          Andrew Davis added a comment - Added some testing instructions.
          Hide
          Andrew Davis added a comment -

          Putting this up for peer review.

          Show
          Andrew Davis added a comment - Putting this up for peer review.
          Hide
          Frédéric Massart added a comment -

          Thanks Charles and Andrew,

          The code looks good, and apart from a few white spaces/identation everything looks fine (example grade/export/ods/grade_export_ods.php:49).

          My only concern is about the capability 'moodle/course:viewhiddenuserfields'. Shouldn't we check for it before allowing the current user to export those data?

          That'd also be nice to check if the user field exists before trying to get its value, that would eventually prevent those debugging errors:

          Invalid get_string() identifier: 'idontexist' or component 'moodle'. Perhaps you are missing $string['idontexist'] = ''; in lang/en/moodle.php?
          
              line 6592 of /lib/moodlelib.php: call to debugging()
              line 7216 of /lib/moodlelib.php: call to core_string_manager->get_string()
              line 2699 of /grade/lib.php: call to get_string()
              line 269 of /grade/export/lib.php: call to get_user_profile_fields()
              line 60 of /grade/export/txt/index.php: call to grade_export->display_preview()
          
          Notice: Undefined property: stdClass::$idontexist in /home/fred/www/repositories/stable_master/moodle/grade/export/lib.php on line 270 Call Stack: 0.0002 683904 1. {main}() /home/fred/www/repositories/stable_master/moodle/grade/export/txt/index.php:0 0.8381 93371896 2. grade_export->display_preview() /home/fred/www/repositories/stable_master/moodle/grade/export/txt/index.php:60 
          
          Show
          Frédéric Massart added a comment - Thanks Charles and Andrew, The code looks good, and apart from a few white spaces/identation everything looks fine (example grade/export/ods/grade_export_ods.php:49). My only concern is about the capability 'moodle/course:viewhiddenuserfields'. Shouldn't we check for it before allowing the current user to export those data? That'd also be nice to check if the user field exists before trying to get its value, that would eventually prevent those debugging errors: Invalid get_string() identifier: 'idontexist' or component 'moodle'. Perhaps you are missing $string['idontexist'] = ''; in lang/en/moodle.php? line 6592 of /lib/moodlelib.php: call to debugging() line 7216 of /lib/moodlelib.php: call to core_string_manager->get_string() line 2699 of /grade/lib.php: call to get_string() line 269 of /grade/export/lib.php: call to get_user_profile_fields() line 60 of /grade/export/txt/index.php: call to grade_export->display_preview() Notice: Undefined property: stdClass::$idontexist in /home/fred/www/repositories/stable_master/moodle/grade/export/lib.php on line 270 Call Stack: 0.0002 683904 1. {main}() /home/fred/www/repositories/stable_master/moodle/grade/export/txt/index.php:0 0.8381 93371896 2. grade_export->display_preview() /home/fred/www/repositories/stable_master/moodle/grade/export/txt/index.php:60
          Hide
          Charles Fulton added a comment -

          @Frederic; thanks for the review. I think I've dealt with the last of the whitespace.

          That's a good question about moodle/course:viewhiddenuserfields. I think you're probably right. None of the fields in the existing export can be hidden. I suppose there could be a use case where someone could export grades but couldn't access all the fields (seems unlikely, but hey). I think in that case the export would gracefully include whatever fields the user could see, and not give any warnings?

          Show
          Charles Fulton added a comment - @Frederic; thanks for the review. I think I've dealt with the last of the whitespace. That's a good question about moodle/course:viewhiddenuserfields. I think you're probably right. None of the fields in the existing export can be hidden. I suppose there could be a use case where someone could export grades but couldn't access all the fields (seems unlikely, but hey). I think in that case the export would gracefully include whatever fields the user could see, and not give any warnings?
          Hide
          Frédéric Massart added a comment -

          Yes it is unlikely, especially when the default values allow teachers/managers to see hidden fields, and do not specify any hidden fields. But, as the option is there I guess it is wise to use it.

          More information about that capability: http://docs.moodle.org/23/en/Capabilities/moodle/course:viewhiddenuserfields
          You can find use of it in /user/index.php:180 (stable master)

          Show
          Frédéric Massart added a comment - Yes it is unlikely, especially when the default values allow teachers/managers to see hidden fields, and do not specify any hidden fields. But, as the option is there I guess it is wise to use it. More information about that capability: http://docs.moodle.org/23/en/Capabilities/moodle/course:viewhiddenuserfields You can find use of it in /user/index.php:180 (stable master)
          Hide
          Frédéric Massart added a comment -

          Assigning this issue to me. I will pull Charles' patch and include the capability check and others. The idea being to have this issue done by the end of the sprint (Friday).

          Show
          Frédéric Massart added a comment - Assigning this issue to me. I will pull Charles' patch and include the capability check and others. The idea being to have this issue done by the end of the sprint (Friday).
          Hide
          Andrew Davis added a comment -

          As Frederic mentioned, I'm busy elsewhere so Frederic and I are switching places. He'll attend to these final issues and I'll take over as peer reviewer.

          Show
          Andrew Davis added a comment - As Frederic mentioned, I'm busy elsewhere so Frederic and I are switching places. He'll attend to these final issues and I'll take over as peer reviewer.
          Hide
          Frédéric Massart added a comment -

          There is my patch guys. I have added the capability checks, I would have used the generic function user_get_user_details() but I'm afraid that would badly affect performances.

          Original patch from Charles:
          git://github.com/mackensen/moodle.git MDL-21572-master
          https://github.com/mackensen/moodle/compare/MDL-21572-master

          Show
          Frédéric Massart added a comment - There is my patch guys. I have added the capability checks, I would have used the generic function user_get_user_details() but I'm afraid that would badly affect performances. Original patch from Charles: git://github.com/mackensen/moodle.git MDL-21572 -master https://github.com/mackensen/moodle/compare/MDL-21572-master
          Hide
          Andrew Davis added a comment -

          Hi. I'm not sure if your diff URL is correct. I can't see any capability check.

          Also, the graded user iterator is also used by the grader report. Are we now going to load information we don't need when the grader report loads? If that is the case perhaps we need to split out the loading of custom field information into a separate function? Given the choice, I suspect we'll be better off having an extra query during an export (an occasional action) rather than joining in extra tables every time the grader report loads (which happens many times a day).

          Show
          Andrew Davis added a comment - Hi. I'm not sure if your diff URL is correct. I can't see any capability check. Also, the graded user iterator is also used by the grader report. Are we now going to load information we don't need when the grader report loads? If that is the case perhaps we need to split out the loading of custom field information into a separate function? Given the choice, I suspect we'll be better off having an extra query during an export (an occasional action) rather than joining in extra tables every time the grader report loads (which happens many times a day).
          Hide
          Frédéric Massart added a comment -

          Thanks for your feedback Andrew,

          the capability check is done in grade_helper::get_user_profile_fields() line 2717.
          I did not notice that the grade_user_iterator class was used in the user report, I have added a parameter to prevent the custom fields to be included by default. I have reflected this change over the exports and updated the branch. Let me know what you think about it.

          Show
          Frédéric Massart added a comment - Thanks for your feedback Andrew, the capability check is done in grade_helper::get_user_profile_fields() line 2717. I did not notice that the grade_user_iterator class was used in the user report, I have added a parameter to prevent the custom fields to be included by default. I have reflected this change over the exports and updated the branch. Let me know what you think about it.
          Hide
          Andrew Davis added a comment -

          I'm sorry, I must be going blind. In what file at what line should I look to see the new parameter?

          Show
          Andrew Davis added a comment - I'm sorry, I must be going blind. In what file at what line should I look to see the new parameter?
          Hide
          Frédéric Massart added a comment -
          Show
          Frédéric Massart added a comment - No worries Andrew. I have commented on my commit, https://github.com/FMCorz/moodle/commit/935512fd5337d20a144f4f68903b46309748b203 .
          Hide
          Andrew Davis added a comment -

          Thanks I think we're ok to go for integration.

          Show
          Andrew Davis added a comment - Thanks I think we're ok to go for integration.
          Hide
          Frédéric Massart added a comment -

          Cool! Thanks Andrew!

          Show
          Frédéric Massart added a comment - Cool! Thanks Andrew!
          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
          Sam Hemelryk added a comment -

          Hi guys,

          I've been having a good look at this.
          Certainly the code is looking in really good shape now.
          I made the following notes during my review.

          graded_users_iterator::init

          • User profile fields can have defaults, it doesn't appear that they are being used if the user has not set a value.
            grade_helper::get_user_profile_fields
          • get_context_instance calls need to be upgraded to context_course:instance... etc
          • is there any need to include /user/profile/lib.php?
            grade_export
          • Despite that class not having good phpdocs I still think the new property and modified methods should have their phpdocs tidied up.

          Really the only thing that needs discussion before we proceed is the idea of field defaults.
          Unless I'd looked over something profile field defaults weren't being used when the user had not entered any data for the field.
          Too be truthful I'm not sure if that is something that has been missed, or whether it is something that was considered and a decision made.
          I'll leave it up to you to tell me.

          For the time being I've left this in integration review.
          If its being missed no probs I'll reopen this and you can fix that up in the next iteration, otherwise I'm keen to hear why it was left out. If there was a good reason then I see not reason this couldn't go in now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've been having a good look at this. Certainly the code is looking in really good shape now. I made the following notes during my review. graded_users_iterator::init User profile fields can have defaults, it doesn't appear that they are being used if the user has not set a value. grade_helper::get_user_profile_fields get_context_instance calls need to be upgraded to context_course:instance... etc is there any need to include /user/profile/lib.php? grade_export Despite that class not having good phpdocs I still think the new property and modified methods should have their phpdocs tidied up. Really the only thing that needs discussion before we proceed is the idea of field defaults. Unless I'd looked over something profile field defaults weren't being used when the user had not entered any data for the field. Too be truthful I'm not sure if that is something that has been missed, or whether it is something that was considered and a decision made. I'll leave it up to you to tell me. For the time being I've left this in integration review. If its being missed no probs I'll reopen this and you can fix that up in the next iteration, otherwise I'm keen to hear why it was left out. If there was a good reason then I see not reason this couldn't go in now. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          Hi Sam, thanks for your comments. I have already updated my branch, but feel free to put it back in dev if that's how it works with the integration process.

          • The default values are now considered, good thinking!
          • get_context_instance() has been replaced by context_course::instance()
          • /user/profile/lib.php is required to load a constant, I added a comment to make it clear
          • I added a few more phpdocs

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Sam, thanks for your comments. I have already updated my branch, but feel free to put it back in dev if that's how it works with the integration process. The default values are now considered, good thinking! get_context_instance() has been replaced by context_course::instance() /user/profile/lib.php is required to load a constant, I added a comment to make it clear I added a few more phpdocs Cheers!
          Hide
          Sam Hemelryk added a comment -

          Thanks for cleaning those points up.
          I've integrated this now, having looked over it once more this morning.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for cleaning those points up. I've integrated this now, having looked over it once more this morning. Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          This looks good.

          Thanks Fred.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This looks good. Thanks Fred. Test passed.
          Hide
          Dan Poltawski added a comment -

          *Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

          Congratulations

          {tracker.user.name}

          !

          You've made into Moodle

          {tracker.fixversion-1}

          +

          I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

          cheers!

          {tracker.friendlyintegrator}
          Show
          Dan Poltawski added a comment - * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}
          Hide
          Dan Poltawski added a comment -

          I've just noticed that the setting added here doesn't have a default value, and so is geting displayed on install: MDL-34603

          Show
          Dan Poltawski added a comment - I've just noticed that the setting added here doesn't have a default value, and so is geting displayed on install: MDL-34603
          Hide
          Helen Foster added a comment -

          Removing the docs_required label as this improvement is now documented:

          http://docs.moodle.org/en/Grade_export

          It's also mentioned in http://docs.moodle.org/dev/Moodle_2.4_release_notes#Miscellaneous

          Show
          Helen Foster added a comment - Removing the docs_required label as this improvement is now documented: http://docs.moodle.org/en/Grade_export It's also mentioned in http://docs.moodle.org/dev/Moodle_2.4_release_notes#Miscellaneous

            People

            • Votes:
              9 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: