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

Feature: Import new name fields for GRLN compliance #9

Merged
merged 12 commits into from
Apr 11, 2024

Conversation

ctam
Copy link
Collaborator

@ctam ctam commented Mar 25, 2024

Ready for code review.

@ctam ctam requested a review from stopfstedt April 8, 2024 18:56
@ctam
Copy link
Collaborator Author

ctam commented Apr 8, 2024

Tests related to the new changes are passing. We will fix the other code check tests in a separate ticket.

// Fixing: this field could have 'question mark' in it.
// If so, just remove it. ($DB->execute() does not like '?')
// Fixing: these fields could have 'question mark' in it.
// If so, just remove it. ($DB->execute() does not like '?').
Copy link
Member

@stopfstedt stopfstedt Apr 8, 2024

Choose a reason for hiding this comment

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

turns out that's by design. for reference: see https://moodle.org/mod/forum/discuss.php?d=173498

my takeaway is that $DB->execute() should be avoided, if possible.

see https://moodledev.io/docs/apis/core/dml#execute

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #10 for this._

@stopfstedt
Copy link
Member

Tests related to the new changes are passing. We will fix the other code check tests in a separate ticket.

I don't see any tests running in the CI.
image

https://github.com/ucsf-education/moodle-admin-tool-ldapsync/actions/runs/8603199986/job/23574542617

@stopfstedt stopfstedt self-requested a review April 9, 2024 00:07
@@ -49,52 +50,109 @@ class tool_ldapsync_importer_testcase extends advanced_testcase {
private $sync = null;

protected function setUp(): void {
// Create new empty test container.
$topdn = 'dc=moodletest,' . TEST_TOOL_LDAPSYNC_DOMAIN;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see TEST_TOOL_LDAPSYNC_DOMAIN declared anywhere. same goes for the various other TEST_TOOL_ constants.

@stopfstedt stopfstedt self-requested a review April 9, 2024 00:30
Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

@ctam how do i run the tests?

@stopfstedt stopfstedt self-requested a review April 9, 2024 00:31
Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

per CLE team discussion, i'm waving this through.

@ctam ctam self-assigned this Apr 11, 2024
@ctam ctam marked this pull request as ready for review April 11, 2024 00:13
@ctam ctam merged commit 801e327 into ucsf-education:main Apr 11, 2024
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants