-
Notifications
You must be signed in to change notification settings - Fork 13
lti changes #644
base: main
Are you sure you want to change the base?
lti changes #644
Changes from 3 commits
7ec5396
ee0dfd7
d07623c
f30091c
32db3a5
070ca4f
4554626
2ad280f
938df6a
b8944e5
48a6467
7ac98ac
abe0606
47bf382
089e1c3
508d95d
28d73e9
96390e1
6a7c7a1
5bfdada
0216198
40f79ca
980b972
cee4464
e2aceec
db33df4
784dd48
5d3a3f4
8a4c151
972352e
aa9d7da
e1fdbb5
98d951e
29cb562
fb09de6
0405223
e2fb9a7
92db213
56d5cbb
9e6533a
7ba436a
13d4ad2
c365961
a4691ea
a533e56
08463b3
9da1fb6
7c53d06
83856ec
b336d3f
a39211c
819dbb7
6f05a41
839a1a8
d29cbd7
17ea6c3
1d8a951
9fbb70e
1351cc1
6c0ce3f
3abcc7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,6 @@ | |
from nbgrader.api import Course | ||
from nbgrader.api import Gradebook | ||
from nbgrader.api import InvalidEntry | ||
from sqlalchemy_utils import create_database | ||
from sqlalchemy_utils import database_exists | ||
|
||
from illumidesk.authenticators.utils import LTIUtils | ||
|
||
|
@@ -18,24 +16,23 @@ | |
nbgrader_db_port = os.environ.get("POSTGRES_NBGRADER_PORT") or 5432 | ||
nbgrader_db_password = os.environ.get("POSTGRES_NBGRADER_PASSWORD") | ||
nbgrader_db_user = os.environ.get("POSTGRES_NBGRADER_USER") | ||
nbgrader_db_name = os.environ.get("POSTGRES_NBGRADER_DATABASE") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If database does not exist, I am getting the following error
|
||
mnt_root = os.environ.get("ILLUMIDESK_MNT_ROOT", "/illumidesk-courses") | ||
|
||
org_name = os.environ.get("ORGANIZATION_NAME") or "my-org" | ||
|
||
if not org_name: | ||
raise EnvironmentError("ORGANIZATION_NAME env-var is not set") | ||
|
||
CAMPUS_ID = os.environ.get("CAMPUS_ID") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the database does exist in rds instance like in the case of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the database does exist in rds instance like in the case of
|
||
if not CAMPUS_ID: | ||
raise EnvironmentError("CAMPUS_ID env-var is not set") | ||
|
||
def nbgrader_format_db_url(course_id: str) -> str: | ||
def nbgrader_format_db_url() -> str: | ||
""" | ||
Returns the nbgrader database url with the format: <org_name>_<course-id> | ||
|
||
Args: | ||
course_id: the course id (usually associated with the course label) from which the launch was initiated. | ||
Returns the nbgrader database url | ||
""" | ||
course_id = LTIUtils().normalize_string(course_id) | ||
database_name = f"{org_name}_{course_id}" | ||
return f"postgresql://{nbgrader_db_user}:{nbgrader_db_password}@{nbgrader_db_host}:{nbgrader_db_port}/{database_name}" | ||
return f"postgresql://{nbgrader_db_user}:{nbgrader_db_password}@{nbgrader_db_host}:{nbgrader_db_port}/{nbgrader_db_name}" | ||
|
||
|
||
class NbGraderServiceHelper: | ||
|
@@ -51,7 +48,7 @@ class NbGraderServiceHelper: | |
database_name: the database name | ||
""" | ||
|
||
def __init__(self, course_id: str, check_database_exists: bool = False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what I'm understanding here is that all courses will be in one database instead of a database-specific for each org_course. This assumes that the database exists by default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, the DB will be common and will be maintained by the migrations in admin-dashboard code base There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may need to add back the |
||
def __init__(self, course_id: str): | ||
if not course_id: | ||
raise ValueError("course_id missing") | ||
|
||
|
@@ -62,56 +59,48 @@ def __init__(self, course_id: str, check_database_exists: bool = False): | |
self.uid = int(os.environ.get("NB_GRADER_UID") or "10001") | ||
self.gid = int(os.environ.get("NB_GRADER_GID") or "100") | ||
|
||
self.db_url = nbgrader_format_db_url(course_id) | ||
self.database_name = f"{org_name}_{self.course_id}" | ||
if check_database_exists: | ||
self.create_database_if_not_exists() | ||
|
||
def create_database_if_not_exists(self) -> None: | ||
"""Creates a new database if it doesn't exist""" | ||
conn_uri = nbgrader_format_db_url(self.course_id) | ||
|
||
if not database_exists(conn_uri): | ||
logger.debug("db not exist, create database") | ||
create_database(conn_uri) | ||
self.db_url = nbgrader_format_db_url() | ||
|
||
def add_user_to_nbgrader_gradebook(self, username: str, lms_user_id: str) -> None: | ||
def add_user_to_nbgrader_gradebook(self, email: str, external_user_id: str, source: str, source_type: str, role: str) -> None: | ||
""" | ||
Adds a user to the nbgrader gradebook database for the course. | ||
|
||
Args: | ||
username: The user's username | ||
lms_user_id: The user's id on the LMS | ||
email: The user's email | ||
external_user_id: The user's id on the external system | ||
source: source from where user was authenticated | ||
source_type: source_type | ||
Raises: | ||
InvalidEntry: when there was an error adding the user to the database | ||
""" | ||
if not username: | ||
raise ValueError("username missing") | ||
if not lms_user_id: | ||
raise ValueError("lms_user_id missing") | ||
if not email: | ||
raise ValueError("email missing") | ||
if not external_user_id: | ||
raise ValueError("external_user_id missing") | ||
|
||
with Gradebook(self.db_url, course_id=self.course_id) as gb: | ||
with Gradebook(self.db_url, course_id=self.course_id, campus_id=CAMPUS_ID) as gb: | ||
try: | ||
gb.update_or_create_student(username, lms_user_id=lms_user_id) | ||
user = gb.update_or_create_user_by_email(email, role=role, external_user_id=external_user_id, source=source, source_type=source_type) | ||
logger.debug( | ||
"Added user %s with lms_user_id %s to gradebook" | ||
% (username, lms_user_id) | ||
"Added user %s with external_user_id %s to gradebook" | ||
% (email, external_user_id) | ||
) | ||
return user.to_dict() | ||
except InvalidEntry as e: | ||
logger.debug("Error during adding student to gradebook: %s" % e) | ||
|
||
def update_course(self, **kwargs) -> None: | ||
""" | ||
Updates the course in nbgrader database | ||
""" | ||
with Gradebook(self.db_url, course_id=self.course_id) as gb: | ||
with Gradebook(self.db_url, course_id=self.course_id, campus_id=CAMPUS_ID) as gb: | ||
gb.update_course(self.course_id, **kwargs) | ||
|
||
def get_course(self) -> Course: | ||
""" | ||
Gets the course model instance | ||
""" | ||
with Gradebook(self.db_url, course_id=self.course_id) as gb: | ||
with Gradebook(self.db_url, course_id=self.course_id, campus_id=CAMPUS_ID) as gb: | ||
course = gb.check_course(self.course_id) | ||
logger.debug(f"course got from db:{course}") | ||
return course | ||
|
@@ -131,7 +120,7 @@ def register_assignment(self, assignment_name: str, **kwargs: dict) -> Assignmen | |
"Assignment name normalized %s to save in gradebook" % assignment_name | ||
) | ||
assignment = None | ||
with Gradebook(self.db_url, course_id=self.course_id) as gb: | ||
with Gradebook(self.db_url, course_id=self.course_id, campus_id=CAMPUS_ID) as gb: | ||
try: | ||
assignment = gb.update_or_create_assignment(assignment_name, **kwargs) | ||
logger.debug("Added assignment %s to gradebook" % assignment_name) | ||
|
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.
Has the removal of
GRADER_EXCHANGE_SHARED_PVC
been tested? I was still having issues after I removed the deployment. That being said I was testing it without efs access points as we didn't need that in the Oregon cluster. I can try again with a new image and approve this afterward. Let me know if you have a new image, if not I'll fetch this PR and create the image and once tested, approve this.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.
I have created a new image of this with pr-71 tag