Skip to content

Add experimental crowdsource hinter module - #275

Closed
rocha wants to merge 15 commits into
hotfix/felix/crowdsource-hinterfrom
felix/hinter2
Closed

Add experimental crowdsource hinter module#275
rocha wants to merge 15 commits into
hotfix/felix/crowdsource-hinterfrom
felix/hinter2

Conversation

@rocha

@rocha rocha commented Jun 26, 2013

Copy link
Copy Markdown
Contributor

No description provided.

Felix Sun and others added 9 commits June 21, 2013 14:35
Conflicts:
	common/static/coffee/src/logger.coffee
Added moderation feature - you can now choose to hold all hints for moderator approval before showing.
… the crowdsource hinter module. (Old tests no longer cover all the code, now that moderation has been added.)
…ier to read. Fixed database non-initialization bug in crowdsource hinter module.
Fixed some bugs in the tests for crowdsourced hinter.
Hinter now displays vote count after voting.

Began testing templates.
Chiseled a little at writing template tests.
@rocha

rocha commented Jun 26, 2013

Copy link
Copy Markdown
Contributor Author

@fephsun I am going to do a first review here

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.

Add an intro docstring, describing what the module does in very general terms. Also make add a line at the beginning that highlights that the module is experimental and should not be used to create content at the moment (because we may change things, etc).

@wedaly

wedaly commented Jun 26, 2013

Copy link
Copy Markdown
Contributor

Diff coverage:
common/lib/xmodule/xmodule/crowdsource_hinter.py 77%
lms/djangoapps/instructor/hint_manager.py 22%

Please add more test coverage before merging.

@rocha

rocha commented Jun 26, 2013

Copy link
Copy Markdown
Contributor Author

@wedaly was your coverage message automated? Cool!

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.

We are breaking early because we only expect one child, correct? If so add a little comment. If we are only expecting one child we should add that to the class documentation.

@rocha

rocha commented Jun 26, 2013

Copy link
Copy Markdown
Contributor Author

This is a meta comment, so I am putting it here. I normally find very useful to add some blank lines to separate logical blocks of code in function. It improves readability a bit (I read somewhere there was a study showing that it improved code comprehension), but it is also a good indicator of when you have to refactor a function. If there are too many logical blocks within a function, it is asking to be broken down and refactored into smaller methods. Here is an example I found with a quick search: http://programmers.stackexchange.com/a/17325 (see how the bad whitespace example gets refactored down).

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.

minor: make_capa_answer_hashable?

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.

Could you add a comment on why you access [answer][pk][1]?

Began enforcing one-vote-per-person.  This can be disabled with debug="True" in the <crowdsource_hinter> tag.

Started tests of the hint manager.

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.

Let me see if I follow, problem_id is the definition_id of the hinter module, correct? If so, the variable name makes it a bit confusing.

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.

You can use .on instead of .live. I think .live has been deprecated. (Check if the version of jQuery that we use supports it first).

@rocha

rocha commented Jun 26, 2013

Copy link
Copy Markdown
Contributor Author

@fephsun looks like some tests are failing now.

@chrisndodge

Copy link
Copy Markdown
Contributor

Hi a drive-by comment, if you don't mind. Have you thought about authoring via Studio yet? I see some sample XML which wraps a new XModule around a Problem XModule, so there could be complexities with how this will be authored in Studio, which is used by a vast majority of course creators.

If not, you might want to drop @markchang @cahrens a quick note to discuss.

Thanks, apologies for the intrusion.

…anager.

Expanded tests of hint_manager.

Enabled the hint_manager by default in development environments.
@rocha

rocha commented Jun 27, 2013

Copy link
Copy Markdown
Contributor Author

@chrisndodge Thanks for stopping by!

At this stage, the module it is just an experiment. We want to collect some data and verify if the students will actually use something like this, and if so, how. I don't recommend that we make it available since it is likely to change.

Unfortunately, in our current architecture we don't have a way to run experiments, or add experimental features without adding them to the main code base.

@fephsun fephsun closed this Jun 28, 2013
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Added contact info on Berkeley press release.
e-kolpakov referenced this pull request in open-craft/openedx-platform Oct 21, 2014
hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Oct 6, 2015
diegomillan referenced this pull request in eduNEXT/edx-platform Sep 14, 2016
douglashall pushed a commit that referenced this pull request Jun 5, 2018
restricted oauth changes & migration files
MAbdurrehman12 pushed a commit to MAbdurrehman12/edx-platform that referenced this pull request Dec 22, 2021
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
* Commits:
  Submit events to the tracking logs, not just Segment
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