Dcikatic/sol 536 [WIP]

[dino-cikatic]

@martynjames @marjev I've merged your work into this PR. I've tried testing with different state accounts and it looks fine. My only concern is that we have to get whole course object for each enrolled course and filter it for content groups. I imagine these can get to double digits fairly easily.

Ticket with acceptance criteria: https://openedx.atlassian.net/browse/SOL-536

[openedx-webhooks]

Thanks for the pull request, @dino-cikatic! 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=8229

[dino-cikatic]

Latest changes here should also fix search on future course discovery feature. There is a branch rebased on feature branch here:
https://github.com/edx/edx-platform/tree/dcikatic/SOL-536-course-discovery

[martynjames]

This needs to be .append here

[martynjames]

the if partition_group piece here seems to be unnecessary; we're already filtering out the None results above.

The original implementation of get_content_groups was like this:

        def get_content_groups(course, user):
            """ Collect content groups for user for this course """
            partition_groups = [
                get_group_for_user_partition(user_partition, course.id, user)
                for user_partition in course.user_partitions
                if user_partition.scheme in INCLUDE_SCHEMES
            ]
            content_groups = [unicode(partition_group.id) for partition_group in partition_groups if partition_group]
            return content_groups if content_groups else None

Seems like you thought that that was difficult to read, but need a little extra effort to ensure that the outcome is the same.

[dino-cikatic]

Original implementation wasn't working because in case group was None it would throw NoneType not iterable error. This happens in cases where group is not automatically assigned.
(I agree, if partition_group is not needed any more)

[dino-cikatic]

Hi @cahrens T&L is set as a second reviewer on this PR. Any chance someone can take a look?
Thank you.

[martynjames]

👍 LGTM @dino-cikatic! - this code works as desired, and corrects my earlier comment on dashboard search - assuming that tests pass...

[cahrens]

@dino-cikatic we need a green checkmark and a working sandbox (with latest code) before we review.

[dino-cikatic]

@cahrens I've setup a sandbox here: http://pr8229.m.sandbox.edx.org
There is a split test set in a last section of edX Demonstration Course and it has already been indexed. Feel free to add more. (standard credentials apply)

[cahrens]

@andy-armstrong this is ready for review

[andy-armstrong]

@dino-cikatic @martynjames I'm not sure if I'm doing something wrong, but I'm able to see content for experiment groups that I don't belong to. When I go to this part of my test course I am in the 'dog' cohort:

http://pr8229.m.sandbox.edx.org/courses/AndyA/AA101/1/courseware/32e0e89c9a2544d2b545deea2dcfa768/364e05b169554f79acf07067260f27a7/

However, I can search for 'siamese' and see the cat content that should be hidden from me.

[andy-armstrong]

Note that the children of a split test do not have to be verticals. There are examples of MIT-built XML courses that use split tests around other kinds of children (I believe chapters). The key thing is that there is a mapping from group to child in the split_test xblock, and that this group applies to the child and all of its descendants.

[andy-armstrong]

Just to clarify: the vertical convention is something that we did in Studio to make it simple to manage a collection of children shown to a particular group. The underlying platform doesn't make such an assumption.