-
Notifications
You must be signed in to change notification settings - Fork 15
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
✨(converter) add edx to xapi video converter #211
Conversation
daa38ad
to
3ff6eb9
Compare
3ff6eb9
to
1eed42d
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.
Looks great) have only minor questions & suggestions)
Also, it seems that some required extensions are missing; a table of required/optional extensions of the video profile can be found here.
If it's not possible to add a required extension due to insufficient data in the edX event, could we add a comment in the converter class doc-string?
@SergioSim |
1eed42d
to
38f7110
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.
Looks great) Almost there 👍 only a few comments/suggestions left
length: Optional[float] = Field(alias=VIDEO_EXTENSION_LENGTH) | ||
playedSegments: Optional[str] = Field(alias=VIDEO_EXTENSION_PLAYED_SEGMENTS) |
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.
Isn't the length
a context extension field (to remove) and the playedSegments
a result extension field (to keep)?
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.
playedSegments is not in the template description, and length neither by the way !
https://profiles.adlnet.gov/profile/90b2c849-d744-4d0c-8bd0-403e7859a35b/templates/d0ee8f46-5704-428c-b21e-b583f99cd571
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.
You are right! It seems there are differences between the video profile on the ADL profile server and the video profile documentation (recipe).
This might be because the video profile on the ADL profile server is version https://w3id.org/xapi/video/v1.0.2
and the documentation for the video profile is for version https://w3id.org/xapi/video/v1.0.3
😕
Also, looking at the ADL profile server user guide, it seems that once a profile is published, it cannot be updated (a part of adding translations).
Thus, could it be that the video profile might not be updated to v1.0.3
on the ADL profile server?
Could we add a note in the xAPI video statements model that they are using version 1.0.2
of the video profile?
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.
Where do you see in the documentation the v1.0.3 version ? I don't get it. Only the 1.0.2 version has been published on the ADL server, I don't get why they would have documented a newer version before publishing it 😞
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.
It seems they indeed don't mention a version number on their gitbook, however, the recipe for the video seeked statement looks very similar to the v1.0.3
jsonld template of the video profile on the xapi-authored-profiles repository.
I don't know why they published v1.0.2
on the profile server instead of v1.0.3
. Maybe it's more widely accepted/used?
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.
@SergioSim It's very unclear indeed. I based the model on v1.0.2 as it is the more visible on the profile documentation. But indeed, it would change a bit if the v1.0.3 is the official new version for this profile. I can ask in a issue the reason why. WDYT?
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.
Good Idea) I think version v1.0.2 should be fine, however, it's always good to double check)
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.
@SergioSim I opened this issue in the xapi-authored-profile repository.
dbe301c
to
398d4f3
Compare
b88deff
to
763d10e
Compare
763d10e
to
1158247
Compare
1158247
to
02c65b0
Compare
5e7154d
to
cdaa89b
Compare
@SergioSim the last round! 🙏 |
6f1bcce
to
707af4b
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.
@@ -17,27 +17,27 @@ | |||
) | |||
|
|||
|
|||
class VideoActionResultExtensionsField(BaseModelWithConfig): | |||
class VideoResultExtensionsField(BaseModelWithConfig): |
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.
Same suggestion here to make the VideoResultExtensionsField
inherit from the BaseModel
which let's us remove the Config
class overwrites in Video*ResultExtensionsField
classes.
class Config: # pylint: disable=missing-class-docstring
min_anystr_length = 0
@SergioSim even if you have already approved it, I made then modification you suggested. For the |
Thanks a lot @quitterie-lcs ) Good Idea, let's discuss/resolve the optional |
edX video events can be converted to xAPI for most of them. The interaction events can still not be converted.
When statements and events fields have to be tested for the edx models, two separate files are created for statements and events fields tests respectively.
`unhashable-member` pylint warning has to be disabled for some parts of the code.
75dca8d
to
2d94ecd
Compare
Purpose
edX video events can be converted to xAPI for most of them. The interaction events can still not be converted.
Proposal
load_video
toinitialized
play_video
toplayed
pause_video
topaused
stop_video
toterminated
seek_video
toseeked