Moodle
  1. Moodle
  2. MDL-37449

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

    Details

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

          Activity

          Hide
          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
          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 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 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 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 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 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 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 Scaramuccia added a comment -

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

          Show
          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 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 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
          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
          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
          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
          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 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 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
          Dan Marsden added a comment -

          looks good to me - pushing for integration.

          Show
          Dan Marsden added a comment - looks good to me - pushing for integration.
          Hide
          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
          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
          Frédéric Massart added a comment -

          Test passed on Oracle and PostgreSQL. Thanks!

          Show
          Frédéric Massart added a comment - Test passed on Oracle and PostgreSQL. Thanks!
          Hide
          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 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: