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 2.4 Branch:
      m24_MDL-37449_Reports_ORA-00918_column_ambiguously_defined
    • Pull Master Branch:
      m25_MDL-37449_Reports_ORA-00918_column_ambiguously_defined
    • Rank:
      47083

      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()

        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: