Skip to content

fix: do not index solutions in CAPA blocks - #29106

Merged
Agrendalath merged 1 commit into
openedx:masterfrom
open-craft:agrendalath/bb-4994-capa_block_indexing
May 10, 2022
Merged

fix: do not index solutions in CAPA blocks#29106
Agrendalath merged 1 commit into
openedx:masterfrom
open-craft:agrendalath/bb-4994-capa_block_indexing

Conversation

@Agrendalath

@Agrendalath Agrendalath commented Oct 25, 2021

Copy link
Copy Markdown
Member

Description

There are some tags in the Problem XBlock that can contain solutions or hints. Most of them are already being removed, but the current regex does not include the case when the tags have attributes. This decreases the possibility of indexing solutions by expanding the regex.

Supporting information

OSPR ticket: OSPR-6173
OpenCraft Jira ticket: BB-4994

Known issues

The ElasticSearch connection was not working for me with the most recent devstack version. I had to add this to lms/envs/private.py and cms/envs/private.py:

ELASTIC_SEARCH_CONFIG = [
    {
        'use_ssl': False,
        'host': 'edx.devstack.elasticsearch710',
        'port': 9200
    }
]

Testing instructions

  1. Enable FEATURES['ENABLE_COURSEWARE_SEARCH'] in LMS.
  2. Enable FEATURES['ENABLE_COURSEWARE_INDEX'] in Studio.
  3. Create a Blank Advanced Problem XBlock with the following content (it will not render correctly, but don't worry about it):
     <problem>
       You should not
       <solution>Test solution.</solution>
       <solution explanation-id="solution0">Test solution with attribute.</solution>
       <solutionset>
         Test solutionset.
       <solution explanation-id="solution1">Test solution within solutionset.</solution>
       </solutionset>
       <targetedfeedback>Test feedback.</targetedfeedback>
       <targetedfeedback explanation-id="feedback0">Test feedback with attribute.</targetedfeedback>
       <targetedfeedbackset>
         Test FeedbackSet.
         <targetedfeedback explanation-id="feedback1">Test feedback within feedbackset.</targetedfeedback>
       </targetedfeedbackset>
       <answer>Test answer.</answer>
       <answer type="loncapa/python">Test answer with attribute.</answer>
       <script>Test script.</script>
       <script type="loncapa/python">Test script with attribute.</script>
       <style>Test style.</style>
       <style media="all and (max-width: 1920px)">Test style with attribute.</style>
       <choicehint>Test choicehint.</choicehint>
       <hint>Test hint.</hint>
       <hintpart>Test hintpart.</hintpart>
       see anything else.
     </problem>
  4. Click the "Reindex" button on the Course Outline page in Studio (just in case).
  5. Go to your dashboard in LMS and search for "anything else". You should see the following result: "You should not see anything else.".

Deadline

"None"

Reviewers

Settings

EDXAPP_FEATURES:
  ENABLE_COURSEWARE_SEARCH: true
  ENABLE_COURSEWARE_INDEX: true

@openedx-webhooks openedx-webhooks added core committer open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 25, 2021
@openedx-webhooks

openedx-webhooks commented Oct 25, 2021

Copy link
Copy Markdown

Thanks for the pull request, @Agrendalath! I've created OSPR-6173 to keep track of it in JIRA, where we prioritize reviews. 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.

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4994-capa_block_indexing branch from 84ddc6d to 10d5b6a Compare October 25, 2021 19:59
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@Agrendalath
Agrendalath marked this pull request as ready for review October 25, 2021 22:48
@natabene

Copy link
Copy Markdown
Contributor

@Agrendalath Thank you for your contribution. Is this ready for our review?

@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, we'd like to do our internal review first. I'll ping you on this one and on openedx/edx-search#116 once we finish it.

@alfredchavez

alfredchavez commented Nov 1, 2021

Copy link
Copy Markdown
Contributor

👍🏽

  • I tested this: checked that the code works as expected(following described steps)
  • I read through the code

@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, this is ready for your review.
cc: @giovannicimolin, would you mind adding this to the OSPR list?

@mariajgrimaldi

Copy link
Copy Markdown
Member

Hi! I could review this one @natabene

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4994-capa_block_indexing branch from 10d5b6a to ce2c8d6 Compare April 20, 2022 19:42
@openedx-webhooks openedx-webhooks removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. core committer labels Apr 20, 2022
@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4994-capa_block_indexing branch from ce2c8d6 to cbc1313 Compare April 20, 2022 22:51
@natabene

Copy link
Copy Markdown
Contributor

@mariajgrimaldi Thanks for letting me know, checking with the owning team now.

@mariajgrimaldi

Copy link
Copy Markdown
Member

Hi there! This is what I've tested:

First, I created a course with Blank Advanced Problem XBlock containing the problem mentioned above
Then:

Before this PR

I reindex my courses and then searched for anything else:
image

After PR:

I had the same result as before if I didn't reindex my course. After reindexing:
image

All courses should be reindexed when we merge this PR, right? so it takes effect. How should we manage this?

cc @natabene

@natabene

Copy link
Copy Markdown
Contributor

@jristau1984 Looks like this might be good to, can you give a blessing before @mariajgrimaldi can merge?

Most tags that could contain solutions or hints were already being removed,
but the regex did not include the case when they contained attributes.
@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4994-capa_block_indexing branch from cbc1313 to 4ece091 Compare April 29, 2022 12:53
@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, confirmed on Slack. Merging.

@openedx-webhooks

Copy link
Copy Markdown

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath
Agrendalath deleted the agrendalath/bb-4994-capa_block_indexing branch May 10, 2022 17:04
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants