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

feat: Make xAPI User Identifier configurable #274

Merged
merged 1 commit into from Apr 19, 2023

Conversation

ziafazal
Copy link
Contributor

This PR has changes to make user identifier in xAPI events configurable. We have introduced a new setting XAPI_AGENT_IFI_TYPE to make it configurable.

This setting can be used to specify the type of inverse functional identifier for actor in xAPI statements. Possible values are external_id, mbox_sha1sum and mbox

when we set it to external_id xAPI statement would represent actor like this

{
"objectType": "Agent",
"account": {"homePage": "http://localhost:18000", "name": "32e08e30-f8ae-4ce2-94a8-c2bfe38a70cb"}
}

setting it to mbox xAPI statement would represent actor like this

{
"objectType": "Agent",
"mbox": "mailto:info@xapi.com"
}

setting it to mbox_sha1sum xAPI statement would represent actor like this

{
"objectType": "Agent",
"mbox_sha1sum": "f427d80dc332a166bf5f160ec15f009ce7e68c4c"
}

Implements #258

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Just the one question about non-existent users, otherwise it looks great

user = get_user(username_or_id)

if not user:
logger.info('User with username "%s" does not exist.', username_or_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly concerned about this case. I can see it happening in the event of log replay with retired users, who get their username changed, but can you think of any other case where it may happen? If it's only retired users, we may be able to change the username case in get_user to use this to make the user consistent: https://github.com/openedx/edx-platform/blob/bf36c429509b92854799d5d9b5203b4eb9d7e0df/common/djangoapps/student/models/user.py#L317

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmtcril I can't think of anyother use case. I have a added a check to get retired user if user is not found in normal flow.

fix: improved test coverage

fix: updated changelog and fix test

feat: get potentially retired user

fix: fix quality violations

fix: quality violation

fix: improve test coverage
Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@ziafazal ziafazal merged commit a788fbd into master Apr 19, 2023
9 checks passed
@ziafazal ziafazal deleted the ziafazal/agent-ifi-type branch April 19, 2023 12:50
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.

None yet

2 participants