Moodle
  1. Moodle
  2. MDL-22773

Include user details in records exported from Database activity

    Details

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

      In a course with at least one student enrolled, log in as teacher or admin and do the following:

      • Create a database activity with approval activated
      • Add one text field ("Test Text")
      • Add the standard list templates
      • Create one database entry
      • Log in as the student, create another database entry
      • Log back in as teacher, open the database export screen
      • Select all four boxes "Test Text", "Include user details", "Include time added/modified", "Include approval status"
      • Click "Export"

      VERIFY: Output data contains columns as follows:

      • Test Text
      • User (filled with user fullname)
      • Username
      • Email
      • Time added
      • Time modified
      • Approval (must be 0 for the student's entry and 1 for the teacher's entry)

      Do the above export steps for CSV format as well as ODS format.

      Show
      In a course with at least one student enrolled, log in as teacher or admin and do the following: Create a database activity with approval activated Add one text field ("Test Text") Add the standard list templates Create one database entry Log in as the student, create another database entry Log back in as teacher, open the database export screen Select all four boxes "Test Text", "Include user details", "Include time added/modified", "Include approval status" Click "Export" VERIFY: Output data contains columns as follows: Test Text User (filled with user fullname) Username Email Time added Time modified Approval (must be 0 for the student's entry and 1 for the teacher's entry) Do the above export steps for CSV format as well as ODS format.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      6079

      Description

      When records are exported from the Database, only the user-defined fields are exported. User details that may be shown in 'View list' mode using ##user## are not exported.

      These patches add an option to the export page to include the user details (full name and email address) as the first two fields in the output file.

      1. data.php.patch
        0.8 kB
        Steve Bond
      2. export_form.php.patch
        1 kB
        Steve Bond
      3. export.php.patch
        2 kB
        Steve Bond
      1. database.png
        45 kB

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks interesting, just need to be considered together with MDL-16593 to decide if user name and mail is enough or we want more fields to be selected and exported (dates... whatever).

          Note: I'd perform one $userrec = get_record('user') and the fullname() function instead of current implementation in patch.

          Show
          Eloy Lafuente (stronk7) added a comment - Looks interesting, just need to be considered together with MDL-16593 to decide if user name and mail is enough or we want more fields to be selected and exported (dates... whatever). Note: I'd perform one $userrec = get_record('user') and the fullname() function instead of current implementation in patch.
          Hide
          Steve Bond added a comment -

          Thanks for the tip Eloy

          Show
          Steve Bond added a comment - Thanks for the tip Eloy
          Hide
          Henning Bostelmann added a comment -

          Patch ported to 2.x, added date/time information and approval status in the same way.

          This should fulfill the requirements, although a more sophisticated solution would certainly be possible.

          Show
          Henning Bostelmann added a comment - Patch ported to 2.x, added date/time information and approval status in the same way. This should fulfill the requirements, although a more sophisticated solution would certainly be possible.
          Hide
          Henning Bostelmann added a comment -

          Would be great if someone could have a look at the patch and say whether it can be integrated.

          Show
          Henning Bostelmann added a comment - Would be great if someone could have a look at the patch and say whether it can be integrated.
          Hide
          Jason Fowler added a comment -

          Code looks good to me. Sorry it took so long to peer review

          Show
          Jason Fowler added a comment - Code looks good to me. Sorry it took so long to peer review
          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
          Aparup Banerjee added a comment -

          Hi Henning,
          Petr has pointed out that there needs to be a finer control of capability checks here that regulate who can take away the sensitive private data.

          Perhaps implementing a new 'mod/data:exportuserinfo' capability with RISK_PERSONAL in the riskbitmask would help here. (similar to 'moodle/backup:userinfo' )

          the rest of the patch looks good to me

          Show
          Aparup Banerjee added a comment - Hi Henning, Petr has pointed out that there needs to be a finer control of capability checks here that regulate who can take away the sensitive private data. Perhaps implementing a new 'mod/data:exportuserinfo' capability with RISK_PERSONAL in the riskbitmask would help here. (similar to 'moodle/backup:userinfo' ) the rest of the patch looks good to me
          Hide
          Aparup Banerjee added a comment -

          reopening this for Henning to look at.

          Show
          Aparup Banerjee added a comment - reopening this for Henning to look at.
          Hide
          Henning Bostelmann added a comment -

          Hi Aparup and Steve,

          after some time of inactivity, I have now amended the patch. Exporting user details now needs the extra capability 'mod/data:exportuserinfo'. Two remarks on the capabilities:

          1) 'mod/data:exportuserinfo' has the risk bit RISK_PERSONAL set. But so have 'mod/data:exportentry' and 'mod/data:exportallentries'.

          2) It seems that access to the "Export" facility is not controlled by 'mod/data:exportallentries' but rather by 'mod/data:viewalluserpresets'. See line 58 in mod/data/export.php and line 34 in mod/data/lib.php. This may be a bug, but it is unrelated, so I didn't try to fix it here.

          The fix is for master at the moment. I would appreciate if it could be backported to 2.3; it should be a simple cherry-pick, but I'm not sure whether that's OK with the release policy since a capability was added.

          Cheers
          Henning

          Show
          Henning Bostelmann added a comment - Hi Aparup and Steve, after some time of inactivity, I have now amended the patch. Exporting user details now needs the extra capability 'mod/data:exportuserinfo'. Two remarks on the capabilities: 1) 'mod/data:exportuserinfo' has the risk bit RISK_PERSONAL set. But so have 'mod/data:exportentry' and 'mod/data:exportallentries'. 2) It seems that access to the "Export" facility is not controlled by 'mod/data:exportallentries' but rather by 'mod/data:viewalluserpresets'. See line 58 in mod/data/export.php and line 34 in mod/data/lib.php. This may be a bug, but it is unrelated, so I didn't try to fix it here. The fix is for master at the moment. I would appreciate if it could be backported to 2.3; it should be a simple cherry-pick, but I'm not sure whether that's OK with the release policy since a capability was added. Cheers Henning
          Hide
          Steve Bond added a comment -

          Excellent, thanks Henning. We're going to 2.3 in late Sep I think so it'd great if this made it into the release by then.

          Show
          Steve Bond added a comment - Excellent, thanks Henning. We're going to 2.3 in late Sep I think so it'd great if this made it into the release by then.
          Hide
          Jason Fowler added a comment -

          Looks good to me Henning

          Show
          Jason Fowler added a comment - Looks good to me Henning
          Hide
          Henning Bostelmann added a comment -

          Thanks for reviewing - can someone send the patch to integration review, then? Unfortunately, I don't have access to that button.

          Show
          Henning Bostelmann added a comment - Thanks for reviewing - can someone send the patch to integration review, then? Unfortunately, I don't have access to that button.
          Hide
          Aparup Banerjee added a comment -

          Hi Henning,
          I've noticed that the capability check on 'mod/data:exportuserinfo' is done in export.php, perhaps this would be better done within lib.php:data_get_exportdata() as the capability seems to apply more centrally to that function (even better cap checks for when others call that function).

          $userdetails = $userdetails && has_capability('mod/data:exportuserinfo', $context);
          

          since this is for master atm : perhaps we could update the function by passing in context?

          Show
          Aparup Banerjee added a comment - Hi Henning, I've noticed that the capability check on 'mod/data:exportuserinfo' is done in export.php, perhaps this would be better done within lib.php:data_get_exportdata() as the capability seems to apply more centrally to that function (even better cap checks for when others call that function). $userdetails = $userdetails && has_capability('mod/data:exportuserinfo', $context); since this is for master atm : perhaps we could update the function by passing in context?
          Hide
          Henning Bostelmann added a comment -

          Hi Aparup,

          now changed as requested - in a way that is (hopefully) backward compatible.

          Show
          Henning Bostelmann added a comment - Hi Aparup, now changed as requested - in a way that is (hopefully) backward compatible.
          Hide
          Aparup Banerjee added a comment -

          Thanks Henning, that seems to do it

          I've integrated that into master only as there is an API change here (tagged as so). I've also tweaked minor whitespaces.

          Note that this was affecting 2.2+ and 2.3+ as well but can't be backported due to it being an improvement + api changes.

          If there's further need to backport please open a separate issue.

          Show
          Aparup Banerjee added a comment - Thanks Henning, that seems to do it I've integrated that into master only as there is an API change here (tagged as so). I've also tweaked minor whitespaces. Note that this was affecting 2.2+ and 2.3+ as well but can't be backported due to it being an improvement + api changes. If there's further need to backport please open a separate issue.
          Hide
          Ankit Agarwal added a comment -

          There is no option to "Include approval status"... attaching screen

          Show
          Ankit Agarwal added a comment - There is no option to "Include approval status"... attaching screen
          Hide
          Ankit Agarwal added a comment -

          Require approval was set "no" in my settings...once I set that to "yes" I can see the checkbox and all things works as expected.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - Require approval was set "no" in my settings...once I set that to "yes" I can see the checkbox and all things works as expected. Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: