itamart / moodle-block_mhaairs Goto Github PK

From Vadim:
Authorization connector has “identity_type” property, which is not being treated in plugin (GetUserInfo entry point). If its value is “internal”, the plugin should assume the user id being passed is an internal user id, otherwise – user name. Similar thing exists in gradebook for reference, except that gradebook also allows “lti” value.

Hello,

Does this plugin support PHP 7.2+? I'm asking because of the use of Zend Framework.

Trying to upgrade plugin version, and issuing any gradebook call.

Attempting to use this block (in Moodle 3.9) with PHP 7.4 throws this deprecation warning:

Deprecated: Array and string offset access syntax with curly braces is deprecated in /var/moodle/blocks/mhaairs/block_mhaairs_util.php on line 273

It appears that this deprecation is new as of PHP 7.4 and the syntax will removed in a future PHP release: PHP 8 was listed as a possibility.

In settings.php there is a call to $PAGE->url->compare that serves as a condition to skip adding the 'available services' setting in any page that is not the admin/settings page of the block, in particular the admin/upgradesettings page that is loaded on install or upgrade. This is because on install the available service config setting would be displayed as a new setting but since there would not be any service the setting could not be set and the install would not be able to proceed.

However calling $PAGE->url->compare without setting the url first generates an error message when initializing the moodle server, e.g. on initialization of the phpunit testing environment.

A more appropriate way to achieve the same effect is by overriding the admin_setting_configmulticheckbox::get_setting() method in the specialized admin_setting_configmulticheckbox_mhaairs such that it returns array() rather than null if the setting is null (which would be the case when there are no choices). This would allow the install/upgrade to continue without trying to set the available services setting.

Add unit tests for MHUtil and for the gradebook service.

I've found this error during unit tests execution:

There was 1 error:

1) block_mhaairs_gradebookservice_update_item_testcase::test_update_grade_update_item
dml_read_exception: Error reading from database (ERROR:  invalid input syntax for integer: ""
SELECT * FROM phpu_grade_items WHERE  itemtype = $1 AND itemmodule = $2 AND iteminstance = $3 AND courseid = $4 AND itemnumber = $5 
[array (
  0 => 'manual',
  1 => 'mhaairs',
  2 => '',
  3 => '121000',
  4 => '0',
)])

/var/www/vhost_swsi/swsi/lib/dml/moodle_database.php:474
/var/www/vhost_swsi/swsi/lib/dml/pgsql_native_moodle_database.php:244
/var/www/vhost_swsi/swsi/lib/dml/pgsql_native_moodle_database.php:753
/var/www/vhost_swsi/swsi/lib/dml/moodle_database.php:1250
/var/www/vhost_swsi/swsi/lib/grade/grade_object.php:220
/var/www/vhost_swsi/swsi/lib/grade/grade_object.php:160
/var/www/vhost_swsi/swsi/lib/grade/grade_item.php:354
/var/www/vhost_swsi/swsi/blocks/mhaairs/tests/gradebookservice_update_item_test.php:222
/var/www/vhost_swsi/swsi/lib/phpunit/classes/advanced_testcase.php:80

To re-run:
 ./vendor/bin/phpunit block_mhaairs_gradebookservice_update_item_testcase blocks/mhaairs/tests/gradebookservice_update_item_test.php

FAILURES!
Tests: 3, Assertions: 30, Errors: 1.

As a quick fix I just added casting strings to integers in one place. Please see attached patch.

Updated code should say (on lines 513)

        $origenc = '';
        if(defined('PHP_MAJOR_VERSION') && PHP_MAJOR_VERSION >= 5) {
            $origenc = mb_internal_encoding();
            mb_internal_encoding('UTF-8');
        } else {
            $origenc = iconv_get_encoding('internal_encoding');
            iconv_set_encoding('internal_encoding', 'UTF-8');
        }

and on lines (now) 617

        if(defined('PHP_MAJOR_VERSION') && PHP_MAJOR_VERSION >= 5) {
            mb_internal_encoding($origenc);
    } else {
            iconv_set_encoding('internal_encoding', $origenc);
        } 

This framework is deprecated and should not have been included in this plugin.

