[FC-0049] feat: Serialize tag data in OLX for blocks by yusuf-musleh · Pull Request #34145 · openedx/openedx-platform

yusuf-musleh · 2024-01-30T16:08:30Z

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:

[Tagging] Copying Tags data when blocks are copied modular-learning#179
Currently implemented without the use of Export ID, but should it should utilize it once the following PRs are merged:
- feat: Add export_id field on Taxonomy openedx-core#145
- feat: Export ID on Taxonomy #34143
For the tags to serialize and appear for OpenAssessmentBlocks, we need:
- [FC-0049] feat: Serialize tag data in OpenAssessmentBlocks edx-ora2#2171

Testing instructions

Start you localdev stack on this branch
Make sure you have Taxonomy/Tags data setup locally: https://github.com/open-craft/taxonomy-sample-data/
Once you have a course with a bunch of units and blocks with tags applied, trying copying units and various types of blocks
For each copy, double check that staged content contains the tags data and there are no issues with pasting
Also confirm that when copying a unit/block that contains no tag, the tags-v1 is not included in the OLX
Finally, make sure the tests cover the various cases.
For the OpenAssessmentBlock to work, you should setup [FC-0049] feat: Serialize tag data in OpenAssessmentBlocks edx-ora2#2171 locally

Private-ref: FAL-3605

openedx-webhooks · 2024-01-30T16:08:36Z

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:

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.

ChrisChV · 2024-01-31T14:35:36Z

@yusuf-musleh I tested with an openassessment block and the tags are not saved in the OLX (on the Sample Taxonomy Course > Unit 2.1.1)

xmodule/xml_block.py

setup.cfg

cms/lib/xblock/tagging/tagged_block_mixin.py

yusuf-musleh · 2024-02-05T14:05:31Z

@ChrisChV I fixed the issued for OpenAssessmentBlocks, turns out we needed to make changes to the edx-ora2 repo, not sure if that is the best approach or not, but it works. There's just some test coverage that is missing. To test it out, you'd need to setup openedx/edx-ora2#2171 locally for it to work.

I've added a test case for it as well.

ChrisChV

@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

bradenmacdonald · 2024-02-12T23:44:44Z

requirements/edx/testing.txt

Can you remove this change from the diff for now? We can merge them independently.

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.

Yup, will do.

yusuf-musleh · 2024-02-14T11:57:05Z

@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.

openedx-webhooks · 2024-02-14T18:30:26Z

@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.

edx-pipeline-bot · 2024-02-14T19:43:58Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-02-14T20:03:17Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-02-14T20:03:17Z

2U Release Notice: This PR has been deployed to the edX production environment.

feanil · 2024-05-13T14:10:23Z

openedx/core/lib/xblock_serializer/test_api.py

+            """
+        )
+
+    def test_tagged_openassessment_block(self):


@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?

@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?

@yusuf-musleh that sounds great, please make that change and add a link to the PR here.

@feanil Sure thing, here it is #34782

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

yusuf-musleh force-pushed the yusuf-musleh/serialize-tag-data-in-olx branch 11 times, most recently from dd0e575 to 0980c09 Compare February 1, 2024 16:19

bradenmacdonald reviewed Feb 1, 2024

View reviewed changes

xmodule/xml_block.py Outdated Show resolved Hide resolved

bradenmacdonald reviewed Feb 1, 2024

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

bradenmacdonald reviewed Feb 1, 2024

View reviewed changes

cms/lib/xblock/tagging/tagged_block_mixin.py Outdated Show resolved Hide resolved

yusuf-musleh force-pushed the yusuf-musleh/serialize-tag-data-in-olx branch 4 times, most recently from daeeeff to e5a7f5b Compare February 4, 2024 19:57

yusuf-musleh mentioned this pull request Feb 5, 2024

[FC-0049] feat: Serialize tag data in OpenAssessmentBlocks openedx/edx-ora2#2171

Merged

5 tasks

yusuf-musleh force-pushed the yusuf-musleh/serialize-tag-data-in-olx branch 3 times, most recently from 7b66704 to 2cd2f09 Compare February 5, 2024 13:22

yusuf-musleh force-pushed the yusuf-musleh/serialize-tag-data-in-olx branch 4 times, most recently from 19ba8b0 to 39ff436 Compare February 12, 2024 10:44

yusuf-musleh changed the title ~~feat: Serialize tag data in OLX for blocks~~ [FC-0049] feat: Serialize tag data in OLX for blocks Feb 12, 2024

ChrisChV approved these changes Feb 12, 2024

View reviewed changes

bradenmacdonald reviewed Feb 12, 2024

View reviewed changes

feat: Serialize tag data in OLX for blocks

9a2bdaa

yusuf-musleh force-pushed the yusuf-musleh/serialize-tag-data-in-olx branch from 39ff436 to 9a2bdaa Compare February 14, 2024 11:08

yusuf-musleh closed this Feb 14, 2024

yusuf-musleh reopened this Feb 14, 2024

bradenmacdonald approved these changes Feb 14, 2024

View reviewed changes

bradenmacdonald merged commit 6e0bc66 into openedx:master Feb 14, 2024

bradenmacdonald deleted the yusuf-musleh/serialize-tag-data-in-olx branch February 14, 2024 18:30

feanil reviewed May 13, 2024

View reviewed changes

yusuf-musleh mentioned this pull request May 13, 2024

test: Update tagged openassessment test #34782

Merged

Conversation

yusuf-musleh commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Uh oh!

openedx-webhooks commented Jan 30, 2024

Uh oh!

ChrisChV commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yusuf-musleh commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisChV left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Feb 14, 2024

Uh oh!

edx-pipeline-bot commented Feb 14, 2024

Uh oh!

edx-pipeline-bot commented Feb 14, 2024

Uh oh!

edx-pipeline-bot commented Feb 14, 2024

Uh oh!

feanil May 13, 2024

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh May 13, 2024

Choose a reason for hiding this comment

Uh oh!

feanil May 13, 2024

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh May 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yusuf-musleh commented Jan 30, 2024 •

edited

Loading

ChrisChV commented Jan 31, 2024 •

edited

Loading

yusuf-musleh commented Feb 5, 2024 •

edited

Loading

yusuf-musleh commented Feb 14, 2024 •

edited

Loading