feat: index library collections in meilisearch [FC-0062] by navinkarkera · Pull Request #35324 · openedx/openedx-platform

navinkarkera · 2024-08-19T14:47:29Z

Description

Updates rebuild_index function to include collections for indexing in meilisearch.

Supporting information

Testing instructions

Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin
Make sure to create few libraries
Run below snippet to add some collections

import random
from openedx_learning.api import authoring as authoring_api
from openedx_learning.api import authoring_models
learning_packages = authoring_models.LearningPackage.objects.all()
for i in range(20):
    learning_id = random.choice(learning_packages)
    authoring_api.create_collection(learning_id, f"collection_{i}", None, f'description_{i}')

Run tutor dev run cms ./manage.py cms reindex_studio --experimental.
Open http://meilisearch.local.edly.io:7700/
Search for collection and verify the contents of collection documents.

Deadline

None

openedx-webhooks · 2024-08-19T14:47:34Z

Thanks for the pull request, @navinkarkera!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

rpenido · 2024-08-20T16:04:15Z

-    status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses and libraries.")
+        ############## Collections ##############
+        status_cb("Indexing collections...")
+        # To reduce memory usage on large instances, split up the Collections into pages of 100 collections:


👍 Great!

rpenido · 2024-08-21T16:27:36Z

 openedx-events                      # Open edX Events from Hooks Extension Framework (OEP-50)
 openedx-filters                     # Open edX Filters from Hooks Extension Framework (OEP-50)
-openedx-learning                    # Open edX Learning core (experimental)
+# FIXME Remove this line after release of openedx-learning


rpenido · 2024-08-21T16:32:32Z

                num_blocks_done += len(docs)

-    status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses and libraries.")
+        ############## Collections ##############


This is starting to get really big.

We have a lot of local variables here, but could you try to create some inner functions like config_index(), index_libraries(), index_courses(), and index_collections()?

@rpenido Not sure if it improves readability but converted some parts to inner functions.

rpenido

LGTM 👍
Thank you for your work, @navinkarkera!

There is a small nit in the testing instructions.

I tested this using the instructions from the PR, using the updated snippet:

import random
from openedx_learning.api import authoring as authoring_api
from openedx_learning.api import authoring_models
learning_packages = authoring_models.LearningPackage.objects.all()
for i in range(20):
    learning_id = random.choice(learning_packages).id
    authoring_api.create_collection(learning_id, f"collection_{i}", None, f'description_{i}')

I read through the code
~~I checked for accessibility issues~~
Includes documentation

Not blocking, but I think a small refactor in our indexing function would be great.

Also, we could add the description field to the searchable attributes. What do you think?

navinkarkera · 2024-08-22T09:32:10Z

@rpenido Thanks!

There is a small nit in the testing instructions.

There was an update to openedx_learning after I created this PR so missed to update the instructions. Thanks!

Also, we could add the description field to the searchable attributes. What do you think?

Updated.

rpenido

Thank you @navinkarkera!

@pomegranited This is ready for CC review.

pomegranited

I ran this and it works perfectly, using your test instructions, thank you @navinkarkera :)

But I've got a concern about this line that I'd like addressed before approving here.

pomegranited · 2024-08-23T05:16:09Z

+        Fields.context_key: collection.learning_package.key,
+    }
+    # Just in case learning_package is not related to a library
+    if hasattr(collection.learning_package, 'contentlibrary'):


Is this actually the way to check if a learning package has an associated content library? It doesn't seem right..
Could you add a test for the case where a collection is associated with a learning package that is not associated with a content library, so we can be sure this logic is correct?

@pomegranited That is a good point. I am not really sure if we have a scenario where a learning_package is not related to a library, maybe a course?

Ya, within the context of openedx, a collection should always be related to a learning_package with a content library (courses don't have collections). I just think the hasattr mechanism is weird, and may always be true, even if collection.learning_package.contentlibrary == None.

@pomegranited Ohh, I added a test with a learning package that is not associated with a library. The hasattr mechanism worked but like you said it is weird and may not work in case the attr is none, so updated to make use of try..catch DoesNotExist exception.

pomegranited

👍 Works perfectly, thank you @navinkarkera !

I also tested with your latest change on openedx-learning.

I tested this using the PR test instructions (with one change, line 6 of the code sample should be learning_id = random.choice(learning_packages).id)
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation
~~User-facing strings are extracted for translation~~ N/A

Co-authored-by: Jillian <jill@opencraft.com>

openedx-webhooks · 2024-08-27T17:45:49Z

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

edx-pipeline-bot · 2024-08-27T19:30:31Z

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

edx-pipeline-bot · 2024-08-27T19:53:20Z

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

edx-pipeline-bot · 2024-08-27T19:53:20Z

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

…35324)

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

