Fix default order by SQL

[hdagheda]

Hi @FMCorz

When 'Where are points used? ' set to 'For the whole site', accessing /blocks/xp/index.php/report/1 page causes below dml issue. its due to default ordering. Please review PR and merge. Our clients are facing issues. Thank you, Heena

======================================== Debug info: ERROR: syntax error at or near "DESC" LINE 10: ORDER BY lvl DESC, DESC LIMIT 20 OFFSET 0 ^ SELECT u.id,u.picture,u.firstname,u.lastname,u.firstnamephonetic,u.lastnamephonetic,u.middlename,u.alternatename,u.imagealt,u.email, COALESCE(x.lvl, 1) AS lvl, x.xp, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance FROM mdl_user u JOIN mdl_context ctx ON ctx.instanceid = u.id AND ctx.contextlevel = $1 LEFT JOIN mdl_block_xp x ON (x.userid = u.id AND x.courseid = $2) WHERE u.id IS NULL ORDER BY lvl DESC, DESC LIMIT 20 OFFSET 0 [array ( 0 => 30, 1 => 1, )] Error code: dmlreadexception

[FMCorz]

Hi @hdagheda,

Would you please provide replication steps for this? We cannot reproduce this issue, our block is set site-wide, and there are students displayed in the report. Please also confirm which version of the block and Moodle you are using.

Regards, Fred

[hdagheda]

Hi @FMCorz ,

Thank you so much for getting back quickly.

We are using moodle $release = '3.3.9 (Build: 20181112)'; and $TOTARA->version = '12.19'; with pgsql.

There are no report set.. once plugin is installed... directly access /blocks/xp/index.php/report/1 page.

Regards, Heena

[FMCorz]

Hi @hdagheda,

This may be specific to Totara. We can't see why this would happen, the method that you changed is based on the core one that returns an array indexed by column keys. After the patch, the array is no longer indexed. The error seems to indicate that some keys of the orderby were not set, and which lead to the SQL error.

If you could provide more information about the content of the orderby variable as we enter and exit the method get_sort_columns, perhaps we'd be able to track this down. If you have made customisations to tablelib this may be the cause of these issues as well.

[hdagheda]

Hi @FMCorz

There hasnt been any customization. Please find content of orderby variable

Enter:
Array
(
    [0] => Array
        (
            [lvl] => 3
        )

)
Exit:
Array
(
    [0] => Array
        (
            [lvl] => 3
        )

    [id] => 4
)

[hdagheda]

After fixing

Enter:
Array
(
    [0] => Array
        (
            [lvl] => 3
        )

)

Exit:
Array
(
    [0] => Array
        (
            [lvl] => 3
        )

    [1] => Array
        (
            [id] => 4
        )

)

[FMCorz]

This is what I get:

/blocks/xp/classes/output/report_table.php:320:
array (size=1)
  'lvl' => int 3

/blocks/xp/classes/output/report_table.php:336:
array (size=3)
  'lvl' => int 3
  'xp' => int 3
  'id' => int 4

Notice how yours is numerically indexed? We do not have access to Totara, so we cannot debug it, but I expect that's where the problem is from. If you can, would you please provide the content of tablelib.php#get_sort_columns and tablelib.php#construct_order_by? Alternatively, are there public release notes from Totara that would outline this sort of change?

[hdagheda]

Please find as below:

/**
     * Get the columns to sort by, in the form required by {@link construct_order_by()}.
     * @return array column name => SORT_... constant.
     */
    public function get_sort_columns() {
        global $SESSION;

        $result = array();

        if (!$this->setup) {
            throw new coding_exception('Cannot call get_sort_columns until you have called setup.');
        }

        if (empty($this->prefs['sortby'])) {
            return array();
        }

        foreach ($this->prefs['sortby'] as $sortbykey => $direction) {
            // The first letter of the sortbykey indicates the source of the data.
            // 'c' means a column in the data, 'd' means a default sort key that's
            // not a displayed column.
            $indextype = substr($sortbykey, 0, 1);
            if ($indextype == 'c') {
                $column = $this->columns[substr($sortbykey, 1)];
                $result[] = array($column => $direction);
                continue; // This column is OK.
            }

            $column = substr($sortbykey, 1);
            if (in_array($column, get_all_user_name_fields()) && isset($this->columns['fullname'])) {
                $result[] = array($column => $direction);
            }

            // If the column hasn't been dealt with by the above conditions, it's not ok. Ignore it.
        }

        return $result;
    }

/**
     * Prepare an an order by clause from the list of columns to be sorted.
     * @param array $cols column name => SORT_ASC or SORT_DESC
     * @return SQL fragment that can be used in an ORDER BY clause.
     */
    public static function construct_order_by($cols, $textsortcols=array()) {
        global $DB;
        $bits = array();

        foreach ($cols as $detail) {
            $column = key($detail);
            $order = current($detail);
            if (in_array($column, $textsortcols)) {
                $column = $DB->sql_order_by_text($column);
            }
            if ($order == SORT_ASC) {
                $bits[] = $column . ' ASC';
            } else {
                $bits[] = $column . ' DESC';
            }
        }

        return implode(', ', $bits);
    }

[FMCorz]

Hi @hdagheda,

As you can tell, Totara has altered the behaviour of these methods. We suggest that you raise this issue with them as it could/would affect the compatibility with other Moodle plugins. We will think about patching Level up!, however without access to Totara we cannot easily test/fix these kind of issues. Moreover, the more Totara distances itself from Moodle, the more difficult it becomes for us to support it.

Latest Moodle code for reference: https://github.com/moodle/moodle/blob/master/lib/tablelib.php#L569

[Peterburnett]

Hi all,

After a little bit of internal discussion with @hdagheda , we will attempt to find a cross compatible patch, that will patch this particular issue, and upstream it back here. I agree that divergences with Totara are difficult to support (especially if you don't have totara access), and it would be nice to help resolve these divergences as they arrive.

Thanks

[hdagheda]

Thank you so much for your time @FMCorz

[FMCorz]

Thank you, closing for now.