feat: xblock skill verification event by navinkarkera · Pull Request #165 · openedx/openedx-events

navinkarkera · 2023-01-04T13:08:58Z

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: #143

Original MR: open-craft#1

Merge checklist:

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.

openedx-webhooks · 2023-01-04T13:09:03Z

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

mphilbrick211 · 2023-01-10T16:23:51Z

Hi @navinkarkera! Is this ready for review?

CC: @mariajgrimaldi

mariajgrimaldi · 2023-01-12T15:12:09Z

can we also add an instruction for testing?

navinkarkera · 2023-01-13T15:24:49Z

can we also add an instruction for testing?

@mariajgrimaldi Below xblock mixin/openedx-filter pipeline is making use of this signal to send user feedback about tags for the xblock. It has testing instructions included.

After completing test instructions from any one MR above, add below snippet somewhere, for example in lms/djangoapps/grades/signals/handlers.py

from openedx_events.learning.signals import XBLOCK_SKILL_VERIFIED
XBLOCK_SKILL_VERIFIED.connect(lambda **x: print(x))

Verify some tags from the UI created in the MR. It will print something like below:

edx.devstack.lms  | {'signal': <OpenEdxPublicSignal: org.openedx.content_authoring.xblock.skill.verified.v1>, 'sender': None, 'xblock_info': XBlockSkillVerificationData(usage_key='block-v1:edX+DemoX+Demo_Course+type@video+block@7503709870094870a123f697b09a692a', verified_sk
ills=[102, 103], ignored_skills=[101, 104]), 'metadata': EventsMetadata(id=UUID('f44a454c-9357-11ed-8ad8-0242ac13000a'), event_type='org.openedx.content_authoring.xblock.skill.verified.v1', minorversion=0, source='openedx/lms/web', sourcehost='lms.devstack.edx', time=dateti
me.datetime(2023, 1, 13, 15, 35, 48, 829956), sourcelib=(4, 1, 1))}

The exact section making use of this event in mixin can be found here. I am not sure how we can manually test these events independently.

mphilbrick211 · 2023-01-23T19:05:23Z

Flagging this for you @mariajgrimaldi :)

navinkarkera · 2023-01-27T14:50:20Z

@mariajgrimaldi Gentle reminder, this MR is blocking openedx/taxonomy-connector#135. Kindly review.

mariajgrimaldi · 2023-01-27T21:31:33Z

@navinkarkera: thanks for the patience! I'll be reviewing this on Monday.

mariajgrimaldi

I appreciate your patience! 🥇 This looks good I just left a few comments, mainly regarding the avro changes. I'll be waiting for your comments.

mariajgrimaldi · 2023-01-30T21:38:15Z

        return deserializer(data)
    elif data_type in PYTHON_TYPE_TO_AVRO_MAPPING:
        return data
+    elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":


Why don't you try 🤔:

Suggested change

elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":

elif data_type_origin in PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin):

Or this type checking is just for lists? Is there a reason that dicts shouldn't be checked?

@mariajgrimaldi I have only implemented support for list/array as dicts might require more change.

Alternatively, can we just do if data_type_origin == list? It seems odd to work backwards from the map.

@rgraber Makes sense.
Just in case we want to support other array like python types like set, the current condition would not require any change.

I can still change it to data_type_origin == list if you think my statement does not make sense.

I still think we should go with == list for clarity

@rgraber Done.

mariajgrimaldi · 2023-01-30T21:48:24Z

+        with self.assertRaises(TypeError):
+            deserializer.from_dict(initial_dict)


why should this fail?

Handling nested lists (with annotation) support again introduces complexity and I did not implement it as it is not required as of now.

If we remove inner list type annotation, i.e. change the type to List[list] it works because the schema is becomes: {'name': 'list_input', 'type': {'type': 'array', 'items': 'array'}}

def test_deserialization_of_nested_list_works(self): SIGNAL = create_simple_signal({"list_input": List[list]}) initial_dict = {"list_input": [[1, 3], [4, 5]]} deserializer = AvroSignalDeserializer(SIGNAL) data = deserializer.from_dict(initial_dict) self.assertEqual(data, initial_dict)

