feat: xblock metadata provider in taxonomy support by navinkarkera · Pull Request #3692 · openedx/course-discovery

navinkarkera · 2022-11-16T06:20:54Z

Description

Implements XBlockMetadataProvider based on abstract XBlockMetadataProvider from openedx/taxonomy-connector#119 in taxonomy/providers/xblock_metadata.py. Utilizes index_dictionary from course blocks api added in openedx/openedx-platform#31273 to fetch text representation of xblock.

Currently the provider supports video and vertical block types, configured via TAXONOMY_XBLOCK_SUPPORTED_TYPES setting. It combines index_dictionary text from all children blocks.

Supporting information

Testing instructions

Checkout master branch in edx-platform.
Start lms and discovery service in devstack using make lms-up discovery-up
Follow instructions given here to setup taxonomy-connector in course-discovery, make sure to clone https://github.com/open-craft/taxonomy-connector in <devstack-base-dir>/src/
Open discovery shell using make discovery-shell
Run ./manage.py migrate
Open django shell using ./manage.py shell
Run below commands to make

from course_discovery.apps.taxonomy_support.providers import DiscoveryXBlockMetadataProvider
provider = DiscoveryXBlockMetadataProvider()
course_blocks = provider.get_all_xblocks_in_course("course-v1:edX+DemoX+Demo_Course")
print(next(course_blocks)) # run few times to check fetched data
blocks = provider.get_xblocks(["block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical_0270f6de40fc"])
print(blocks) # should print video and vertical block

Pre-merge checklist

merge feat: adds xblock skills tagging models taxonomy-connector#119
merge feat: api for block metadata including index_dictionary openedx-platform#31273
Update taxonomy-connector in requirements (already updated as part of c0d4f28)

openedx-webhooks · 2022-11-16T06:20:59Z

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 · 2022-11-22T02:08:38Z

@azanbinzahid - could you please take a look at this and allow tests to run?

azanbinzahid · 2022-11-22T11:09:30Z

@azanbinzahid - could you please take a look at this and allow tests to run?

Hi @mphilbrick211, apologies for the inconvenience. Team, please take a look @ansabgillani @DawoudSheraz @Ali-D-Akbar

navinkarkera · 2022-11-22T15:03:53Z

+taxonomy-connector @ git+https://github.com/open-craft/taxonomy-connector.git@5b9fedcc5c68370f3de5d49e18aae72d0d8226ad
+    # FIXME: remove this once a new tag is pushed in taxaonomy-connector


Temporarily added this to make CI/CD work.
Note to self: Remove this and update to latest taxonomy version before merge

@DawoudSheraz @Ali-D-Akbar @ansabgillani Apologies, could you please allow the tests to run again. It was missing this change.

@ansabgillani Thank you! Although it did not work as CI is using local.txt which I missed to update. Please run the tests again. Sorry.

The tests have been run. Do you have the access to taxonomy-connector to release a new version? If not, I can get this released so you can run make upgrade to update the version for taxonomy-connector. Just let me know about it.

@Ali-D-Akbar Thank you! Fixed commit lint issue as well as a failing test (although this was working locally, lets see if the change works).

Do you have the access to taxonomy-connector to release a new version? If not, I can get this released so you can run make upgrade to update the version for taxonomy-connector. Just let me know about it.

I don't have the access to create new versions, but we are in discussion with @irfanuddinahmad and @sameenfatima78 for taxonomy-connector MR. Maybe you guys can coordinate and release. Thank you for your help.

@navinkarkera We'll need to merge the PR in taxonomy-connector first before merging and releasing a new version here.

@sameenfatima78 Yes, I meant releasing a new version in taxonomy-connector.

Ah okay, I can take care of that if you don't have access right now.

farhaanbukhsh

I am still trying to test on local devstack but there are few comments which I could get while reading the code :)

farhaanbukhsh · 2022-11-23T16:28:37Z

Hey @navinkarkera thank you for the amazing work, I am trying to test the code with the instructions you provided and I am getting

LMS logs

