Felix/studio hints #764

fephsun · 2013-08-21T19:09:28Z

Adds studio support for the crowdsourced hinting xmodule.

Also adds instructor documentation for crowdsourced hinting.

Slater-Victoroff · 2013-08-21T19:17:44Z

common/lib/capa/capa/responsetypes.py

+        """
+        return compare_with_tolerance(
+            evaluator({}, {}, a),
+            evaluator({}, {}, b),


It probably makes sense to use dict() instead of {} here mostly because that's what you're using everywhere else just for consistency's sake. It's also about 33% more efficient, but that's not really important since it's so fast.

valera-rozuvan · 2013-08-27T13:15:01Z

@fephsun Please look into what is causing the build to report as unstable. Waiting for https://github.com/edx/edx-platform/pull/442 to be merged into master so that this PR branch can be rebased against master with PR 442 stuff in it.

- Cherry-picked onto main hinting branch. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py

the id of each problem in Studio - this is visually ugly, and a temporary fix. - Cherrypicked into main hinting branch. Conflicts: cms/djangoapps/contentstore/views/component.py

- Cherry-picked onto main hinter branch. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py

fephsun · 2013-08-27T17:49:46Z

@valera-rozuvan Fixed instability in 442, merged 442, and rebased new master onto this branch.

Waiting for a code review on this branch now.

target problem choices. Added a url for the hint manager in studio.

Removed a test factory that wasn't being used.

… one problem on a page. Added instructor-facing documentation. Added graceful failure if we can't find parent module of hinter.

valera-rozuvan · 2013-08-28T11:52:12Z

@jkarni Can you please review this PR from the point of view of new XBlocks Studio integration/architecture. I am not very familiar with the new XBlocks. But from the comment https://github.com/edx/edx-platform/pull/804#issuecomment-23383966 I think this is a relevant request.

singingwolfboy · 2013-08-28T13:38:55Z

common/lib/xmodule/xmodule/crowdsource_hinter.py

+    Used by studio to get all the problems on the same page as the hinter.
+    """
+    global problem_choices
+    return problem_choices


Why is this global variable necessary? If the variable is necessary, why is this function necessary, when all it does is return the variable?

The global variable is needed because problem_choices needs to be fed to the xmodule field before we get a chance to populate problem_choices.

I'm no longer certain that the function is necessary. I will look into it.

Actually, yeah, the function is necessary. Otherwise, the xmodule field gets a separate copy of problem_choices than the xmodule, and it's not possible for me to change the xmodule field's problem_choices.

Pass-by-value vs. pass-by-reference is hard.

cahrens · 2013-08-28T13:42:51Z

You should add an instance of crowdsource hinter to one of our test courses that we use for import/export. That way we can make sure it imports and exports OK.

See common\test\data. There are courses for testing conditional, openended, word_cloud, etc. Then /home/christina/mitx_all/edx-platform/common/lib/xmodule/xmodule/tests/test_import.py and /home/christina/mitx_all/edx-platform/common/lib/xmodule/xmodule/tests/test_export.py.

singingwolfboy · 2013-08-28T13:47:40Z

common/lib/xmodule/xmodule/crowdsource_hinter.py

+            if parent is not None:
+                break
+        if parent is None:
+            # Weird... not supposed to happen.  Just get out.


Massive red flag here. At the very least, this should log a warning.

Also, instead of the double-break, I'd suggest writing a generator and looping over it, something like this:

def parent_and_child(modules): """Generate parent/child pairs""" for parent in modules: for child in parent.get_children(): yield parent, child def get_parent(child, modules): """Return the parent of the given child by looping over all parent/child pairs in modules.""" for candidate_parent, candidate_child in parent_and_child(modules): if child.location == candidate_child.location: return candidate_parent parent = get_parent(self, all_modules)

But you should also check if there is a more efficient way of doing this.

Getting rid of the double-break. But, no we don't have any better way of finding the parent of a module :( It's really annoying.

@singingwolfboy Getting this strange feeling of deja vu...

Isn't silently returning if you can't find the parent a bad idea? You're using this function for its side-effects, and there's no way to tell if the function succeeded from the outside right now.

I just posted that function as an example. If you want, you could add a line at the end that raises some sort of exception (maybe a ValueError?) if it reaches that point.

I was referring to the original code, not your suggestion. Sorry for the confusion.

cahrens · 2013-08-28T13:52:20Z

Should update test_advanced_components_in_edit_unit in /home/christina/mitx_all/edx-platform/cms/djangoapps/contentstore/tests/test_contentstore.py.