For testing purposes it is required to check if a grade item exists and to retrieve the grade item details. This should be done via a web service.

Moodle core provides the web service core_grades_get_grades since 2.7. The service returns student course total grade and grades for activities. If no users are specified the function returns the grade items info (category not included).

If we need this service for all supported versions, or if we need to include category, we can add to the plugin a web service get_grade to check for existence the way the plugin's update_grade (aka gradebookservice) currently does. This should work for testing since in testing we know the complete details of the grade item we are looking for.

block_mhaairs_action provides entry point to retrieve user info and validate login. These functions are effectively web services and can be wrapped as such. Two new services will be added:

• get_user_info
• validate login

This enhancement will also include the following:

• Fixing incorrect type in the gradebookservice service function definition (the type should be write as it updates the moodle database).
• Renaming the gradebookservice function name to update_grade to align it with the moodle convention for web service names. The current service function will remain available for backwards compatibility.
• Cosmetic fix of an error message in MHUtil::get_user_info().

The plugin currently contains testclient_form for the Moodle core test client, but this form does not work because it does not have input fields for item details and grades. While this form may be fixed it may be better to implement the an internal test client for the plugin to have better control over input and output.

• Protocol - fixed to REST
• Response format - XML|JSON (default to JSON).
• Auth - select either Password/Username or Token.
• Service params
  • Source of grade - string (default: mhaairs)
  • Course id - string (default: empty)
  • Item type - string (default: mod)
  • Item module - string (default: assignment)
  • Item instance - string (default: 0)
  • Item number - string (default: 0)
• Item details (null if not enabled)
  • Category id - string (default: empty)
  • Course id - string (default: empty)
  • Identify type - string (default: empty)
  • Item name - string (default: empty)
  • Item type - string (default: mod)
  • Id number - string (default: 0)
  • Grade type - string (default: 1)
  • Grade max - string (default: 100)
  • Item info - string (default: empty)
  • Deleted - bool (default: false)
• Grades (null if not enabled)
  • User id - string (default: empty)
  • Grade - string (default: empty)

From Vadim:
There is a GetServerTime entry point, which returns the local timestamp from server. That time is supposed to be sent back as part of the token to GetUserInfo, to avoid time zone problems between servers. Currently this doesn’t work, because the GetServerTime returns time in UTC, and this time is being compared to the local time in mh_is_token_valid function. It’s a silly bug that needs to be straightened out.

The util api in block_mhaairs_util.php currently consists of 2 classs, MHUserInfo and MHAuthenticationResult and a set of functions. All the "free" functions can be wrapped with an MHUtil class to normalize the util API. All the mh_ functions will be defined as public static. Usage will change from

mh_some_function()

to

MHUtil::some_function()

The actual behavior remains the same.

Plugin calls to these util functions should be adjusted accordingly. This touches:

• block_mhaairs.php
• block_mhaairs_action.php
• block_mhaairs_info.php
• redirect.php

Thank you for building this block/plugin. It looks to be what I've been looking for.

However, when I installed it on my Moodle instances, the LTI outcomes grade transfer broke. The Moodle LTI outcomes service endpoint (/mod/lti/service.php) returns a 404. I haven't had time to investigate yet, but wanted to open a ticket in case you may know right away what the cause may be.

Thank you,
Mike

In the get_sso_url function in block_mhaairs_util.php, there is this snipped of code to determine if a user is a teacher or a student:

        // Normalize the user's role name to insructor or student.
        if ($roles = get_user_roles($context, $USER->id)) {
            foreach ($roles as $role) {
                $rolename = empty($role->name) ? $role->shortname : $role->name;
                if ($rolename == 'teacher' || $rolename == 'editingteacher') {
                    $rolename = 'instructor';
                    break;
                }
            }
            if ($rolename != null && $rolename != 'instructor') {
                $rolename = 'student';
            }
        }

I'm managing a site where the editingteacher shortname got switched to 'faculty', so now teachers are showing up as students on McGraw Hill's side. Wouldn't it make more sense for this to check if they have the 'block/mhaairs:viewteacherdoc' permission instead (or make a new permission and add it to the teacher archetype)?