edx.devstack.lms  | During handling of the above exception, another exception occurred:
edx.devstack.lms  | 
edx.devstack.lms  | Traceback (most recent call last):
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/manager.py", line 96, in get_collected
edx.devstack.lms  |     block_structure = BlockStructureFactory.create_from_store(
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/factory.py", line 86, in create_from_store
edx.devstack.lms  |     return block_structure_store.get(root_block_usage_key)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/store.py", line 99, in get
edx.devstack.lms  |     serialized_data = self._get_from_store(bs_model)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/store.py", line 190, in _get_from_store
edx.devstack.lms  |     raise BlockStructureNotFound(bs_model.data_usage_key)
edx.devstack.lms  | openedx.core.djangoapps.content.block_structure.exceptions.BlockStructureNotFound: Block structure not found; data_usage_key: block-v1:edX+DemoX+Demo_Course+branch@draft-branch+version@637e0bef5d758277968b2140+type@course+block@course
edx.devstack.lms  | 
edx.devstack.lms  | During handling of the above exception, another exception occurred:
edx.devstack.lms  | 
edx.devstack.lms  | Traceback (most recent call last):
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
edx.devstack.lms  |     response = get_response(request)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/base.py", line 181, in _get_response
edx.devstack.lms  |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
edx.devstack.lms  |     return view_func(*args, **kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/views/generic/base.py", line 70, in view
edx.devstack.lms  |     return self.dispatch(request, *args, **kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/utils/decorators.py", line 43, in _wrapper
edx.devstack.lms  |     return bound_method(*args, **kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/rest_framework/views.py", line 509, in dispatch
edx.devstack.lms  |     response = self.handle_exception(exc)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/rest_framework/views.py", line 506, in dispatch
edx.devstack.lms  |     response = handler(request, *args, **kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/rest_framework/generics.py", line 199, in get
edx.devstack.lms  |     return self.list(request, *args, **kwargs)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/lms/djangoapps/course_api/blocks/views.py", line 227, in list
edx.devstack.lms  |     get_blocks(
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/lms/djangoapps/course_api/blocks/api.py", line 122, in get_blocks
edx.devstack.lms  |     blocks = course_blocks_api.get_course_blocks(
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/lms/djangoapps/course_blocks/api.py", line 107, in get_course_blocks
edx.devstack.lms  |     return get_block_structure_manager(starting_block_usage_key.course_key).get_transformed(
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/manager.py", line 65, in get_transformed
edx.devstack.lms  |     block_structure = collected_block_structure.copy() if collected_block_structure else self.get_collected()
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/manager.py", line 105, in get_collected
edx.devstack.lms  |     block_structure = self._update_collected()
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/manager.py", line 128, in _update_collected
edx.devstack.lms  |     BlockStructureTransformers.collect(block_structure)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/transformers.py", line 78, in collect
edx.devstack.lms  |     transformer.collect(block_structure)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/lms/djangoapps/course_blocks/transformers/user_partitions.py", line 51, in collect
edx.devstack.lms  |     SplitTestTransformer.collect(block_structure)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/lms/djangoapps/course_blocks/transformers/split_test.py", line 45, in collect
edx.devstack.lms  |     root_block = block_structure.get_xblock(block_structure.root_block_usage_key)
edx.devstack.lms  |   File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content/block_structure/block_structure.py", line 815, in get_xblock
edx.devstack.lms  |     return self._xblock_map[usage_key]
edx.devstack.lms  | KeyError: BlockUsageLocator(CourseLocator('edX', 'DemoX', 'Demo_Course', 'draft-branch', ObjectId('637e0bef5d758277968b2140')), 'course', 'course')
edx.devstack.lms  | [23/Nov/2022 16:24:25] "GET /api/courses/v1/blocks/block-v1:edX+DemoX+Demo_Course+branch@draft-branch+version@637e0bef5d758277968b2140+type@vertical+block@4a1bba2a403f40bca5ec245e945b0d76?all_blocks=True&depth=all&requested_fields=children HTTP/1.1" 500 418770

I am using the key that is fetched in the get_all_xblocks_in_course to fetch get_xblocks. Let me know if I am missing something!

navinkarkera · 2022-11-24T06:35:26Z

@farhaanbukhsh This seems like an issue with course-blocks API. Anyway, the get_xblocks method will not be used to fetch data for draft version (we only want published versions) and even if it is accidentally used for a draft version, it logs an exception and returns data for all other passed items. Is that acceptable or do you feel differently?

navinkarkera · 2022-11-24T06:41:09Z

Also the test failing in CI seems to be passing locally and the changes in this MR should not have any effect on this test.

Please re-run the test if possible.

farhaanbukhsh

👍

I have added a few nits, but over all the PR looks good.

✅ I tested this: by running and testing the provider locally on the devstack.
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

mphilbrick211 · 2022-11-30T16:38:44Z

Hi @ansabgillani! Could you please take a look at this and allow tests to run?

mphilbrick211 · 2022-11-30T17:09:19Z

@navinkarkera looks like you have branch conflicts that need to be resolved to push this through.

navinkarkera · 2022-12-06T07:41:18Z

@mphilbrick211 @ansabgillani @sameenfatima78 This is now ready for merge (checked all required items in pre-merge checklist).

navinkarkera · 2022-12-06T13:40:22Z

@DawoudSheraz @Ali-D-Akbar Can you please help merge this MR.

navinkarkera · 2022-12-15T07:10:17Z

@DawoudSheraz @Ali-D-Akbar @mphilbrick211 @ansabgillani Please merge this MR if no further changes are required.

mphilbrick211 · 2022-12-21T19:41:50Z

Hi @navinkarkera! We're looking into having tests enabled for you.

mphilbrick211 · 2022-12-22T13:56:45Z

Hi @navinkarkera! Your tests were enabled, but you have some that failed and will need to be re-run. You'll also need to rebase to resolve the branch date issue.

navinkarkera · 2022-12-22T14:29:43Z

Hi @navinkarkera! Your tests were enabled, but you have some that failed and will need to be re-run. You'll also need to rebase to resolve the branch date issue.

@mphilbrick211 Thanks you. Fixed the quality issues, but the test is passing locally and it has no relation to this MR. Please re-run the tests.

mphilbrick211 · 2022-12-27T17:19:32Z

Hi @navinkarkera - I am unable to run the tests, but I believe you should be able to. If you are running into issues, please let me know.

navinkarkera · 2022-12-28T07:10:26Z

@mphilbrick211 I can only rebase with master but the workflow needs approval.

DawoudSheraz · 2022-12-28T07:14:52Z

@mphilbrick211 I can only rebase with master but the workflow needs approval.

I have given Approve and Run on this PR before. Not sure why it is asking again.

navinkarkera · 2022-12-28T14:27:03Z

@DawoudSheraz Thank you.

It did ran successfully.

But the master was updated and I had to rebase again. Someone would need to approve it again (till my MR is merged each run of workflow requires approval).

DawoudSheraz · 2022-12-28T15:15:09Z

+            'block-v1:edX+DemoX+Demo_Course+type@video+block@0b9e39477cf34507a7a48f74be381fdd',
+            'block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical_0270f6de40fc',
+        ]
+        resource = 'api/courses/v1/blocks/'


you can add a settings/constants variable for this. This has been used in client file too.

DawoudSheraz

A few nits, but overall good. A question: how/where will this be used? The blocks API is an important and sometimes expensive API. I am just wondering how often LMS will be hit to get blocks data.

test: add tests for xblock provider and block api refactor: lint issues and change api param refactor: make xblock supported types configurable refactor: use default partner id if available refactor: use block metadata api to fetch text content test: fix xblock provider test refactor: docstrings and conditions refactor: replace double quotes with single quotes refactor: extract vertical and video child blocks from given list of xblocks chore: upgrade deps for taxonomy-connector fix: preserve content text order fix: handle none values in index_dictionary

navinkarkera · 2022-12-28T16:44:36Z

A few nits, but overall good. A question: how/where will this be used? The blocks API is an important and sometimes expensive API. I am just wondering how often LMS will be hit to get blocks data.

@DawoudSheraz Whenever a block is published and when we run refresh_xblock_skills management command. Please find related MRs below:

openedx-webhooks · 2022-12-29T07:17:34Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 16, 2022

navinkarkera force-pushed the navin/xblock-metadata-provider branch 3 times, most recently from 8a3a256 to d9f2431 Compare November 21, 2022 14:24

navinkarkera commented Nov 22, 2022

View reviewed changes

navinkarkera force-pushed the navin/xblock-metadata-provider branch 3 times, most recently from c983097 to 13850e6 Compare November 23, 2022 09:15

farhaanbukhsh requested changes Nov 23, 2022

View reviewed changes

farhaanbukhsh approved these changes Nov 24, 2022

View reviewed changes

Comment thread course_discovery/apps/taxonomy_support/providers.py Outdated

Comment thread course_discovery/apps/taxonomy_support/providers.py

Comment thread course_discovery/apps/taxonomy_support/providers.py Outdated

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 6b073b3 to 7b0fa21 Compare November 24, 2022 12:48

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 7b0fa21 to 7a8bc89 Compare December 6, 2022 07:38

navinkarkera mentioned this pull request Dec 6, 2022

feat: hook xblock publish, delete and duplicate openedx-events openedx/openedx-platform#31350

Merged

1 task

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 7a8bc89 to 24571dd Compare December 6, 2022 10:03

sameenfatima78 approved these changes Dec 6, 2022

View reviewed changes

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 24571dd to 2f64860 Compare December 7, 2022 16:30

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 2f64860 to 59f61fd Compare December 15, 2022 07:08

navinkarkera force-pushed the navin/xblock-metadata-provider branch 2 times, most recently from 1dff86c to 6d29eb4 Compare December 21, 2022 11:48

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 21, 2022

mphilbrick211 added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Dec 22, 2022

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 6d29eb4 to aed9dcc Compare December 22, 2022 14:27

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 22, 2022

navinkarkera force-pushed the navin/xblock-metadata-provider branch from aed9dcc to f9317eb Compare December 22, 2022 14:27

navinkarkera force-pushed the navin/xblock-metadata-provider branch from f9317eb to 97cdc1c Compare December 28, 2022 07:09

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 97cdc1c to 0ba60b5 Compare December 28, 2022 14:25

DawoudSheraz reviewed Dec 28, 2022

View reviewed changes

DawoudSheraz approved these changes Dec 28, 2022

View reviewed changes

navinkarkera force-pushed the navin/xblock-metadata-provider branch from 0ba60b5 to 01aa707 Compare December 28, 2022 16:41

ansabgillani approved these changes Dec 28, 2022

View reviewed changes

DawoudSheraz approved these changes Dec 29, 2022

View reviewed changes

DawoudSheraz merged commit 1508c6f into openedx:master Dec 29, 2022

		taxonomy-connector @ git+https://github.com/open-craft/taxonomy-connector.git@5b9fedcc5c68370f3de5d49e18aae72d0d8226ad
		# FIXME: remove this once a new tag is pushed in taxaonomy-connector

Uh oh!

Conversation

navinkarkera commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Pre-merge checklist

Uh oh!

openedx-webhooks commented Nov 16, 2022

Uh oh!

mphilbrick211 commented Nov 22, 2022

Uh oh!

azanbinzahid commented Nov 22, 2022

Uh oh!

navinkarkera Nov 22, 2022 • 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!

navinkarkera Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

Ali-D-Akbar Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

navinkarkera Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

sameenfatima78 Nov 23, 2022 • 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!

navinkarkera Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

sameenfatima78 Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

farhaanbukhsh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farhaanbukhsh commented Nov 23, 2022

Uh oh!

navinkarkera commented Nov 24, 2022

Uh oh!

navinkarkera commented Nov 24, 2022

Uh oh!

farhaanbukhsh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mphilbrick211 commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mphilbrick211 commented Nov 30, 2022

Uh oh!

navinkarkera commented Dec 6, 2022

Uh oh!

navinkarkera commented Dec 6, 2022

Uh oh!

navinkarkera commented Dec 15, 2022

Uh oh!

mphilbrick211 commented Dec 21, 2022

Uh oh!

mphilbrick211 commented Dec 22, 2022

Uh oh!

navinkarkera commented Dec 22, 2022

Uh oh!

navinkarkera commented Nov 16, 2022 •

edited

Loading

navinkarkera Nov 22, 2022 •

edited

Loading

sameenfatima78 Nov 23, 2022 •

edited

Loading

mphilbrick211 commented Nov 30, 2022 •

edited

Loading