Do we want List[list] to work? Doesn't that open us up to issues with something like [[Object, 'a'], [1, "bananas"]]?

@rgraber Good point! I have updated it to allow only simple types like int, str, bool etc.

mariajgrimaldi · 2023-01-30T22:08:14Z

Don't forget to rebase with the main branch!

navinkarkera

@mariajgrimaldi Replied to your queries inline and rebased with main.

navinkarkera · 2023-01-31T13:36:05Z

        return deserializer(data)
    elif data_type in PYTHON_TYPE_TO_AVRO_MAPPING:
        return data
+    elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":


@mariajgrimaldi I have only implemented support for list/array as dicts might require more change.

navinkarkera · 2023-01-31T13:47:58Z

+        with self.assertRaises(TypeError):
+            deserializer.from_dict(initial_dict)


Handling nested lists (with annotation) support again introduces complexity and I did not implement it as it is not required as of now.

If we remove inner list type annotation, i.e. change the type to List[list] it works because the schema is becomes: {'name': 'list_input', 'type': {'type': 'array', 'items': 'array'}}

def test_deserialization_of_nested_list_works(self): SIGNAL = create_simple_signal({"list_input": List[list]}) initial_dict = {"list_input": [[1, 3], [4, 5]]} deserializer = AvroSignalDeserializer(SIGNAL) data = deserializer.from_dict(initial_dict) self.assertEqual(data, initial_dict)

mariajgrimaldi · 2023-01-31T14:27:20Z

Hello there folks! @robrap @rgraber @timmc-edx. We're introducing these changes to the library, and I wanted to ensure you're OK with them. Please let us know! 😋

rgraber

Couple questions. In general, I think more comments about what get_args and get_origin are expected to return would make it easier to follow.

rgraber · 2023-02-06T18:51:51Z

        return deserializer(data)
    elif data_type in PYTHON_TYPE_TO_AVRO_MAPPING:
        return data
+    elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":


Alternatively, can we just do if data_type_origin == list? It seems odd to work backwards from the map.

rgraber · 2023-02-06T18:52:03Z

+            raise TypeError(
+                "List without annotation type is not supported. The argument should be a type, for eg., List[int]"
+            )
+        if arg_data_type[0] in PYTHON_TYPE_TO_AVRO_MAPPING:


I think this will break on List[CourseKey] (or other custom-serialized class). That's ok but we should be very clear in exactly how much support we have for array types.

As I mentioned in one of replies above, I have updated the code to only support basic types inside lists and it will raise meaningful error if unsupported type is passed.

rgraber · 2023-02-06T19:42:54Z

+        with self.assertRaises(TypeError):
+            deserializer.from_dict(initial_dict)


Do we want List[list] to work? Doesn't that open us up to issues with something like [[Object, 'a'], [1, "bananas"]]?

navinkarkera · 2023-02-14T06:46:04Z

@mariajgrimaldi @rgraber Gentle reminder. Let me know if changes are required, else we should be good to merge.

mariajgrimaldi · 2023-02-14T15:21:54Z

we would like your approval here @rgraber :) thanks!

rgraber

Couple more things, but overall looks great!

rgraber · 2023-02-16T17:41:58Z

        return deserializer(data)
    elif data_type in PYTHON_TYPE_TO_AVRO_MAPPING:
        return data
+    elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":


I still think we should go with == list for clarity

rgraber · 2023-02-16T17:49:14Z

        with self.assertRaises(Exception):
            schema_from_signal(DICT_SIGNAL)
+
+    def test_throw_exception_to_list_or_dict_types_without_annotation(self):


Isn't this just a duplicate of the above test? Did you mean to rename?

@rgraber Thanks for catching it, I indeed meant to rename the function.

Update: Actually it was helping in code coverage, so I have updated the test to include a signal with List type.

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 test: fix code coverage refactor: add docstrings

rgraber

Looks good, thanks!

mariajgrimaldi

great job! thank you

openedx-webhooks · 2023-02-17T16:12:22Z

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

mariajgrimaldi · 2023-02-17T16:21:45Z

@navinkarkera: the new release is out!

navinkarkera · 2023-02-20T09:48:22Z

@mariajgrimaldi @rgraber Thank you!

