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

issue #1443 added the sis_name retrieval back to user query for UDW cron query #1444

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

zqian
Copy link
Member

@zqian zqian commented Sep 20, 2022

This is partial revert of cron.hjson change in #1392: restored sis_name, and still leave out the user's name field.

Will file a UDP request for including Canvas user login_id in UDP, so that we can retrieve the sis_name field in UDP cron, too

… query for UDW cron; will file a UDP request for including Canvas user login_id in UDP, so that we can retrieve the sis_name field in UDP cron, too
@pushyamig
Copy link
Contributor

I am reviewing it now.

@pushyamig
Copy link
Contributor

I ran the cron for UDW queries and I was able to see sis_name populated ( but was not before unless an LTI launch happens). I saw the media events like Mivideo populated for course 562259 and I did noticed for another course 563654 leccap event also populated.

This change looks good and the query that is revered back exactly that one in the past (i.e the working version)

@jonespm
Copy link
Member

jonespm commented Sep 20, 2022

With the current code these names are present in the auth_user->username field.

However it looks like there's code in the cron that converts the -1 user_id to a numeric value from the user table. I think the better fix for this will be just continuing along and storing the canvas user Id from the launch somewhere and just using that rather than querying from the warehouse. We may need to extend the Django User model.

user is really a special many to one table which still has some redundancy with the Django auth_user table. This is just legacy which isn't cleaned up.

I guess this is fine for now and probably quickest, but I don't think it's ultimately necessary and can be fixed by:

  1. Capturing and storing the canvas_user_id during the launch in a custom canvas_user model.
  2. Using this table in the cron instead of user

The biggest negative I can see of this approach is that the canvas_user_id won't be available until the user logs in for the first time. So there will be 24 hours until the cron is able to update for this user to get MiVideo events. I think the only way that could be fixed would be to store either/or the sis_name or user_id to resource_access and just doing the lookup in the view rather than the cron.

@jonespm
Copy link
Member

jonespm commented Nov 29, 2022

This looks good for the UDW, but this also needs to include a fix to pull what we can for UDP. I'm working on that and will test both of these out.

@jonespm
Copy link
Member

jonespm commented Nov 29, 2022

I tested with both UDW and UDP. You can give it a review if you'd like before merging @zqian.

For the UDP it still has the limitation of not being able to return ids for internal or staff accounts. But seems to do fine for students and friend accounts. It does put their ID in there as the name which is better than nothing and maybe all we have for now.

@zqian zqian merged commit 7937fa3 into tl-its-umich-edu:master Nov 30, 2022
@zqian zqian deleted the issue_1443 branch November 30, 2022 17:26
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.

Some process needs to still retrieve/persist user_id and sis_name as it is used for MiVideo access events
3 participants