Skip to content

fix: do not decorate ELLIPSIS with <b> tags - #123

Merged
idegtiarov merged 2 commits into
openedx:masterfrom
open-craft:agrendalath/bb-5589-fix_invalid_match_decorations
Apr 8, 2022
Merged

fix: do not decorate ELLIPSIS with <b> tags#123
idegtiarov merged 2 commits into
openedx:masterfrom
open-craft:agrendalath/bb-5589-fix_invalid_match_decorations

Conversation

@Agrendalath

@Agrendalath Agrendalath commented Mar 21, 2022

Copy link
Copy Markdown
Member

Multiple matches are merged with ELLIPSIS as a separator.
This reverses the order of operations so that the matches are decorated before they are merged. This way, the content of ELLIPSIS is not decorated with HTML <b> tags.

Jira ticket (nonpublic): BB-5589

Testing instructions

  1. Add the following to {lms,cms}/envs/private.py:
    from .common import FEATURES
    
    FEATURES['ENABLE_COURSEWARE_SEARCH'] = True
    FEATURES['ENABLE_COURSEWARE_SEARCH_FOR_COURSE_STAFF'] = True
    FEATURES['ENABLE_DASHBOARD_SEARCH'] = True
    FEATURES['ENABLE_COURSE_DISCOVERY'] = True
    FEATURES['ENABLE_COURSEWARE_INDEX'] = True
    FEATURES['ENABLE_LIBRARY_INDEX'] = True
    
    ELASTIC_SEARCH_CONFIG = [
        {
            'use_ssl': False,
            'host': 'edx.devstack.elasticsearch710',
            'port': 9200
        }
    ]
  2. Go to the demo course in Studio and click the "Reindex" button.
  3. Go to the Dashboard in LMS and type Passing a course into the search field.
  4. Results should not contain broken HTML code.

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Agrendalath! I've created OSPR-6537 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.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Mar 21, 2022
@Agrendalath
Agrendalath force-pushed the agrendalath/bb-5589-fix_invalid_match_decorations branch from 9c4269a to 74fd412 Compare March 21, 2022 14:45
Multiple matches are merged with `ELLIPSIS` as a separator.
This reverses the order of operations so that the matches are decorated before
they are merged. This way, the content of `ELLIPSIS` is not decorated with HTML
`<b>` tags.
Collections Abstract Base Classes are deprecated in `collections` since
Python 3.3. They will be moved to `collections.abc` after Python 3.8.
https://docs.python.org/3.8/library/collections.html
@Agrendalath
Agrendalath force-pushed the agrendalath/bb-5589-fix_invalid_match_decorations branch from 74fd412 to af021c9 Compare March 21, 2022 14:54
Comment thread search/utils.py

import importlib
import collections
from collections.abc import Iterable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just included this change because it was breaking tests for me locally. We don't worry about compatibility for Python versions older than 3.3, so this is a safe replacement.

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Working as described.

Before applying PR After applying PR
Screenshot_20220322_102202 Screenshot_20220322_102245
  • I tested this: tested this on master branch on devstack
  • I read through the code

@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, this is ready for your review.

@natabene

Copy link
Copy Markdown

@Agrendalath Thank you!

@sarina

sarina commented Apr 7, 2022

Copy link
Copy Markdown
Contributor

@idegtiarov - this ticket was tagged for Core Contributor review, but you are the only CC on this repo. Would you be available to do a review?

@idegtiarov

Copy link
Copy Markdown

@sarina thank you for notifying me, I've missed this PR. Will take a look.

@sarina

sarina commented Apr 7, 2022

Copy link
Copy Markdown
Contributor

@idegtiarov no worries, we started assigning CCs specifically for review only a few days ago. You can always check which PRs that 2U wants us to review at this link: https://openedx.atlassian.net/issues/?jql=project%20%3D%20OSPR%20AND%20status%3D%22Open%20edX%20Community%20Review%22

@idegtiarov idegtiarov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@idegtiarov

Copy link
Copy Markdown

@sarina one question: could I expect the author will merge his PR or I should do that?

@sarina

sarina commented Apr 8, 2022

Copy link
Copy Markdown
Contributor

@idegtiarov - this is tough because it's not clear how to tell who has commit rights to what repo. Only CCs and 2U people can commit to repos. In this case @Agrendalath is a CC but not to this repo (you can see the list here: https://openedx.atlassian.net/wiki/spaces/COMM/pages/3156344833/Current+Core+Contributors+to+the+Open+edX+Project), so you should merge in this case.

I think in general if you're not sure, you can approve and ask the author if they are able to merge or if they'd like you to do it.

@idegtiarov
idegtiarov merged commit 3cef5ef into openedx:master Apr 8, 2022
@idegtiarov

Copy link
Copy Markdown

@sarina got it, thanks.

PR is Merged.

@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

Copy link
Copy Markdown
Member Author

Thank you for handling this, @sarina.

@idegtiarov, thank you for the review. If you wouldn't mind, please publish a new GitHub release. It will publish the package to PyPI.

@Agrendalath
Agrendalath deleted the agrendalath/bb-5589-fix_invalid_match_decorations branch April 8, 2022 13:39
@idegtiarov

Copy link
Copy Markdown

@Agrendalath new release is published https://github.com/openedx/edx-search/tree/v3.3.0

salman2013 pushed a commit that referenced this pull request Jun 21, 2023
* fix: do not decorate `ELLIPSIS` with `<b>` tags

Multiple matches are merged with `ELLIPSIS` as a separator.
This reverses the order of operations so that the matches are decorated before
they are merged. This way, the content of `ELLIPSIS` is not decorated with HTML
`<b>` tags.

* fix: use correct module for Iterable

Collections Abstract Base Classes are deprecated in `collections` since
Python 3.3. They will be moved to `collections.abc` after Python 3.8.
https://docs.python.org/3.8/library/collections.html
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.

6 participants