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

Error accessing the SCORM Basic Report: "ORA-00918: column ambiguously defined"

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: SCORM
    • Labels:
    • Database:
      Oracle
    • Testing Instructions:
      Hide

      With Oracle (That means you, Moodle HQ, we should be testing oracle )

      1. Create a SCORM Activity
      2. Attend to it
      3. As a user with enough privileges click on both the Basic and Interactions reports
      4. Check that you can view the report and no sql error is seen

      Without Oracle (and as extra check with oracle): (Difficulty: developer, requires a code hack to look at the queries to overcome the lack of an Oracle based Moodle instance)

      1. Add $DB->set_debug(true); as described in https://moodle.org/mod/forum/discuss.php?d=222153#p966855
      2. Create a SCORM Activity
      3. Attend to it
      4. As a user with enough privileges click on both the Basic and Interactions reports
      5. Check that the offending SQL query has no more the doubled u.email
      Show
      With Oracle (That means you, Moodle HQ, we should be testing oracle ) Create a SCORM Activity Attend to it As a user with enough privileges click on both the Basic and Interactions reports Check that you can view the report and no sql error is seen Without Oracle (and as extra check with oracle): (Difficulty: developer, requires a code hack to look at the queries to overcome the lack of an Oracle based Moodle instance) Add $DB->set_debug(true); as described in https://moodle.org/mod/forum/discuss.php?d=222153#p966855 Create a SCORM Activity Attend to it As a user with enough privileges click on both the Basic and Interactions reports Check that the offending SQL query has no more the doubled u.email
    • Workaround:
      Hide

      Go to Site administration ► Users ► Permissions ► User policies and uncheck Email address in Show user identity (showuseridentity)

      Show
      Go to Site administration ► Users ► Permissions ► User policies and uncheck Email address in Show user identity ( showuseridentity )
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m25_MDL-37449_Reports_ORA-00918_column_ambiguously_defined

      Description

      mod/scorm/report.php?id=70

      Debug info: ORA-00918: column ambiguously defined
      SELECT *
      FROM (SELECT DISTINCT u.id || '#' || COALESCE(st.attempt, 0) AS uniqueid, st.scormid AS scormid, st.attempt AS attempt, u.id AS userid, u.idnumber, u.firstname, u.lastname, u.picture, u.imagealt, u.email, u.email FROM m_user u LEFT JOIN m_scorm_scoes_track st ON st.userid = u.id AND st.scormid = 1 WHERE u.id IN (:o_param14,:o_param15,:o_param16,:o_param17) AND (st.userid IS NOT NULL OR st.userid IS NULL) ORDER BY uniqueid)
      WHERE rownum <= :o_oracle_num_rows
      [array (
      'o_param14' => 23,
      'o_param15' => 22,
      'o_param16' => 25,
      'o_param17' => 24,
      'o_oracle_num_rows' => 20,
      )]
      Error code: dmlreadexception
      Stack trace:

      line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown
      line 274 of /lib/dml/oci_native_moodle_database.php: call to moodle_database->query_end()
      line 1101 of /lib/dml/oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
      line 345 of /mod/scorm/report/basic/report.php: call to oci_native_moodle_database->get_records_sql()
      line 85 of /mod/scorm/report.php: call to scorm_basic_report->display()

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            bouncing to Ankit in case he's able to take a look - Ankit I don't have Oracle installed so am unlikely to look at this anytime soon - if this isn't something you're able to look at either feel free to assign to moodle.com instead. thanks!

            Sergio - if you were able to suggest a patch for this it would help a lot - if not it may sit here in the tracker for a while.

            Show
            danmarsden Dan Marsden added a comment - bouncing to Ankit in case he's able to take a look - Ankit I don't have Oracle installed so am unlikely to look at this anytime soon - if this isn't something you're able to look at either feel free to assign to moodle.com instead. thanks! Sergio - if you were able to suggest a patch for this it would help a lot - if not it may sit here in the tracker for a while.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Dan,
            I'm just looking at the recent post in the Community.
            The issue is linked to the doubled u.email in the SQL query: I'm looking at the code to see the reason for that.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Dan, I'm just looking at the recent post in the Community. The issue is linked to the doubled u.email in the SQL query: I'm looking at the code to see the reason for that.
            Hide
            matteo Matteo Scaramuccia added a comment -

            A first patch proposal could be:

            $ git diff
            diff --git a/mod/scorm/report/basic/report.php b/mod/scorm/report/basic/report.php
            index 86dbbff..6a1efcb 100644
            --- a/mod/scorm/report/basic/report.php
            +++ b/mod/scorm/report/basic/report.php
            @@ -270,8 +270,8 @@ class scorm_basic_report extends scorm_default_report {
                                         // Construct the SQL
                         $select = 'SELECT DISTINCT '.$DB->sql_concat('u.id', '\'#\'', 'COALESCE(st.attempt, 0)').' AS uniqueid, ';
                         $select .= 'st.scormid AS scormid, st.attempt AS attempt, ' .
            -                    'u.id AS userid, u.idnumber, u.firstname, u.lastname, u.picture, u.imagealt, u.email' .
            -                    get_extra_user_fields_sql($coursecontext, 'u', '', array('idnumber')) . ' ';
            +                    'u.id AS userid, u.idnumber, u.firstname, u.lastname, u.picture, u.imagealt' .
            +                    get_extra_user_fields_sql($coursecontext, 'u', '', array('email', 'idnumber')) . ' ';
             
                         // This part is the same for all cases - join users and scorm_scoes_track tables
                         $from = 'FROM {user} u ';

            I'll ping the OP in the Community to let her be involved in testing and report back here.

            Show
            matteo Matteo Scaramuccia added a comment - A first patch proposal could be: $ git diff diff --git a/mod/scorm/report/basic/report.php b/mod/scorm/report/basic/report.php index 86dbbff..6a1efcb 100644 --- a/mod/scorm/report/basic/report.php +++ b/mod/scorm/report/basic/report.php @@ -270,8 +270,8 @@ class scorm_basic_report extends scorm_default_report { // Construct the SQL $select = 'SELECT DISTINCT '.$DB->sql_concat('u.id', '\'#\'', 'COALESCE(st.attempt, 0)').' AS uniqueid, '; $select .= 'st.scormid AS scormid, st.attempt AS attempt, ' . - 'u.id AS userid, u.idnumber, u.firstname, u.lastname, u.picture, u.imagealt, u.email' . - get_extra_user_fields_sql($coursecontext, 'u', '', array('idnumber')) . ' '; + 'u.id AS userid, u.idnumber, u.firstname, u.lastname, u.picture, u.imagealt' . + get_extra_user_fields_sql($coursecontext, 'u', '', array('email', 'idnumber')) . ' ';   // This part is the same for all cases - join users and scorm_scoes_track tables $from = 'FROM {user} u '; I'll ping the OP in the Community to let her be involved in testing and report back here.
            Hide
            matteo Matteo Scaramuccia added a comment -

            If the OP will confirm the solution, the same should be applied into interactions/report.php: I'll do some tests by my own in the next days looking at the SQL code generated regardless the database server - my dev env is based on LAMP.

            Show
            matteo Matteo Scaramuccia added a comment - If the OP will confirm the solution, the same should be applied into interactions/report.php : I'll do some tests by my own in the next days looking at the SQL code generated regardless the database server - my dev env is based on LAMP .
            Hide
            matteo Matteo Scaramuccia added a comment -

            My first patch proposal was wrong, details in https://moodle.org/mod/forum/discuss.php?d=222153#p966855.

            Show
            matteo Matteo Scaramuccia added a comment - My first patch proposal was wrong, details in https://moodle.org/mod/forum/discuss.php?d=222153#p966855 .
            Hide
            matteo Matteo Scaramuccia added a comment -

            Mia, the OP in the Community, confirmed that the proposal did the trick.
            @Dan: backport to 2.4 and 2.3 too?

            Show
            matteo Matteo Scaramuccia added a comment - Mia, the OP in the Community, confirmed that the proposal did the trick. @Dan: backport to 2.4 and 2.3 too?
            Hide
            danmarsden Dan Marsden added a comment -

            code looks good to me - +1 for 2.3, 2.4 and master - feel free to submit for integration when you're ready!

            Show
            danmarsden Dan Marsden added a comment - code looks good to me - +1 for 2.3, 2.4 and master - feel free to submit for integration when you're ready!
            Hide
            andreabix Andrea Bicciolo added a comment -

            Hi,

            just compared the patch we produced with the proposed one, differences are only on the order of two fields, that are not significant. Our +1 for integration review on 2.3,2.4 and Master.

            Show
            andreabix Andrea Bicciolo added a comment - Hi, just compared the patch we produced with the proposed one, differences are only on the order of two fields, that are not significant. Our +1 for integration review on 2.3,2.4 and Master.
            Hide
            matteo Matteo Scaramuccia added a comment -

            @Dan: OK, here they are 2.2 included
            @Andrea: I've decided to adopt the alphabetical order though $CFG->showuseridentity can be "idnumber,email,phone1,phone2,department,institution"

            Show
            matteo Matteo Scaramuccia added a comment - @Dan: OK, here they are 2.2 included @Andrea: I've decided to adopt the alphabetical order though $CFG->showuseridentity can be " idnumber,email,phone1,phone2,department,institution "
            Hide
            danmarsden Dan Marsden added a comment -

            looks good to me - pushing for integration.

            Show
            danmarsden Dan Marsden added a comment - looks good to me - pushing for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Matteo, integrated to master, 24 and 23. We only are backporting security fixes from 2.3 so its not there.

            cheers!
            dan

            Show
            poltawski Dan Poltawski added a comment - Thanks Matteo, integrated to master, 24 and 23. We only are backporting security fixes from 2.3 so its not there. cheers! dan
            Hide
            fred Frédéric Massart added a comment -

            Test passed on Oracle and PostgreSQL. Thanks!

            Show
            fred Frédéric Massart added a comment - Test passed on Oracle and PostgreSQL. Thanks!
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

            Regards, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Mar/13