-
Notifications
You must be signed in to change notification settings - Fork 7
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
docs: adds How To do xAPI Transforms #114
Conversation
and refines the xAPI Concepts docs to provide more detail.
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
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.
Thanks for this documentation
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.
Couple of nits and comments, otherwise this looks great. Couple of last thoughts:
- Maybe you could throw in a reference to the ERB filters somewhere, so that people know they don't necessarily have to create their own transforms if they only want to override some default behavior.
- Related, it's probably also worth noting that there can be only one registered xapi transformer per tracking log event name, so it is possible to completely override the default transforms, and also may be an unintended side effect of writing your own.
docs/concepts/xapi_concepts.rst
Outdated
|
||
Verb | ||
~~~~ | ||
Verbs in xAPI are URIs from the paired with short, translated, human-friendly labels. Verbs |
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.
nit: Couple of typos in this sentence
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.
docs/concepts/xapi_concepts.rst
Outdated
It can be a learner, instructor, system administrator, or any other agent | ||
involved in the learning process. | ||
|
||
Actors in Aspects are uniquely identified using a platform-generated ``external_id`` which |
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.
Might want to link in this doc: changing_actor_identifier somewhere here as it has some important specifics.
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.
docs/how-tos/xapi_transforms.rst
Outdated
The verb is the primary differentiator between different xAPI events in Open edX. Select a verb that describes the event as concisely and accurately as possible, so that future, similar | ||
events can still be discerned. | ||
|
||
Where possible, reuse verbs from one of the registered `xAPI profiles`_. See `ERB's verb list`_ for verbs currently in use in Aspects. |
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.
We should probably discourage people from reusing verbs that are already in use in ERB whenever possible, as we do a lot of processing based on the verb and it will potentially lead to confusion and maybe performance issues to overload terms.
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.
Talking about it, we should also update our filters to not only filter events based on the verb but also the object type and even the context activities. Mainly to allow developers to use the same verb and don't trigger any error on the downstream tables.
For the completion-aggregator and with internal libraries @andrey-canon had a lot of issues with the completion events because he was using the same verb
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.
Talking about it, we should also update our filters to not only filter events based on the verb but also the object type and even the context activities.
That's a good point.
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've updated this PR to discourage verb re-use: https://docsopenedxorg--114.org.readthedocs.build/projects/openedx-aspects/en/114/how-tos/xapi_transforms.html#verb
Yep, happy to do that. But I'm having trouble figuring out how these filters work.. does the @openedx_filter(filter_type="edx.completion.block_completion.changed")
def get_object(self):
... |
@pomegranited I believe the |
@pomegranited Brian is right, you need to use one of those filters. Here are some examples for the filters: https://github.com/eduNEXT/eox-nelp/blob/master/eox_nelp/openedx_filters/xapi/filters.py |
@Ian2012 @bmtcril Ah cool, thank you for these pointers. But xAPI filters doesn't seem to be a complete list? e.g. XApiContextFilter uses |
@pomegranited basically that is not in the list because that is not in the master code, the library that you are checking depends on a specific version of event-routing-backends that included that filter in this pr however I haven't had enough time to open the upstream pr |
@andrey-canon Ahh... I understand now, thank you! I've updated the code example to use a filter that's already in upstream: 123930a @bmtcril could you take another look at this PR when you get a chance? |
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.
LGTM thanks!
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Adds a "how to" document for creating new xAPI Transforms.
Supporting information
Closes #75