Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-22773

Include user details in records exported from Database activity

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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:

      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.

        Gliffy Diagrams

        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
            stronk7 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
            stronk7 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
            daveyboond Steve Bond added a comment -

            Thanks for the tip Eloy

            Show
            daveyboond Steve Bond added a comment - Thanks for the tip Eloy
            Hide
            bostelm 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
            bostelm 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
            bostelm 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
            bostelm 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
            phalacee Jason Fowler added a comment -

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

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

            reopening this for Henning to look at.

            Show
            nebgor Aparup Banerjee added a comment - reopening this for Henning to look at.
            Hide
            bostelm 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
            bostelm 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
            daveyboond 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
            daveyboond 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
            phalacee Jason Fowler added a comment -

            Looks good to me Henning

            Show
            phalacee Jason Fowler added a comment - Looks good to me Henning
            Hide
            bostelm 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
            bostelm 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
            nebgor 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
            nebgor 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
            bostelm Henning Bostelmann added a comment -

            Hi Aparup,

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

            Show
            bostelm Henning Bostelmann added a comment - Hi Aparup, now changed as requested - in a way that is (hopefully) backward compatible.
            Hide
            nebgor 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
            nebgor 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_frenz Ankit Agarwal added a comment -

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

            Show
            ankit_frenz Ankit Agarwal added a comment - There is no option to "Include approval status"... attaching screen
            Hide
            ankit_frenz 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_frenz 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  3/Dec/12