fix: added content search to lms installed apps

[Asespinel]

Description

This PR adds openedx.core.djangoapps.content.search to INSTALLED_APPS in lms/envs/common.py to restore the functionality of the Feedback XBlock tab in the Instructor tab.

The Feedback XBlock tab relies on the get_lms_link_for_item utility, which internally depends on openedx.core.djangoapps.content.search

Testing instructions

1.Install FeedbackXblock
2. Add the following patch as an inline plug-in in tutor.

  openedx-common-settings: |
    OPEN_EDX_FILTERS_CONFIG = {
      "org.openedx.learning.instructor.dashboard.render.started.v1": {
        "fail_silently": False,
        "pipeline": [
          "feedback.extensions.filters.AddFeedbackTab",
        ]
      },
    }

3. Activate the Tutor inline plug-in.
4. Go to the Instructor tab and feedback tab in the course with a feedback xblock

Deadline

None

[openedx-webhooks]

Thanks for the pull request, @Asespinel!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

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

Details

---

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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.

[Asespinel]

Hi @pomegranited! 👋
I know you've been working on something similar around the feedback tab, so I thought you might be the perfect person to review this. When you get a chance, could you please take a look at this PR? Thanks so much!

[pomegranited]

Hi @Asespinel :)

I know you've been working on something similar around the feedback tab, so I thought you might be the perfect person to review this. When you get a chance, could you please take a look at this PR? Thanks so much!

My apologies, but I won't have time to look at this for a few weeks.

@mphilbrick211 Could you help us find a reviewer?

[mphilbrick211]

@openedx/wg-build-test-release-security-patchers would someone be able to take a look at this?

[mariajgrimaldi]

@Asespinel: I don't really understand how these imports are related to the Feedback Xblock. Can you elaborate further how the using the block raises those errors? Also, I do think the safest approach would be to find the root of the problem, instead of adding try-excepts to avoid the runtime issues. Can we do that instead?

[Asespinel]

@Asespinel: I don't really understand how these imports are related to the Feedback Xblock. Can you elaborate further how the using the block raises those errors? Also, I do think the safest approach would be to find the root of the problem, instead of adding try-excepts to avoid the runtime issues. Can we do that instead?

Hi majo, I hope everything is well. The problem occurs because the search model isn't registered in the INSTALLED APPS of the LMS. If we don't use those import catchins, we get a Runtime error when going into the instructor tab:

The platform import get_lms_link_for_item was added to the plugin in December 2023:
openedx/FeedbackXBlock@92d381b#diff-4b3d83ec4e1ea8ea32243cca19c4efc9f44f0d7fbc4da5fcd6b645199c0bcefcR12-R16

When the plugin imports this function, it indirectly triggers a chain of imports:

get_lms_link_for_item →

cms/djangoapps/contentstore/toggles.py →

content/search/api.py →

And finally, it tries to import this model: SearchAccess

That last import is where the error happens.

The SearchAccess model was only added in April 2024:
d672110#diff-75df6268aac78fef857f1c970d53931b93209f2f0fb019d17a83f906749c1bffR13-R15

That’s why this import didn’t fail in Redwood, but it does fail in Sumac, since SearchAccess didn’t exist in Redwood.

Now, after adding these changes we don't get that error and I see the course feedback tab correctly.

[magajh]

@Asespinel just curious — could you share a bit more context on why these changes to the contentstore module are needed?

[magajh]

Do we also need to move the index_config imports inside the try/except block? Or is it enough to just wrap the models imports? when are the imports from index_config getting triggered?

[magajh]

@mariajgrimaldi as @Asespinel explained in more detail, we recently discovered that the SearchAccess model — which was added in 2024 — ends up being imported indirectly by a module used in the Feedback XBlock, and, since the content.search app is not included in the LMS INSTALLED_APPS, attempting to access the feedback tab in the instructor dashboard raises an import error. This seems to be the root cause of the issue.

We considered two possible solutions:

