-
Notifications
You must be signed in to change notification settings - Fork 0
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: add aggregator events #6
base: master
Are you sure you want to change the base?
Conversation
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 am not sure or is not under my scope. But how are loaded the json test current and expected data.
I saw something similar in this file
event-routing-backends/event_routing_backends/processors/xapi/tests/test_transformers.py
Line 17 in a3d132b
EXCEPTED_EVENTS_FIXTURES_PATH = '{}/fixtures/expected'.format(os.path.dirname(os.path.abspath(__file__))) |
but I dont now if there is somehting like to fixtures or someone else is testing these jsons.
Mainly the current
jsons are not inside xapi level
https://github.com/nelc/event-routing-backends/pull/6/files#diff-d5842e91648b041fe5ecf8caed70496a53e778d476b9561a494065980f1c4850
I thinks that is similar to this line
event-routing-backends/event_routing_backends/processors/tests/transformers_test_mixin.py
Lines 68 to 76 in a3d132b
input_event_file_path = '{test_dir}/fixtures/current/{event_filename}'.format( | |
test_dir=TEST_DIR_PATH, event_filename=event_filename | |
) | |
# if an event's expected fixture doesn't exist, the test shouldn't fail. | |
# evaluate transformation of only supported event fixtures. | |
expected_event_file_path = '{expected_events_fixtures_path}/{event_filename}'.format( | |
expected_events_fixtures_path=self.EXCEPTED_EVENTS_FIXTURES_PATH, event_filename=event_filename | |
) |
but variable is like
EXCEPTED_EVENTS_FIXTURES_PATH
. And EXCEPTED
that is different that tthe expected
folder?/
|
||
Returns: | ||
`Activity` | ||
""" | ||
if not self.object_type: |
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.
This could raise multple 500? when object_type is not defined. Is it possible?
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.
this works like an abstract class, if someone use this instead of one of the sub-classes will get the errors that you mentioned
""" | ||
Transformer for events generated when a user completes a section, subsection or unit. | ||
""" | ||
object_type = constants.XAPI_ACTIVITY_MODULE |
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.
Intesresting that module encompasses section, subsection or unit
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.
Based on the nelc requirements this should be module they doesn't make any differences between unit section and sub-sections
return Result( | ||
completion=self.get_data("data.completion") == 1.0, | ||
extensions=Extensions( | ||
{constants.XAPI_ACTIVITY_PROGRESS: self.get_data("data.completion") * 100} |
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.
So here you need the progress beetween [0, 100]
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 just moved the transformer from the completion file to the progress file I didn't design any logic here
original class
Description:
This implements multiple transformers for events that I'm proposing in this PR
Progress events, those will be emitted when the block has not reached the 100% of completion
edx.completion_aggregator.progress.course
edx.completion_aggregator.progress.chapter
edx.completion_aggregator.progress.sequential
edx.completion_aggregator.progress.vertical
Completion events, those will be emitted when the block has reached the 100% of completion
edx.completion_aggregator.completion.course
edx.completion_aggregator.completion.chapter
edx.completion_aggregator.completion.sequential
edx.completion_aggregator.completion.vertical
And basically I'm proposing this since the current "completion" implementation doesn't cover completion events and that has the same limitation as the edx-completion plugin, that only works with unit components
JIRA: Link to JIRA ticket
Dependencies: dependencies on other outstanding PRs, issues, etc.
Merge deadline: List merge deadline (if any)
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.