-
Notifications
You must be signed in to change notification settings - Fork 39
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
Change the LTI launch message, if user is not a course member #1417
Conversation
dashboard/lti_new.py
Outdated
@@ -192,7 +192,7 @@ def check_if_instructor(roles, username, course_id): | |||
def course_user_roles(roles, username): | |||
user_membership_roles = set([role for role in roles if role.find(COURSE_MEMBERSHIP) == 0]) | |||
if not user_membership_roles: | |||
logger.info(f'User {username} does not have course membership roles, must be admin {roles}') | |||
logger.error(f'User {username} does not have course membership roles, but has {roles} roles') |
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 just let the role print out as part of the message. But I am ok to remove it.
User Janedoe does not have course membership roles, but has ['http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator', 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Instructor', 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student', 'http://purl.imsglobal.org/vocab/lis/v2/system/person#User'] roles
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.
Okay thanks. I don't think this need to be bumped up to an error. In many cases they may be an administrator. But I think the problem came up on the certification tests of
However they said this would pass, it's just the message was confusing.
- Launch Instructor with Short Role Value
[2022-05-05 15:26:24 -0400] [INFO] [lti_new.py:194] User l.c.hutton@district1.com does not have course membership roles, must be admin ['Instructor']
- Launch Instructor with Unknown Role
[2022-05-05 15:28:46 -0400] [INFO] [lti_new.py:194] User l.c.hutton@district1.com does not have course membership roles, must be admin ['http://purl.imsglobal.org/vocab/lis/v2/unknown/unknown#Helper'
- Launch Instructor with No Role
[2022-05-05 15:31:46 -0400] [INFO] [lti_new.py:194] User l.c.hutton@district1.com does not have course membership roles, must be admin ['']
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 think our search for
COURSE_MEMBERSHIP = 'http://purl.imsglobal.org/vocab/lis/v2/membership'
Was where this was failing. So it doesn't find course membership roles but there are some roles. I don't know if we need to do anything for the No Role, even though they say it's fine for certification.
I'm not even sure what this course_user_roles
method is used for here? It looks like the only thing it does is remove the role if the user is TA and INSTRUCTOR? It seems like this could just be removed and that logic be in the check_if_instructor
method since that's the only place it's used.
user_roles = course_user_roles(roles, username)
is_instructor = check_if_instructor(user_roles, username, course_id)
(user_roles
is unused after 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 changed the logic of how instructor only is checked. I log message only if he is an instructor.
This check was in place since we only allow instructors to enable the data extraction process in MyLA after the very initial launch of the tool. Apart from that, we also allow MyLA Admin to enable the data extraction process ( or to say have a course entry in course
table.
Below user roles we need to test with
- Admins in Canvas
- Instructor in a course
- Instructor and TA in a course
- TA only
- Admin In MyLA
85bb581
to
9c32a58
Compare
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 think this change looks good! Thanks. Could you update the original issue with a test plan. This should be able to be tested by @jennlove-um by just trying to launch as a few different user types.
I added the test plan to the issues, Merging now |
Fixes #1381