Skip to content

[WIP] Mjevtic/sol 536 implement experiment aware content search - #8100

Closed
marjev wants to merge 4 commits into
masterfrom
mjevtic/SOL-536-implement-experiment-aware-content-search
Closed

[WIP] Mjevtic/sol 536 implement experiment aware content search#8100
marjev wants to merge 4 commits into
masterfrom
mjevtic/SOL-536-implement-experiment-aware-content-search

Conversation

@marjev

@marjev marjev commented May 19, 2015

Copy link
Copy Markdown
Contributor

@martynjames

This PR replaces #8084

Latest commit addresses the comments you left on the #8084

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @marjev! 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=8100&repo=edx%2Fedx-platform

Comment thread cms/djangoapps/contentstore/__init__.py Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@martynjames Can you put some light on why this is necessary here? With this line in __init__.py, I can't import the constant SPLIT_TEST_COMPONENT_TYPE.

This is just a temporary solution. I would like to get your opinion on the matter since I saw that you've added this as a part of #7313.

@marjev marjev changed the title Mjevtic/sol 536 implement experiment aware content search [WIP] Mjevtic/sol 536 implement experiment aware content search May 19, 2015

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.

Is this really the only way to get group_id? Splitting display name strings by space?

@marjev
marjev force-pushed the mjevtic/SOL-536-implement-experiment-aware-content-search branch 2 times, most recently from 6f4724e to 27c3bc3 Compare May 21, 2015 09:45

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

∃ a constant in cms/djangoapps/contentstore/views/component.py as follows:
SPLIT_TEST_COMPONENT_TYPE = 'split_test' - I would use this reference

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to resort to that, but I experience the circular dependancy issue.

I'll try to think of a way to avoid that issue.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, yes - it's certainly not worth the circular dependency!

@martynjames

Copy link
Copy Markdown

@marjev - A couple of problems I found while testing...
i) Support for searching AB blocks within dashboard - either we need to surpress them, or include the content_groups identifiers in the filter for all courses. (I started to take a stab at this here if it helps - edx@c36e386).
ii) I added a breakpoint to see if I get any results that I need to exclude (Ideally this should not happen) - have not delved deep into this, but maybe this is the Experiment + Cohort case (I have it setup here locally); either way worth investigating.

@marjev

marjev commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

@martynjames I'll look into that, thanks for the ground work, I'm sure it will prove to be helpful

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @marjev! 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=8100

@dino-cikatic

Copy link
Copy Markdown
Contributor

@marjev I believe my latest commit should take care of split_test name and group id issue.

@dino-cikatic
dino-cikatic force-pushed the mjevtic/SOL-536-implement-experiment-aware-content-search branch from bfe8fa7 to 27d1661 Compare May 27, 2015 06:37
@dino-cikatic
dino-cikatic force-pushed the mjevtic/SOL-536-implement-experiment-aware-content-search branch from 27d1661 to 5554e3c Compare May 27, 2015 07:13
@marjev

marjev commented May 28, 2015

Copy link
Copy Markdown
Contributor Author

Closing this PR since it's replace by #8229.

@marjev marjev closed this May 28, 2015
@marjev
marjev deleted the mjevtic/SOL-536-implement-experiment-aware-content-search branch May 28, 2015 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants