Bugfix/username

[arthurian]

@lduarte1991 At long last, here's the PR I've been promising. It's a little more comprehensive than the band-aid fix I was originally contemplating, but hopefully not too destructive. Anyway, here's what I was thinking for resolving the duplicate user object issue. I'm going to recap the problem statement we discussed offline for posterity (or anyone else looking at this).

Problem Statement The situation we have right now is duplicate User objects. Every time a new profile is created, a new user object is created, even if a user object already exists with the same username. This PR attempts to solve that problem by only creating one user and then relating profiles back to it as they are created.

Here's what happens when an instructor registered in edX or canvas as "foo" uses the tool:

EdX(edx.org):
   - Course Context: FooX
        - User(id=1, username=foo) + LTIProfile(id=100, anon_id=222, user_id=1)
   - Course Context: BarX
        - User(id=2, username=foo) + LTIProfile(id=200, anon_id=333, user_id=2)
   - Course Context: ZedX
        - User(id=3, username=foo) + LTIProfile(id=300, anon_id=444, user_id=3)

Canvas(one.instructure.com):
   - User(id=1, username=foo) + LTIProfile(id=100, anon_id=111, user_id=1)
   - Course Context: Foo
   - Course Context: Bar
   - Course Context: Zed
Canvas(two.instructure.com):
   - User(id=2, username=foo) + LTIProfile(id=200, anon_id=222, user_id=2)
   - Course Context: Foo
   - Course Context: Bar
   - Course Context: Zed

The changes in this PR should result in something more like this:

EdX(edx.org):
   - User(id=1, username=foo)
   - Course Context: FooX
        - LTIProfile(id=100, anon_id=222, user_id=1)
   - Course Context: BarX
        - LTIProfile(id=200, anon_id=333, user_id=1)
   - Course Context: ZedX
        - LTIProfile(id=300, anon_id=444, user_id=1)

User(id=1, username=HUID)
Canvas(one.instructure.com):
   - LTIProfile(id=100, anon_id=111, user_id=1)
   - Course Context: Foo
   - Course Context: Bar
   - Course Context: Zed
Canvas(two.instructure.com):
   - LTIProfile(id=200, anon_id=222, user_id=1)
   - Course Context: Foo
   - Course Context: Bar
   - Course Context: Zed

Migrations

• Added the following fields to LTIProfile: name and scope.
  • name is the display name that can appear on the UI. Right now, the only place is on the edit course page where you can add admins. This is the field that will show instead of the user object's username (if it has a value). For backwards compatibility, if the profile has no name, then it will use the user objects' username.
  • scope defines the area in which the _anonid can be considered unique by the tool. For edX, this is just the course context, if I understand correctly. For Canvas, however, this is the entire domain/instance. In either case, the combination of the _anonid and scope should uniquely identify that profile. The scope can be used to limit what profiles are displayed as potential admin candidates for the course.
• Dropped the uniqueness constraint on the user relationship by changing it to a ForeignKey. This makes it official that there's a many-to-one relationship between profiles and user objects.

Code Changes This is a brief overview of the changes, but of course, you can read the diff below.

• I refactored the utils.create_new_user() function to reflect the above changes. The caller should pass in a username that can be considered globally unique on the platform. For edX, this is assumed to be the registered username, and for Canvas this is the SIS id (i.e. huid, xid, etc). Once the user object is created for the first time with that username, it will associate any subsequent profiles with it. Since you already have multiple user objects with the same username in edX, I added some logic to reuse the first user object (ordered by user.id). This might not be the best way to handle that, but since the data is already kind of corrupted, I'm not sure what the best course of action is.
• I refactored the views.launch_lti() function to pass the required parameters when creating new users. There's only two places where it is called. The second instance of where this is called has to do with creating the "preview" user to work around the issue in edX Studio.
• I refactored the models.LTICourse.create_course() method to set the course name properly and pulled out the method to add admins.

Comments What do you think of this approach to the problem? I made a number of assumptions as to how we can implement this in a way that preserves backwards compatibility and doesn't require extensive data cleanup (although that would be good to do at some point).

Do you see anything that could cause problems in edX? Is there a better approach or some things you think we should change? I'm open to any suggestions, this is just a starting point.

[lduarte1991]

Hmmm, nothing stands out as wrong or throwing any red flags. I would like to think of a way to clean up the database a bit (if at all possible). There's one particular course in Hx that has 10 different "Modules" so there are 10 versions of each of the TFs involved...I may play around with it a bit and see how I can do a work around...

[lduarte1991]

@arthurian Question: so in this method you have multiple LTIProfiles. Does that mean that the following is still true in your case:

Course1: username_1 is an admin username_2 is an admin

Course2: username_1 is an admin

Now if username_1 logged into Course2 and wanted to add username_2 as an admin, they would see the following:

• [x] username_1
• [ ] username_1
• [ ] username_2
• [ ] username_2

How would you know which "username_2" should be added?

[arthurian]

@lduarte1991 In that scenario, _username1 would only see other admins that had already logged in to Course 2. If _username2 has not logged in to Course 2 yet (i.e. no LTIProfile exists for that course), then they would no show up in the list of available admins for that course. Once _username2 logged in to Course 2, then _username1 could add them as an admin.

This behavior is controlled by the scope attribute on the LTIProfile model. See here in views.py and here in the CourseForm. The idea is that scope is used by the CourseForm to know which profiles exist for that course/consumer instance and as a result, can be added as admins.

I think that behavior should actually be changed, since we'd like to be able to add an admin by User object, even if their LTIProfile doesn't exist yet (i.e. they haven't logged in to the course/consumer instance yet). The problem is that we store "admin" users by the LTIProfile instance, and not by the User instance.

[lduarte1991]

Well the problem would still persist if the person you're trying to add has not ever created anything and they're not Users. I know there's a branch I made a while ago that has a draft of the what i've been thinking about below:

I agree that in CourseForm we should be loading users and not profiles to choose from. Below that should be a textfield that will take in a new username that is not in the list of Users.

These would get saved to a table that contains the Username and the Course ID.

Then during launch, when a user tries to log in we can check to see if they're on this new table. If their name and the course they are trying to access from are in this table, they get added as admins after their user/profile has been created. Finally the row is removed from this new table.

[lduarte1991]

@arthurian Take a look here.

[arthurian]

@lduarte1991 That functionality would be ideal. I'll take a look at the code you linked to and get back to you once I've had a chance to test it (I have a heavy meeting schedule today, so might not be able to look at it closely until tomorrow).

[lduarte1991]

It's also really old code (I just never got around to cleaning it up so I didn't merge it) but I can definitely make a separate PR after merging this one if you'd prefer. I haven't had any issues while running it the last few days.

[lduarte1991]

@arthurian I've been playing around with this in a local instance and am quite happy with the way it works. I might merge this ( :+1: ) and worry about the branch I pointed to you in a separate pull request. Thoughts?

[arthurian]

Sounds good to me! Let's do a follow up PR with the commits you pointed to.

--Artie

On Mar 31, 2016, at 11:57 AM, lduarte1991 notifications@github.com<mailto:notifications@github.com> wrote:

@arthurianhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_arthurian&d=CwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=fuNl0I548vn12NB-xHRU-ikQ_T-T1HwW_kcCe2uJ56M&m=4OSH-YUtUI11Fk4bQ_NqHxuxba0ISs_X8OEgZzMnitc&s=BXIf7ryMFzlTfDZgDnnZRYnaZZnMGQ0Z1taAMXglgdo&e= I've been playing around with this in a local instance and am quite happy with the way it works. I might merge this ( [:+1:] ) and worry about the branch I pointed to you in a separate pull request. Thoughts?

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_lduarte1991_hxat_pull_64-23issuecomment-2D203998762&d=CwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=fuNl0I548vn12NB-xHRU-ikQ_T-T1HwW_kcCe2uJ56M&m=4OSH-YUtUI11Fk4bQ_NqHxuxba0ISs_X8OEgZzMnitc&s=vSrZGZRCZn1pqvj5aFWlJs9Yot98zONrN50UUesqG-U&e=