-
Notifications
You must be signed in to change notification settings - Fork 2
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
Client tests #28
Conversation
* adding csv data in the form of arrays * adding unit test for the csv files * adding helper functions for processing csv files
* added a function that converts CSV data into Arrays * Clean-up some functions into helper functions
* only the orgs table was created
* execute function takes in csv data and returns that data in the right format * unit tests to confirm the format
* Process the users and orgs data * added better csv data examples in csv_data_helper
* synchronize is able to process csv data and stores it into the database * Fixed comments and added proper headers
- Fixed array handling in csv_data_helper - Updated csv_client configurations to properly handle type casting values
- only class was called before, now all of the classes in the school can be called
@@ -137,6 +137,9 @@ public function refresh_data(): Iterable { | |||
$this->get_filter(), | |||
$this->get_params(), | |||
function($data) { | |||
if (!($data instanceof stdClass)) { |
There was a problem hiding this comment.
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
* This plugin synchronises enrolment and roles with a One Roster endpoint. | ||
* | ||
* @package enrol_oneroster | ||
* @copyright Andrew Nicols <[email protected]> |
There was a problem hiding this comment.
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
|
||
defined('MOODLE_INTERNAL') || die; | ||
|
||
require_once($CFG->dirroot . '/lib/oauthlib.php'); |
There was a problem hiding this comment.
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 stdClass; | ||
use enrol_oneroster\local\v1p1\oneroster_client as versioned_client; | ||
use enrol_oneroster\local\interfaces\rostering_client; | ||
|
There was a problem hiding this comment.
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]> |
There was a problem hiding this comment.
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
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its probably worth having tests around a bunch of your helper methods for data loading etc as well, ensuring that the files can be read correctly and error in cases where they should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and especially the validation functions. Probably use a DataProvider for these
@@ -37,4 +38,5 @@ | |||
*/ | |||
abstract class oneroster_client implements client_interface, rostering_client_interface { | |||
use root_oneroster_client; | |||
use csv_oneroster_client; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be its own fixture so there aren't side effects on the Oauth classes tests
* | ||
* @covers enrol_oneroster\OneRosterHelper | ||
*/ | ||
class processcsv_test extends \advanced_testcase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI checks if the test class name is aligned with the filename. Check this with coding style docs
|
||
namespace enrol_oneroster\tests; | ||
use enrol_oneroster\OneRosterHelper; | ||
global $CFG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isnt required, as its called FROM the global namespace
*/ | ||
public function testCheckManifestAndFiles_allFilesPresent() | ||
{ | ||
$missing_files = OneRosterHelper::check_manifest_and_files($this->manifestPath, $this->testDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great, but its also worth testing each function individually
Changes completed and committed |
No description provided.