singingwolfboy · 2013-08-28T13:52:41Z

common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py

+    def get_items(self, item):
+        descriptor = MagicMock()
+        descriptor.name = 'capa'
+        return [descriptor]


More concise and better specified:

from xmodule.modulestore import ModuleStoreBase FakeModulestore = Mock(spec=ModuleStoreBase) FakeModuleStore.get_items.return_value = [MagicMock(name='capa')]

cahrens · 2013-08-28T13:53:47Z

I would recommend a Lettuce test that covers editing a crowdsource hinter instance in Studio.

singingwolfboy · 2013-08-28T13:54:05Z

common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py

+
+# Patch the crowdsourced hinter module from here.
+with patch('xmodule.modulestore.django.modulestore', new=FakeModulestore):
+    from xmodule.crowdsource_hinter import CrowdsourceHinterModule


This seems really odd. Why would you patch outside of an actual test case? What's the point?

cahrens · 2013-08-28T13:56:58Z

Do you intend anything to be edited in the XML Editor view of the instance in Studio? Assuming you do not, you should be using MetadataOnlyEditingDescriptor so the XML editor view is not presented.

singingwolfboy · 2013-08-28T14:00:27Z

common/lib/xmodule/xmodule/crowdsource_hinter.py

+        child_url = self.problem_module.system.ajax_url
+
+        # Return a little section for displaying hints.
+        out = '<section class="crowdsource-wrapper" data-url="' + self.system.ajax_url +\
            '" data-child-url = "' + child_url + '"> </section>'


This is begging for a mini template. I'd suggest something like:

out = '<section class="crowdsource-wrapper" data-url="{url}" data-child-url="{child_url}"> </section>'.format( url=self.system.ajax_url, child_url=child_url)

singingwolfboy · 2013-08-28T14:16:24Z

common/lib/xmodule/xmodule/crowdsource_hinter.py

+            responder = self.problem_module.lcp.responders.values()[0]
+        except IndexError:
+            self.init_error = 'The problem you specified does not have any response blanks!'
+            return


If there errors are being displayed to the user, they'll need to be wrapped in gettext for i18n purposes.

cahrens · 2013-08-28T14:20:24Z

It looks like there are some pylint/pep8 things to be cleaned up.

http://jenkins.edx.org:8080/job/edx-feature-branch-tests/11088/Diff_Quality_Report/?

cahrens · 2013-08-28T14:21:38Z

According to this, crowdsource_hinter.py only has 45.2% unit test coverage.

http://jenkins.edx.org:8080/job/edx-feature-branch-tests/11088/Diff_Coverage_Report/?

fephsun · 2013-08-28T16:14:38Z

@rocha @valera-rozuvan @Lyla-Fischer

Found a pretty big problem with this PR: the instructor hint manager breaks in mongo courses. The reason is that the hint manager directly queries the sql database for hints, but in mongo courses, hints live in a mongo database instead. I feel pretty embarrassed that I didn't find this earlier :(

I can fix the hint manager today, but a whole bunch of tests will be broken after that, and it doesn't look like I can fix everything before I leave.

jkarni · 2013-08-28T16:36:54Z

common/lib/xmodule/xmodule/crowdsource_hinter.py

+        global problem_choices
+        """
+        Create a list of problems within this section, to offer as choices for the
+        target problem.


Still don't entirely understand why you're using global. I prefer returning a value and then having the caller set the variable to that value. Makes it much easier to test/reason about independently.

A whole bunch of tests are now broken.

…e assertions away from sql.

fephsun · 2013-08-28T20:53:39Z

So, I finished fixing the instructor view bug, but some tests are still broken.

Today was my last day as a formal edX intern. I am subscribed to this PR, so I can answer questions, but I won't have access to a dev environment.

@rocha will be finishing this up after he gets back from vacation, I think.

singingwolfboy · 2013-10-07T18:55:45Z

What's the status of this pull request? @rocha, are you going to finish it up? Or should we just close it?

rocha · 2013-10-07T19:49:32Z

It doesn't look like I am going to have time to work on this any time soon.

@Lyla-Fischer does it make sense to make an entry in the Blades team icebox for follow up someday?

Lyla-Fischer · 2013-10-08T14:44:54Z

Yes. Will do.

Lyla-Fischer · 2013-10-08T14:45:46Z

https://edx-wiki.atlassian.net/browse/BLD-235

…caching Also make the actual ?next redirection work with caching

…ests Ziafazal/yonk 494 tests

Bumping Rocket chat to v0.2.8

Slater-Victoroff reviewed Aug 21, 2013
View reviewed changes

fephsun added 4 commits August 27, 2013 13:47

Removed parent-child relationship for crowdsource_hinter.

923662c

- Cherry-picked onto main hinting branch. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py

It is now possible to add hints from Studio. Instructors can see

f568995

the id of each problem in Studio - this is visually ugly, and a temporary fix. - Cherrypicked into main hinting branch. Conflicts: cms/djangoapps/contentstore/views/component.py

Drop-down menu for selecting target problem for hinter.

a93a928

- Cherry-picked onto main hinter branch. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py

Feeding a list of problems to Studio is now less hacky.

efde0b9

fephsun added 3 commits August 27, 2013 14:41

In the studio editor, only problems on the same page are displayed as

3ba4e1c

target problem choices. Added a url for the hint manager in studio.

Fixed tests for studio hint authoring.

65046e3

Removed a test factory that wasn't being used.

Fixed bug: Impossible to pick hinting target problem if there is only…

07ca075

… one problem on a page. Added instructor-facing documentation. Added graceful failure if we can't find parent module of hinter.

singingwolfboy reviewed Aug 28, 2013
View reviewed changes

jkarni reviewed Aug 28, 2013
View reviewed changes

fephsun added 2 commits August 28, 2013 15:10

Fixed instructor hint manager so that it works with mongo courses.

a5e35bf

A whole bunch of tests are now broken.

Fixing tests for hint management. Basically done - just need to chang…

6edb2fe

…e assertions away from sql.

rocha closed this Oct 7, 2013

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#764 from MITx/feature/victor/fix-index-js-…

3358eba

…caching Also make the actual ?next redirection work with caching

benpatterson deleted the felix/studio-hints branch January 21, 2015 13:14

itsjeyd pushed a commit to open-craft/edx-platform that referenced this pull request Feb 10, 2017

Merge pull request openedx#764 from edx-solutions/ziafazal/YONK-494-t…

cb9d0e6

…ests Ziafazal/yonk 494 tests

andrey-canon pushed a commit to eduNEXT/edx-platform that referenced this pull request Jun 21, 2018

Merge pull request openedx#764 from proversity-org/master

d72df75

Bumping Rocket chat to v0.2.8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Felix/studio hints #764

Felix/studio hints #764

fephsun commented Aug 21, 2013

Slater-Victoroff Aug 21, 2013

valera-rozuvan commented Aug 27, 2013

fephsun commented Aug 27, 2013

valera-rozuvan commented Aug 28, 2013

singingwolfboy Aug 28, 2013

fephsun Aug 28, 2013

fephsun Aug 28, 2013

cahrens commented Aug 28, 2013

singingwolfboy Aug 28, 2013

fephsun Aug 28, 2013

jkarni Aug 28, 2013

jkarni Aug 28, 2013

singingwolfboy Aug 28, 2013

jkarni Aug 28, 2013

cahrens commented Aug 28, 2013

singingwolfboy Aug 28, 2013

cahrens commented Aug 28, 2013

singingwolfboy Aug 28, 2013

cahrens commented Aug 28, 2013

singingwolfboy Aug 28, 2013

singingwolfboy Aug 28, 2013

cahrens commented Aug 28, 2013

cahrens commented Aug 28, 2013

fephsun commented Aug 28, 2013

jkarni Aug 28, 2013

fephsun commented Aug 28, 2013

singingwolfboy commented Oct 7, 2013

rocha commented Oct 7, 2013

Lyla-Fischer commented Oct 8, 2013

Lyla-Fischer commented Oct 8, 2013

Felix/studio hints #764

Felix/studio hints #764

Conversation

fephsun commented Aug 21, 2013

Choose a reason for hiding this comment

valera-rozuvan commented Aug 27, 2013

fephsun commented Aug 27, 2013

valera-rozuvan commented Aug 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 28, 2013

Choose a reason for hiding this comment

cahrens commented Aug 28, 2013

Choose a reason for hiding this comment

cahrens commented Aug 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 28, 2013

cahrens commented Aug 28, 2013

fephsun commented Aug 28, 2013

Choose a reason for hiding this comment

fephsun commented Aug 28, 2013

singingwolfboy commented Oct 7, 2013

rocha commented Oct 7, 2013

Lyla-Fischer commented Oct 8, 2013

Lyla-Fischer commented Oct 8, 2013