-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[FC-0049] feat: Serialize tag data in OLX for blocks #34145
[FC-0049] feat: Serialize tag data in OLX for blocks #34145
Conversation
Thanks for the pull request, @yusuf-musleh! 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:
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. |
@yusuf-musleh I tested with an |
dd0e575
to
0980c09
Compare
daeeeff
to
e5a7f5b
Compare
7b66704
to
2cd2f09
Compare
@ChrisChV I fixed the issued for OpenAssessmentBlocks, turns out we needed to make changes to the I've added a test case for it as well. |
19ba8b0
to
39ff436
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.
@yusuf-musleh Thanks for this work! 👍
- I tested this: I followed the testing instructions.
- I read through the code and considered the security, stability and performance implications of the changes.
- Includes tests for bugfixes and/or features added.
- Includes documentation
requirements/edx/testing.txt
Outdated
@@ -992,7 +992,7 @@ optimizely-sdk==4.1.1 | |||
# via | |||
# -c requirements/edx/../constraints.txt | |||
# -r requirements/edx/base.txt | |||
ora2==6.0.31 | |||
ora2 @ git+https://github.com/open-craft/edx-ora2@yusuf-musleh/serialize-tag-data-in-openassessment-olx |
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 you remove this change from the diff for now? We can merge them independently.
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.
Oh I see, this has tests that depend on that PR. OK, we'll have to wait. Please ping me when the other PR has merged, and then I can merge this one.
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.
Yup, will do.
39ff436
to
9a2bdaa
Compare
@bradenmacdonald The ora2 PR has been merged, and the requirements in this PR are updated. It should be should be good to go. edit: Accidentally clicked on close PR, my bad. |
@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
""" | ||
) | ||
|
||
def test_tagged_openassessment_block(self): |
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.
@yusuf-musleh @bradenmacdonald @ChrisChV
This test, relies on the openassessment block which comes from ora2 and having this test here caused an interesting issue. When new attributes got added to the block in the edx-ora2 repo, it broke this test because the expected OLX in this repo did not have the new attributes. This seems like bad coupling across repos and my instinct would be to move/remove this specific test. But I'm not sure what the specific goals of this test and how important it is. Can we remove this from the edx-platform repo?
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.
@feanil The implementation/tests got updated since and now tags are no longer serialized and included in the actual OLX. So we could actually change the test case, namely changing this assert to just check that there is no tags data in the OLX, and we can get rid of EXPECTED_OPENASSESSMENT_OLX
since we would no longer need it.
That way it wouldn't cause issues when attributes are added/removed from the ora2, and avoids removing the test case, does that sound like a reasonable approach?
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.
@yusuf-musleh that sounds great, please make that change and add a link to the PR here.
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.
Description
This PR serializes applied tags on various blocks and includes them in the OLX when they are copied. The format of the serialized tags are based on openedx/modular-learning#174
Supporting information
Related Ticket:
Testing instructions
tags-v1
is not included in the OLXPrivate-ref: FAL-3605