1. Use a try/except block (as was done when the model was originally introduced, specifically to avoid import errors in other modules d672110#diff-95b1d167a319870ade835b889d600b6e46a5fcc44b127c421ee03f8a5719f36eR12).
2. Add the app to the LMS INSTALLED_APPS.

Given that some imports were wrapped in a try/except when the model was originally introduced, we thought following that pattern here would be appropriate. That said, we’d love to hear what you think about the approach we're proposing — or if you believe there’s a better way to solve the issue

[mariajgrimaldi]

Thank you both for the context, @magajh @Asespinel! What I'd like to understand is why adding SearchAccess app wasn't an option when this implementation was first introduced. I see that the pattern was introduced but in test files, so I'd like to look more into other options.

I'll tag @rpenido and @pomegranited in case they have more insight on this issue. In the meantime, I think about this more thoroughly. Thanks!

[kdmccormick]

I would rather avoid this approach. Wrapping imports in try-except increases the complexity of the entire module. It creates two realities (one where the classes are defined, and another where the classes aren't) and our unit tests will most likely cover only one of those realities, leaving the other reality untested. Same for our manual named-release tests.

Is there a reason not to add content.search to the INSTALLED_APPS?

[Asespinel]

I would rather avoid this approach. Wrapping imports in try-except increases the complexity of the entire module. It creates two realities (one where the classes are defined, and another where the classes aren't) and our unit tests will most likely cover only one of those realities, leaving the other reality untested. Same for our manual named-release tests.

Is there a reason not to add content.search to the INSTALLED_APPS?

You're absolutely right—wrapping imports in try-except can introduce unnecessary complexity and lead to inconsistent test coverage. I agree that adding content.search to INSTALLED_APPS is a cleaner and more reliable approach.

If everyone is on board with this direction, I'm happy to go ahead and make that change. Let me know if there are any concerns before I proceed.

[mariajgrimaldi]

@Asespinel: yes, I think that would be the cleanest approach. Thank you all :)

[Asespinel]

Done @mariajgrimaldi @kdmccormick ! Please let me know if you need some changes.

[kdmccormick]

Please update the PR title, commit message, and description.

[Asespinel]

@kdmccormick I updated the descriptiosn as requested, I also created a PR with this change for teak here: #36864. Please let me know if you require more changes. Thanks!

[kdmccormick]

@bradenmacdonald You originally wrote the search app and added it to CMS's installed apps list. Before we merge, is there any reason we should not install this app into LMS as well?

[bradenmacdonald]

@kdmccormick The search app is really just designed to work in CMS at the moment. I am not sure if it will directly cause any problems by adding it to the LMS, but I think it would give the false impression that it's "meant" to be used in an LMS context, which it's really not.

There is a content search feature in the LMS but it needs a complete overhaul at some point, and when that work is done we would decide if we're expanding content.search to support LMS use cases or implementing a separate content.learner_search app. The LMS permissions model is much more complicated than the Studio one, and search works totally differently in each case so I don't want people to conflate them.

we recently discovered that the SearchAccess model — which was added in 2024 — ends up being imported indirectly by a module used in the Feedback XBlock, and, since the content.search app is not included in the LMS INSTALLED_APPS, attempting to access the feedback tab in the instructor dashboard raises an import error. This seems to be the root cause of the issue.

The Feedback XBlock should not be importing get_lms_link_for_item. It's an internal CMS API, and not part of the XBlock API at all. You can also easily replicate it's functionality without importing anything: url = f"/courses/{block.course_key}/jump_to/{block.location}". If you need an absolute URL, just copy the entire code of get_lms_link_for_item into your XBlock, or add a proper XBlock service for getting block URLs (which we should have anyways).

There are other problematic imports in FeedbackXBlock that are totally breaking separation of concerns, but maybe that's necessary for it to work at all. So my recommendation is just to re-implement get_lms_link_for_item in the block or replace it with an even simpler version as shown above.

[bradenmacdonald]

What I'd like to understand is why adding SearchAccess app wasn't an option when this implementation was first introduced.

I don't quite understand the question here but i'm happy to answer any questions about SearchAccess. It's just used to map each course/library to an integer ID which makes Studio search permissions work more efficiently. If your test case is not trying to update the studio search index (and it shouldn't be), there's no reason it should be dealing with SearchAccess at all.

[kdmccormick]

The Feedback XBlock should not be importing get_lms_link_for_item. It's an internal CMS API, and not part of the XBlock API at all. You can also easily replicate it's functionality without importing anything: url = f"/courses/{block.course_key}/jump_to/{block.location}". If you need an absolute URL, just copy the entire code of get_lms_link_for_item into your XBlock, or add a proper XBlock service for getting block URLs (which we should have anyways).

Great, that sounds like the right fix.

The search app is really just designed to work in CMS at the moment. I am not sure if it will directly cause any problems by adding it to the LMS, but I think it would give the false impression that it's "meant" to be used in an LMS context, which it's really not.

Okay, was it known from this start that this app was only meant to be used in the CMS? The best way to indicate that would have been putting it in the cms directory rather than the openedx directory.

[bradenmacdonald]

Okay, was it known from this start that this app was only meant to be used in the CMS? The best way to indicate that would have been putting it in the cms directory rather than the openedx directory.

That makes sense to me, but I've always operated based on this guidance which totally contradicts that. (And cms is a mess.)

https://github.com/openedx/edx-platform/blob/b6cec3c67ede90270af8e18ffe92ca0c1c7d0e72/openedx/README.rst#L4-L11

[kdmccormick]

@bradenmacdonald That's fair. That README guidance is very misleading. Here's a PR to update it.

cms is mess

I know cms/djangoapps/contenstore is way too big, but even so, it's really nice to have CMS-specific code sequestered into the cms root directory. We even have some importlinter contracts to help (partially) enforce that. It would be cool to move that search app to cms one day. Even content_libraries probably belongs under cms (#33428)

[Asespinel]

@kdmccormick
Hi team,
I’ve prepared a PR in the Feedback XBlock that separates it from platform core dependencies by re-implementing the required logic internally. openedx/FeedbackXBlock#166 This avoids importing from CMS-only apps like content.search and ensures compatibility across both LMS and Studio. Ready for review when you get the chance! Thanks for all the feedback.

[kdmccormick]

Closed in favor of openedx/FeedbackXBlock#166