-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add support for complex types in dicts and lists #483
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
base: main
Are you sure you want to change the base?
feat: add support for complex types in dicts and lists #483
Conversation
Thanks for the pull request, @BryanttV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
62b7edd
to
e301f54
Compare
FYI @bmtcril @robrap @timmc-edx, this is the follow-up work from the discussion in PR #433! I'll be reviewing this shortly. Thank you, @BryanttV! |
bc3d3e6
to
7913f38
Compare
I'm going to attempt testing this locally against the redis and kafka backends today, sorry for the delay |
So far I'm not able to test successfully against the redis bus. I believe I've set things up correctly, but when submitting an ORA response (Demo Course, Module 3, "Intermediate Assessment Tools" -> "Open Response Assessment (ORA)") I get the following error:
Testing of the discussion event is in progress. |
Hi @bmtcril, what branch are you using? You should use this branch with the changes in the data attributes. |
@BryanttV I believe I'm on the correct branch since the error I'm seeing is a new one added in this PR's |
@BryanttV I was able to confirm that I'm on the right branch, and testing the discussions case also have an error:
|
7913f38
to
15356f9
Compare
Hi @bmtcril. I tested again, and it works for me. 🤔 I created two branches, the branch of this PR ( |
eb20710
to
014c404
Compare
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.
Sorry for the long delay @BryanttV I finally got things in a state to test and it worked well for me. I haven't tried the Kafka bus. Thanks for your patience.
If and when this merges (or before), can someone ensure kafka CI passes once this gets updated? Does anyone have any concerns about that, or do we think it should pass? |
openedx_events/tooling.py
Outdated
# "org.openedx.learning.user.notification.requested.v1", | ||
# "org.openedx.learning.forum.thread.created.v1", | ||
# "org.openedx.learning.forum.thread.response.created.v1", | ||
# "org.openedx.learning.forum.thread.response.comment.created.v1", | ||
# "org.openedx.learning.course.notification.requested.v1", | ||
# "org.openedx.learning.ora.submission.created.v1", |
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.
can we move these changes to another PR?
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.
Sure, I moved these changes here: #510
@BryanttV, friendly ping to Robert's comment. Can we look more into that? |
014c404
to
48a968a
Compare
Hi @mariajgrimaldi @robrap! I created a PR to test the CI in event-bus-kafka: openedx/event-bus-kafka#319. All checks passed ✅ |
e9c4c14
to
027c672
Compare
Description
This PR adds support for complex types in dictionaries and lists. e.g.
List[Dict[str, str]
: List of DictionariesList[List[int]
: List of ListsList[EventData]
: List of Data AttributesDict[str, List[str]]
: Dict of ListsDict[str, Dict[str, bool]]
: Dict of DictsDict[str, EventData]
: Dict of Data AttributesSupporting information
Testing instructions
To test with the event bus:
Install this plugin with the changes in this branch (complex-types-in-dicts-and-lists-samples). That branch modifies some data attributes for testing purposes.
Install an event bus backend like the Redis implementation in the LMS.
Set up the event bus with Tutor. For that, create a Tutor inline plugin with this configuration so the event is produced to Redis:
Now, to emit each event:
COURSE_NOTIFICATION_REQUESTED
:dict[str, List[str]]
Create a Waffle Flag for your course in
{lms_domain}/admin/waffle_utils/waffleflagcourseoverridemodel/
notifications.enable_notifications
You can check this by answering an ORA assignment (Check the next event for more details)
You should see a log in the LMS like this:
ORA_SUBMISSION_CREATED
:List[dict[str, str]]
In Studio, create a unit with an ORA assignment.
In the LMS, answer to the assignment.
You should see a log in the LMS like this:
COURSE_DISCUSSIONS_CHANGED
:List[DiscussionTopicContext]
In Studio, go to Pages & Resources > Discussions ⚙️ > Settings and update the configuration (maybe create a new discussion topic).
You should see a log in the CMS like this:
Other information
The
COURSE_NOTIFICATION_REQUESTED
is emitted in different ways inedx-platform
. This event works with the event bus in some cases, but not all. For example, If we create a new discussion thread (LMS > Discussions > Add a post), that emits an event, but sending to the event bus fails.The reason for this is that the
content_context
is a field of typedict
, but it can receive different types, such asint
,str
, orNone
. Here is an example of the event data.Currently, the support for Union Types such as
List[str | int]
ordict[str, str | int | None]
is not included, and for that reason is failing. And, the reason the event does not fail to answer an ORA assignment is that the event data looks like this:All value keys in
content_context
are strings.Deadline
None
Checklists
Check off if complete or not applicable:
Merge Checklist:
Post Merge:
finished.