This reverts commit 26a8dd5.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 4, 2023

navinkarkera force-pushed the navin/verification-events branch from ec33782 to d3b4807 Compare January 4, 2023 13:10

This was referenced Jan 4, 2023

feat: xblock skill verification event open-craft/openedx-events#1

Closed

feat: xblock verification event handler openedx/taxonomy-connector#135

Merged

mphilbrick211 requested a review from mariajgrimaldi January 10, 2023 16:23

mariajgrimaldi reviewed Jan 12, 2023

View reviewed changes

Comment thread openedx_events/learning/data.py Outdated

mariajgrimaldi reviewed Jan 12, 2023

View reviewed changes

Comment thread openedx_events/event_bus/avro/tests/test_deserializer.py

navinkarkera force-pushed the navin/verification-events branch from d28c3ee to 7fe2742 Compare January 13, 2023 15:15

navinkarkera force-pushed the navin/verification-events branch from 7fe2742 to 3392538 Compare January 13, 2023 15:25

mariajgrimaldi self-assigned this Jan 23, 2023

mariajgrimaldi self-requested a review January 23, 2023 19:10

navinkarkera force-pushed the navin/verification-events branch 2 times, most recently from 99d3b41 to 38da62a Compare January 27, 2023 14:47

mariajgrimaldi requested changes Jan 30, 2023

View reviewed changes

navinkarkera force-pushed the navin/verification-events branch from 38da62a to dcda85d Compare January 31, 2023 11:28

navinkarkera commented Jan 31, 2023

View reviewed changes

rgraber reviewed Feb 6, 2023

View reviewed changes

robrap added the event-bus Work related to the Event Bus. label Feb 6, 2023

navinkarkera force-pushed the navin/verification-events branch 2 times, most recently from 8a33abb to 0009eda Compare February 14, 2023 06:44

navinkarkera force-pushed the navin/verification-events branch from 0009eda to b9c11a2 Compare February 16, 2023 08:04

rgraber suggested changes Feb 16, 2023

View reviewed changes

navinkarkera added 4 commits February 17, 2023 12:33

feat: xblock skill verification event

b0b6f07

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 test: fix code coverage refactor: add docstrings

refactor: allow simple types in list

6c6d3f1

chore: bump version and update changelog

b9b80eb

fix: remove duplicate test and add comments to test

6b6b646

navinkarkera force-pushed the navin/verification-events branch from 5f62d9f to 6b6b646 Compare February 17, 2023 07:03

fix: code coverage

58b1f18

rgraber approved these changes Feb 17, 2023

View reviewed changes

mariajgrimaldi approved these changes Feb 17, 2023

View reviewed changes

mariajgrimaldi merged commit 26a8dd5 into openedx:main Feb 17, 2023

navinkarkera added a commit to open-craft/openedx-events that referenced this pull request Feb 20, 2023

Revert "feat: xblock skill verification event (openedx#165)"

a6cc3b8

This reverts commit 26a8dd5.

	elif PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin) == "array":
	elif data_type_origin in PYTHON_TYPE_TO_AVRO_MAPPING.get(data_type_origin):

		with self.assertRaises(TypeError):
		deserializer.from_dict(initial_dict)

Uh oh!

Conversation

navinkarkera commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Jan 4, 2023

Uh oh!

mphilbrick211 commented Jan 10, 2023

Uh oh!

Uh oh!

Uh oh!

mariajgrimaldi commented Jan 12, 2023

Uh oh!

navinkarkera commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mphilbrick211 commented Jan 23, 2023

Uh oh!

navinkarkera commented Jan 27, 2023

Uh oh!

mariajgrimaldi commented Jan 27, 2023

Uh oh!

mariajgrimaldi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mariajgrimaldi commented Jan 30, 2023

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi commented Jan 31, 2023

Uh oh!

rgraber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Feb 14, 2023

Uh oh!

mariajgrimaldi commented Feb 14, 2023

Uh oh!

rgraber left a comment

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Jan 4, 2023 •

edited

Loading

navinkarkera commented Jan 13, 2023 •

edited

Loading

mariajgrimaldi left a comment •

edited

Loading

navinkarkera Feb 17, 2023 •

edited

Loading