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 tracking logs #1

Open
wants to merge 2 commits into
base: open-release/maple.master
Choose a base branch
from

Conversation

andrey-canon
Copy link
Collaborator

@andrey-canon andrey-canon commented Dec 4, 2023

Description:
This pr adds the capability of publishing aggregator events

image

Testing instructions:

  1. Create a section with one subsection and one unit
  2. Completes previous unit
  3. check that chapter, sequential and vertical events was emitted

Reviewers:

  • tag reviewer

Merge checklist:

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

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

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

@andrey-canon andrey-canon marked this pull request as ready for review December 4, 2023 17:34
@andrey-canon andrey-canon marked this pull request as draft December 4, 2023 17:35
@andrey-canon andrey-canon marked this pull request as ready for review December 4, 2023 17:35
@andrey-canon andrey-canon force-pushed the and/add_tracking_logs branch 3 times, most recently from b5e3d4d to c450d9f Compare December 4, 2023 21:38
@andrey-canon andrey-canon force-pushed the and/add_tracking_logs branch 2 times, most recently from 329502f to d0f60e3 Compare December 4, 2023 22:09
@andrey-canon andrey-canon force-pushed the and/add_tracking_logs branch from d0f60e3 to ead8f61 Compare December 4, 2023 22:14
@andrey-canon andrey-canon changed the base branch from master to open-release/maple.master December 5, 2023 15:01
return obj, is_new

@staticmethod

Choose a reason for hiding this comment

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

The idea of this static method is to reuse the function class in whatever place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The method doesn't need the self(instance) or cls(class reference) therefore by definition it's a static method, so when I implemented that I didn't intend to create a function to be used in multiple places it's just a static method that could be used in other context

updated_aggregators: List of Aggregator intances

"""
for obj in updated_aggregators:

Choose a reason for hiding this comment

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

Why you change to obj if in other places is agregator.

for aggregator in updated_aggregators:

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 understand your concern and IMO it's much better aggregator however your argument is not valid since in other places is obj

obj, is_new = self.update_or_create(

aggregation_data = [obj.get_values() for obj in updated_aggregators]

"created": obj.created,
"earned": obj.earned,
"possible": obj.possible,
"percent": obj.percent,

Choose a reason for hiding this comment

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

This doesnt need anything change.
I mean this serializer is ok?

percent = models.FloatField(validators=[validate_percent])

Choose a reason for hiding this comment

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

Beetween [0,1]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That method just validates the kind of event (progress or completion) the data that is sent must be as accurate as possible

@@ -13,18 +13,18 @@ def plugin_settings(settings):
# This setting controls which type of event will be published to change the default behavior
# the block type should be removed or added from the progress or completion list.
settings.ALLOWED_COMPLETION_AGGREGATOR_EVENT_TYPES = {
"progress": [
"progress": {

Choose a reason for hiding this comment

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

Why do you change from list to set ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johanseto johanseto self-requested a review December 5, 2023 19:57
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