Eugeny/problem type search

[e-kolpakov]

Background: This PR is a reimplementation of https://github.com/edx/edx-platform/pull/6346 and contains the following:

• Capa problems search index support
• Machinery to create dedicated index for library contents
• Library capa problem type filtering reimplemented using search

Jira tickets: SOL-268, OC-557
Discussions: discussion on related PR, personal email to @martynjames
Dependencies: Search in multivalue fields - merged, #7313 - merged
Sandbox URL: LMS, Studio
Related: previous version
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Testing instructions: no user facing changes should appear. Test instructions similar to https://github.com/edx/edx-platform/pull/6346:

1. Create a library with a couple of Problem XBlocks of different types (e.g. "Checkboxes", "Dropdown", "Multiple Choice")
2. Add Library Content XBlock to a course.
3. Edit Library Content settings. By default "Problem Type" should be set to "Any Type", LMS view should display problems of any type.
4. Set "Problem Type" to one of the problem types in the library (e.g. "Checkboxes", "Dropdown" or "Multiple Choice""). Please note that some problem type templates fall into same problem type (e.g. "Custom Javascript/Python" and "Drag and Drop" fall into "Custom Evaluated Script"). Save, publish, navigate to LMS as a student and ensure only Problems of selected type are shown.
5. Set "Problem Type" to any problem type not in the library (e.g. any except "Checkboxes", "Dropdown" or "Multiple Choice""). Save, publish, note the warning message in Studio, navigate to LMS as a student and ensure no XBlocks are shown.

[openedx-webhooks]

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?number=7641&repo=edx%2Fedx-platform

[e-kolpakov]

@martynjames you might want to take a look at this - I'm planning this to reach a ready-for-formal-upstream-review status tomorrow, but I would appreciate any early feedback.

[martynjames]

Requires a rebase, which would also get rid of the additional commits that end up herein tag have effectively been committed (although squashed).... (It may be easier to cherry-pick your commits on top of them). Also, can you confirm the story under which these changes are being done - SOL-286 appears to be in the backlog...

[openedx-webhooks]

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=7641

[antoviaque]

@martynjames For the ticket, it was a typo in the header here, although the link was good - fixed, the ticket is https://openedx.atlassian.net/browse/SOL-268

[martynjames]

sp - category

[martynjames]

Looks really good - starts to complicate my next alteration to add in the course about information, but not too much. Will look again after the rebase has been performed.

[e-kolpakov]

@martynjames @Kelketek rebased + fixed failing tests; please review

[Kelketek]

Of these classmethods, are all subclasses expected to implement them? Any methods which need to be implemented by all subclasses should use the @abstractmethod decorator, as is used in the modulestore.

[martynjames]

@e-kolpakov - LGTM 👍

[Kelketek]

Is there a particular reason this comment is needed?

[martynjames]

Not necessarily, it can be removed safely - the same/similar comment was present in the equivalent course test, and is equally irrelevant therein. Its origin comes from testing the test - update_search_index is called from listen_for_course_publish (and I assume listen_for_library_update)

[e-kolpakov]

@Kelketek I'll keep this comment just to avoid removing the "source" one a couple of lines above (the one @martynjames was talking about) - that commit was added in upstream master so there must be some value.

[martynjames]

As I stated above, there is no value to the comment; or the original.

[Kelketek]

@e-kolpakov The library at http://content-libraries.sandbox.opencraft.com:18010/library/library-v1:Opencraft+New1 contains a Dropdown, but the RCB configured for Dropdowns at http://content-libraries.sandbox.opencraft.com:18010/container/block-v1:Arbi+101+2015+type@vertical+block@fb0ff738e6744e148bca0f5f7e1a1e0b says there are no Dropdown problems. Could you take a look at this?

[e-kolpakov]

@Kelketek lib indexing occurs when lirary is updated. NewLib was created before indexing was in place, and no one have updated it (before I took a look at it), so there were no index records for it. I've triggered an index update for it and now it works fine. Although, it might make sense to add a "migration" of some kind to (re)index existing libraries - I believe it is possible to trigger reindexing course manually.

Also, I've used up my stock of "it" words, so I'll have to do without them for some time :)

[e-kolpakov]

@Kelketek @martynjames I've followed the path of least resistance and added a management command to manually reindex libraries. I don't think adding a button in Studio to reindex library is the right way to solve the problem of "cold-start", since libraries search indices are always automatically updated when library is modified. So, the only way the library could have missing/outdated search index entries is when search indexing is switched into "enabled" position - this is definitely a maintenance task. That's why, in my opinion, adding a one-time maintenance step makes more sense than supporting manual reindexing - code changes for manual reindexing are spread over UI, js and django views (all of which needs test coverage), while maintenance command consists of single file.

[Kelketek]

@e-kolpakov http://content-libraries.sandbox.opencraft.com/courses/course-v1:Arbi+101+2015/courseware/7710378ebe20477282082d0cdb6ac7a8/85664a69508947409f250e4bd561468f/1 doesn't show anything, despite having a few RCBs and a text item. Could you take a look at this and verify the static assets have been compiled and are working right?

[Kelketek]

@e-kolpakov At this point the code looks good to me. While I'd still like you to remove that code zombie requirement, as long as I can verify it works as expected, it will have my 👍 . Since the sandbox isn't working correctly, I'm going to work on something else and come back to this later when I have time to take a peek and see if I can get it working before you wake up again.

[Kelketek]

@e-kolpakov I've tried forcing the rebuild of static files but this didn't solve the issue. You may have something bigger on your hands. Please take a look as soon as you're able to.

[e-kolpakov]

@Kelketek looks like the sandbox is toast - it doesn't show any content in any course. This is not the case for local instances of course. Most likely sandbox needs rebuilding, I recall @antoviaque said something about it being broken. Could you please check it out in your local dev environment while we fix sandbox issues?

[Kelketek]

@e-kolpakov I've verified this works locally, and the current code looks good to me. As discussed over IRC, however, we do still need tests for the management command. Let me know as soon as you have these.

[antoviaque]

@e-kolpakov If a sandbox is misbehaving, you can simply switch to another one (maybe sandbox4.opencraft.com?) and mark the broken sandbox as needing a reprovisioning on the allocation page - I've added to my todo to reset content-libraries.sandbox.opencraft.com when I get a chance.

[e-kolpakov]

@Kelketek tests for the management command added.

@martynjames there are some minor changes since your review - you might want to check them out.

@marcotuts could you please review this? This is likely the last iteration before merging (sandbox updated)

[e-kolpakov]

Jarvis, test this please

[Kelketek]

@e-kolpakov 👍 (And there was much rejoicing! :)

[marcotuts]

Awesome! Thanks so much for the detailed testing instructions as well, that was very straightforward. +1 from a product perspective.

[e-kolpakov]

@marcotuts thank you! I believe the established practice is to use 👍 emoticon to indicate that one completely satisfied with what (s)he have seen and gives "formal" permission to merge the PR. We're not robots though, so I'm pretty sure you meant just that and merge this.