Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client tests #28

Closed
wants to merge 10 commits into from
4 changes: 4 additions & 0 deletions classes/client_helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,8 @@ public static function get_client(

return new $classname($tokenurl, $server, $clientid, $clientsecret);
}

public static function get_csv_client(): client_interface {
return new \enrol_oneroster\local\csv_client();
}
}
3 changes: 3 additions & 0 deletions classes/local/collection.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ public function refresh_data(): Iterable {
$this->get_filter(),
$this->get_params(),
function($data) {
if (!($data instanceof stdClass)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be needed, execute should ensure it always returns a StdClass

$data = (object) $data;
}
$data = static::parse_returned_row($this->container, $data);
if ($this->recordfilter && !call_user_func($this->recordfilter, $data)) {
return null;
Expand Down
265 changes: 265 additions & 0 deletions classes/local/csv_client.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* One Roster Client.
*
* This plugin synchronises enrolment and roles with a One Roster endpoint.
*
* @package enrol_oneroster
* @copyright Andrew Nicols <[email protected]>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright here needs updating. Copyright should generally be all 4 of you folks

* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/

namespace enrol_oneroster\local;

defined('MOODLE_INTERNAL') || die;

require_once($CFG->dirroot . '/lib/oauthlib.php');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this require needed? If not, this and the defined("MOODLE_INTERNAL") check can be removed, as this class inclusion will have no side effects on its own


use enrol_oneroster\local\interfaces\client as client_interface;
use enrol_oneroster\local\oneroster_client as root_oneroster_client;
use enrol_oneroster\local\command;
use enrol_oneroster\local\interfaces\filter;
use stdClass;
use enrol_oneroster\local\v1p1\oneroster_client as versioned_client;
use enrol_oneroster\local\interfaces\rostering_client;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double newline


/**
* One Roster CSV Client.
*
* @package enrol_oneroster
* @copyright Andrew Nicols <[email protected]>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dont need class level PHPDoc if there is only 1 class in the file

* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class csv_client implements client_interface {
use root_oneroster_client;
use versioned_client;

private $data;
private int $count;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go


/**
* Authenticate the client. This is a no-op for the CSV client.
*
* @return void
*/
public function authenticate(): void {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is required but is it worth refactoring the interface to make this optional?

return;
}


public function set_data($manifest, $users, $classes, $orgs, $enrollments, $academicSessions) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use type hinting and return hinting on non interface methods

$this->data = [
'manifest' => $manifest, // Manifest data
'users' => $users, // Users data
'classes' => $classes, // Classes data
'orgs' => $orgs, // Orgs data
'enrollments' => $enrollments, // Enrollments data
'academicSessions' => $academicSessions, // Academic sessions data
];
}


/**
* Execute the supplied command.
*
* @param command $command The command to execute
* @param filter $filter
* @return stdClass
*/
public function execute(command $command, ?filter $filter = null): stdClass {
$url = $command->get_url('');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment like: Split url like org/123 into components to key them into arrays

$basepath = explode('/', $url)[1];
$tokens = explode('/', $url);
$basepath = $tokens[1];
$param = $tokens[2] ?? '';
$type = $tokens[3] ?? '';

$orgId = 'org-sch-222-456';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove hardcoded value



switch ($basepath):
case 'orgs':
/** The endpoint getOrg is called to fetch the org data */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt need to be a block comment, can just be //

if ($param == $orgId || $param == '') {
$orgdata = $this->data['orgs'];

$keys = array_map(function($keys) { return $keys['sourcedId']; }, $orgdata);
$mapped_data = array_combine($keys, $orgdata);

if (isset($mapped_data[$orgId])) {
$org = (object) $mapped_data[$orgId];
$org->status = 'active';
$org->dateLastModified = date('Y-m-d H:i:s');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be converted to unix timestamp?

}

return (object) [
'response' => (object) [
'org' => (object) $org
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$org shouldnt be object casted

]
];
}

case 'schools':
/** The endpoint getTermsForSchool is called to fetch a list of classes hkeysd in a term */
if ($type == 'terms') {
$academicsessiondata = $this->data['academicSessions'];
$keys = array_map(function($keys) { return $keys['sourcedId']; }, $academicsessiondata);
$mapped_data = array_combine($keys, $academicsessiondata);

$academicSession = [];
foreach ($mapped_data as $academicId => $academicdata) {
$academic = (object) $academicdata;
$academic->parent = isset($academicdata['parentSourcedId']) ? (object) [
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ternary is a bit gnarly. Might be easier to move each part of into a var, then just ternary on the vars themselves.

eg
$1 = long things
$2 = long things

$final = condition ? $1 : $2;

'sourcedId' => $academicdata['parentSourcedId']
] : [null];
$academic->children = isset($academicdata['sourcedId']) ? (object) [
'sourcedId' => $academicdata['sourcedId']
] : [null];

unset($academic->parentSourcedId);

$academicSession[$academicId] = $academic;
}
return (object) [
'response' => (object) [
'academicSessions' => $academicSession,
'terms' => $academicSession
]
];
}


if ($type == 'classes') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types should probably be a set of constants at the top of the class

/** The endpoint getClassesForSchool is called to fetch all students for a class */
$classdata = $this->data['classes'];

$keys = array_map(function($keys) { return $keys['sourcedId']; }, $classdata);
$mapped_data = array_combine($keys, $classdata);

$classes = [];

foreach ($mapped_data as $classId => $classData) {
$class = (object) $classData;
if (isset($class->schoolSourcedId) && $class->schoolSourcedId == $orgId) {
$class->school = (object) [ 'sourcedId' => $class->schoolSourcedId ];

$class->course = isset($class->courseSourcedId) ? (object) [ 'sourcedId' => $class->courseSourcedId ] : null;

$class->terms = isset($class->termSourcedIds) ? array_map(function($term) {
return (object) ['sourcedId' => $term];
}, (array) $class->termSourcedIds) : [null];

$class->subject = isset($class->subjects) ? array_map(function($subject) {
return (object) ['subject' => $subject];
}, (array) $class->subjects) : [null];

$class->period = isset($class->periods) ? array_map(function($period) {
return (object) ['period' => $period];
}, (array) $class->periods) : [null];

unset($class->schoolSourcedId);
unset($class->courseSourcedId);
unset($class->termSourcedIds);
unset($class->subjects);
unset($class->periods);
}
$classes[$classId] = $class;
}
return (object) [
'response' => (object) [
'classes' => $classes
]
];
}

if ($type == 'enrollments') {
/** The endpoint getEnrollmentsForSchool is called to fetch all enrolments in a school */
$enrollmentdata = $this->data['enrollments'];
$keys = array_map(function($keys) { return $keys['sourcedId']; }, $enrollmentdata);
$mapped_data = array_combine($keys, $enrollmentdata);

$enrollments = [];

foreach ($mapped_data as $enrollmentId => $enrollmentData) {
$enrollment = (object) $enrollmentData;
if (isset($enrollment->schoolSourcedId) && $enrollment->schoolSourcedId == $orgId) {

$enrollment->user = isset($enrollmentData['userSourcedId']) ? (object) [
'sourcedId' => $enrollmentData['userSourcedId']] : null;

$enrollment->school = (object) [
'sourcedId' => $enrollmentData['schoolSourcedId']];

$enrollment->class = isset($enrollmentData['classSourcedId']) ? (is_array($enrollmentData['classSourcedId']) ? array_map(function($classSourcedId) {
return (object) ['sourcedId' => $classSourcedId]; }, $enrollmentData['classSourcedId']) : [(object) ['sourcedId' => $enrollmentData['classSourcedId']]]) : [null];

unset($class->schoolSourcedId);
unset($class->classSourcedId);
}
$enrollments[$enrollmentId] = $enrollment;
}
return (object) [
'response' => (object) [
'enrollments' => $enrollments
]
];
}


case 'users':
/** The endpoint GetAllUsers is called to fetch all users */
$usersData = $this->data['users'];

$keys = array_map(function($user) {return $user['sourcedId']; }, $usersData);
$mapped_data = array_combine($keys, $usersData);

$users = [];
foreach ($mapped_data as $userId => $userData) {
$user = (object) $userData;

if (isset($user->orgSourcedIds) && in_array($orgId, (array) $user->orgSourcedIds)) {

$user->orgs = array_map(function($org) {
return (object) ['sourcedId' => $org];
}, is_array($userData['orgSourcedIds']) ? $userData['orgSourcedIds'] : [$userData['orgSourcedIds']]);

$user->agents = isset($userData['agentSourcedIds']) ? array_map(function($agent) {
return (object) ['sourcedId' => $agent];
}, (array) $userData['agentSourcedIds']) : [null];

$user->userIds = isset($userData['userIds']) ? array_map(function($userId) {
return (object) ['type' => $userId['type'], 'identifier' => $userId['identifier']];
}, (array) $userData['userIds']) : [null];

unset($user->orgSourcedIds);
unset($user->agentSourcedIds);
}

$users[$userId] = $user;
}
return (object) [
'response' => (object) [
'users' => $users
]
];
default:
return new stdClass();
endswitch;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a trailing newline here

3 changes: 1 addition & 2 deletions classes/local/endpoints/rostering.php
Original file line number Diff line number Diff line change
Expand Up @@ -509,8 +509,7 @@ protected static function get_command_data(string $command): array {
if (array_key_exists($command, self::$commands)) {
return self::$commands[$command];
}

return parent::get_command_data($command);
return parent::get_command_data($command);
}

/**
Expand Down
3 changes: 1 addition & 2 deletions classes/local/factories/entity_factory.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ public function fetch_org_by_id(string $id): ?org_entity {
*/
public function fetch_school_by_id(string $id): ?school_entity {
$data = $this->fetch_from_cache('org', $id);

if (!$data) {
$data = school_entity::fetch_data(
$this->container,
Expand Down Expand Up @@ -383,7 +382,7 @@ public function fetch_grading_period_by_id(string $id): ?grading_period_entity {
* @param stdClass $data
* @return user_entity
*/
public function get_user_from_result(stdClass $data): user_entity {
public function get_user_from_result(stdClass $data): user_entity {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These might need to be removed at final merge time

$this->store_record_in_cache('user', $data->sourcedId, $data);
return new user_entity($this->container, $data->sourcedId, $data);
}
Expand Down
8 changes: 5 additions & 3 deletions classes/local/v1p1/oneroster_client.php
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,10 @@ public function sync_roster(?int $onlysincetime = null): void {
// Most systems do not have many organisations in them.
// Fetch all organisations to add them to the cache.
$this->fetch_organisation_list();

$schoolidstosync = explode(',', get_config('enrol_oneroster', 'datasync_schools'));
$countofschools = count($schoolidstosync);


$this->get_trace()->output("Processing {$countofschools} schools");

Expand All @@ -166,6 +167,7 @@ public function sync_roster(?int $onlysincetime = null): void {
// All timezones in One Roster are Zulu.
$this->sync_users_in_schools($schoolidstosync, $onlysince);


// Fetch the details of all enrolment instances before running the sync.
$this->cache_enrolment_instances();

Expand Down Expand Up @@ -318,7 +320,7 @@ public function sync_school(school_entity $school, ?DateTime $onlysince = null):
// Only fetch users last modified in the onlysince period.
$classfilter->add_filter('dateLastModified', $onlysince->format('o-m-d'), '>');
}

$this->get_trace()->output("Fetching class data", 3);
$classes = $school->get_classes([], $classfilter);
foreach ($classes as $class) {
Expand Down Expand Up @@ -465,7 +467,7 @@ protected function update_or_create_category(coursecat_representation $entity):
$localcategory = core_course_category::create($remotecategory);
$this->add_metric('coursecat', 'create');
}

return $localcategory;
}

Expand Down
Loading
Loading