Skip to content

feat: xblock skill verification event - #1

Closed
navinkarkera wants to merge 2 commits into
navin/skill-tagging-eventsfrom
navin/verification-events
Closed

feat: xblock skill verification event#1
navinkarkera wants to merge 2 commits into
navin/skill-tagging-eventsfrom
navin/verification-events

Conversation

@navinkarkera

@navinkarkera navinkarkera commented Nov 28, 2022

Copy link
Copy Markdown
Member

Adds data class and event to send skill verification data for an XBlock.

Updates avro serialization & de serialization just enough to support array types.

Description: The idea is that users will verify the tags/skills associated to an XBlock. We want to send this data via openedx-event signals to course-discovery and update the relevant tables.

JIRA: Private-ref: BB-6885

Dependencies: openedx#143

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.
  • Delete working branch (if not needed anymore)

Author concerns: Only support for array type is added as it is required for this event.

@tecoholic tecoholic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@navinkarkera Nice work here. I think you are a master of iterative development. :)
I have left a couple suggestions and a question linked to the location of the events in openedx/openedx-platform#31350. I will add the +1 once they are resolved.

Comment thread CHANGELOG.rst Outdated
Comment thread openedx_events/content_authoring/signals.py Outdated
Comment thread openedx_events/content_authoring/signals.py Outdated
Comment thread openedx_events/event_bus/avro/custom_serializers.py
Comment thread openedx_events/learning/signals.py Outdated
@navinkarkera
navinkarkera force-pushed the navin/skill-tagging-events branch from df0fc92 to 4c815d8 Compare December 5, 2022 13:14
@navinkarkera
navinkarkera force-pushed the navin/verification-events branch 2 times, most recently from 1be5aa7 to 68e8b42 Compare December 5, 2022 13:57
@navinkarkera

navinkarkera commented Dec 5, 2022

Copy link
Copy Markdown
Member Author

@tecoholic Thanks! Addressed all your comments.

Nice work here. I think you are a master of iterative development. :)

Thank you! 😊

@navinkarkera
navinkarkera force-pushed the navin/skill-tagging-events branch from 04b5014 to 1934a83 Compare December 9, 2022 07:17
@navinkarkera
navinkarkera force-pushed the navin/verification-events branch from 68e8b42 to 31b2256 Compare December 9, 2022 07:20

@tecoholic tecoholic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@navinkarkera 👍 The PR looks good. I think you have missed updating the version for the package.

  • I tested this: Relied on the unittests for the serializer as there are no testing instructions.
  • I read through the code
  • I checked for accessibility issues - NA
  • Includes documentation

@navinkarkera
navinkarkera force-pushed the navin/verification-events branch from 31b2256 to 77ad965 Compare December 12, 2022 14:05
@navinkarkera

Copy link
Copy Markdown
Member Author

I think you have missed updating the version for the package.

@tecoholic Ohh, thanks for catching it. Updated.

Adds data class and event to send skill verification data for an XBlock.

feat: add support for array avro types

chore: add changelog

docs: update signals docs to be more generic
@navinkarkera
navinkarkera force-pushed the navin/verification-events branch from ec33782 to d3b4807 Compare January 4, 2023 13:10
@navinkarkera

Copy link
Copy Markdown
Member Author

Replaced by openedx#165

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