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: add aggregator events #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrey-canon
Copy link
Collaborator

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:

  1. Install this version of completion aggregator feat: add tracking logs open-craft/openedx-completion-aggregator#173
  2. Raise the event by completing a section or subsection or unit
  3. Verify the the send task was executed
  4. Verify that your LRS is getting the events

image

image

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

Copy link
Collaborator

@johanseto johanseto left a 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

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

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:
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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}
Copy link
Collaborator

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]

Copy link
Collaborator Author

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

@johanseto johanseto self-requested a review December 11, 2023 15:13
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.

2 participants