navinkarkera force-pushed the navin/collection_indexing branch 4 times, most recently from 4aa6025 to af40d47 Compare August 20, 2024 13:51

navinkarkera changed the title ~~feat: index library collections in meillisearch~~ feat: index library collections in meilisearch Aug 20, 2024

navinkarkera changed the title ~~feat: index library collections in meilisearch~~ feat: index library collections in meilisearch [FC-0062] Aug 20, 2024

navinkarkera force-pushed the navin/collection_indexing branch 3 times, most recently from 8ce6b33 to 3777566 Compare August 20, 2024 14:00

navinkarkera mentioned this pull request Aug 20, 2024

feat: add get_collections api openedx/openedx-core#214

Merged

navinkarkera force-pushed the navin/collection_indexing branch from 3777566 to 885ee47 Compare August 20, 2024 14:12

rpenido reviewed Aug 20, 2024

View reviewed changes

navinkarkera force-pushed the navin/collection_indexing branch 2 times, most recently from 523fab9 to d605a5c Compare August 21, 2024 11:01

navinkarkera marked this pull request as ready for review August 21, 2024 11:01

navinkarkera force-pushed the navin/collection_indexing branch from 2d5d1f7 to 47db5d1 Compare August 21, 2024 14:23

navinkarkera requested a review from rpenido August 21, 2024 15:00

rpenido reviewed Aug 21, 2024

View reviewed changes

navinkarkera force-pushed the navin/collection_indexing branch 2 times, most recently from da19109 to c1edbb0 Compare August 22, 2024 09:30

navinkarkera requested a review from rpenido August 22, 2024 13:52

rpenido approved these changes Aug 22, 2024

View reviewed changes

navinkarkera force-pushed the navin/collection_indexing branch from c1edbb0 to 51ba403 Compare August 22, 2024 15:14

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Aug 22, 2024

pomegranited requested changes Aug 23, 2024

View reviewed changes

navinkarkera added 9 commits August 23, 2024 11:25

feat: index library collections in meillisearch

d48ae03

test: add collection indexing test

9e6dc43

fix: lint issues

7769685

refactor: include library details with collection

9ee0bd8

test: update collection indexing tests

88783ba

fix: lint issues

18c7d07

build: point to temporary openedx-learning branch [REMOVE before merge]

ca0c8ce

refactor: rebase with latest changes

61c8715

refactor: rebuild_index function

59e26aa

navinkarkera force-pushed the navin/collection_indexing branch from 51ba403 to 0e87179 Compare August 23, 2024 05:57

refactor: collection with unrelated library

7664e02

navinkarkera force-pushed the navin/collection_indexing branch from 0e87179 to 7664e02 Compare August 23, 2024 06:06

pomegranited approved these changes Aug 23, 2024

View reviewed changes

Comment thread openedx/core/djangoapps/content/search/documents.py Outdated

refactor: warning text

af577d6

Co-authored-by: Jillian <jill@opencraft.com>

pomegranited force-pushed the navin/collection_indexing branch from c38a198 to ed66903 Compare August 27, 2024 05:33

chore: use openedx-learning==0.11.2

2ac22c3

pomegranited force-pushed the navin/collection_indexing branch from ed66903 to 2ac22c3 Compare August 27, 2024 05:44

bradenmacdonald merged commit c65478e into openedx:master Aug 27, 2024

bradenmacdonald deleted the navin/collection_indexing branch August 27, 2024 17:45

rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Aug 27, 2024

feat: index library collections in studio meilisearch index (openedx#…

2c66cf0

…35324)

ormsbee mentioned this pull request Sep 27, 2024

XBlock asset support for new libraries experience (WIP) #35557

Closed

Uh oh!

Conversation

navinkarkera commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

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!

rpenido left a comment

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Aug 22, 2024

Uh oh!

rpenido left a comment

Choose a reason for hiding this comment

Uh oh!

pomegranited 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!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openedx-webhooks commented Aug 27, 2024

Uh oh!

edx-pipeline-bot commented Aug 27, 2024

Uh oh!

edx-pipeline-bot commented Aug 27, 2024

Uh oh!

edx-pipeline-bot commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

navinkarkera commented Aug 19, 2024 •

edited

Loading

openedx-webhooks commented Aug 19, 2024 •

edited

Loading