Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete hinter manager. #11752

Merged
merged 2 commits into from
Mar 7, 2016
Merged

Delete hinter manager. #11752

merged 2 commits into from
Mar 7, 2016

Conversation

cahrens
Copy link

@cahrens cahrens commented Mar 4, 2016

I came upon this code because I wanted to delete js/course_groups/cohorts.js, which is oddly imported in hint_manager.html (I can't see how the hint manager is at all related to cohorts).

This "instructor view" (not integrated into an instructor dashboard) was written for the crowdsource prototype xmodule by Felix Sun (an intern) in 2013-- #303. It does not appear to be enabled on edx or edge (following the URL pattern indicated in comments and urls.py).

@pmitros and I chatted about the old crowdsource xmodule. It can be deleted, except for one small test case that @pmitros would like to move out of the old code and preserve. There is an active PR to introduce a new crowdsource xblock, #9095.

@nedbat I don't think this needs to be mentioned in Eucalyptus due to its history.

@nedbat
Copy link
Contributor

nedbat commented Mar 4, 2016

Is this something we should mention in Eucalyptus notes?

@pmitros
Copy link
Contributor

pmitros commented Mar 4, 2016

The old crowdsourced hinter code is no longer maintained and it is now slightly bit rotted. The only reason it is still in place is because it tests logger.log() and logger.listen(). That's an important test case to have, since it is part of our XBlocks client API, and at least some open source XBlocks do rely on it. There isn't a good reason to couple having that test case to the somewhat large and complex pieces of code that is the CSH.

This PR will take a bit rotted no-longer-functional piece of code, and replaces it with half of a bit rotted no-longer-functional piece of code. I don't think that's a positive step in of itself. People sometimes decide to clean up code with bitrot. If half of it is ripped out, it's a bit of a trap; after such a cleanup, you still end up with a non-working system.

It would be cleaner to just rip out the entire CSH XModule (as opposed to just the instructor view), and replace it with a small, independent, uncoupled test case which makes sure that logger.log and logger.listen both continue to work on the client side (which is the only function this code still serves). That would be a major code cleanup, major code reduction, and I suspect not that much work.

(Note that the above is neither a 👍 or a 👎. I don't have enough context on what level of code quality we aim for at this point)

@cahrens
Copy link
Author

cahrens commented Mar 4, 2016

The bok choy test failure is a known flaky bug.

@cahrens
Copy link
Author

cahrens commented Mar 4, 2016

@robrap and @dianakhuang Please code review the deletion of this file.

I created a techdebt story for the removal of the rest of the crowdsource module xblock. I imagine we would need to make sure no courses are including it, and add it to the list of deprecated xblocks. https://openedx.atlassian.net/browse/TNL-4195

@dianakhuang
Copy link
Contributor

👍 Works for me.

@pmitros
Copy link
Contributor

pmitros commented Mar 4, 2016

@cahrens I don't believe removing the overall module would require that much work. This was a prototype used in one place in one course. I just confirmed this with a SQL query. This was never an edX feature.

If you wanted to do this right -- just add a minimal test case for logger.log and logger.listen, I'm happy to put my own neck on the line for skipping all of that process.

@pmitros
Copy link
Contributor

pmitros commented Mar 4, 2016

(specifically, the last time a hint was contributed was 2013-07-28)

@cahrens
Copy link
Author

cahrens commented Mar 4, 2016

It turns out that the logger already has dedicated unit tests, see https://github.com/edx/edx-platform/blob/master/common/static/js/spec/logger_spec.js. So I am proceeding with deleting all parts of crowdsource_xmodule.

@@ -24,6 +24,9 @@
return {
/**
* Emits an event.
*
* Note that this method is used by external XBlocks, and the API cannot change without
Copy link
Author

Choose a reason for hiding this comment

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

@cahrens
Copy link
Author

cahrens commented Mar 4, 2016

Bok choy test failure is related to a known flaky issue, https://openedx.atlassian.net/browse/TNL-4151 (which is in our current sprint!).

@robrap and @dianakhuang Please review the complete deletion of crowdsource module. @pmitros, if you care to give a thumbs-up it can stand in for one of the other reviews!

@pmitros
Copy link
Contributor

pmitros commented Mar 4, 2016

👍 I'd suggest (not require) similar comments around the test cases as well (no need to re-review). Thank you for taking the lead on this.

@cahrens
Copy link
Author

cahrens commented Mar 6, 2016

Bok choy failure is a known issue, in our current sprint. @dianakhuang please re-review now that this includes the complete deletion.

@dianakhuang
Copy link
Contributor

👍

cahrens pushed a commit that referenced this pull request Mar 7, 2016
@cahrens cahrens merged commit dbc2b41 into master Mar 7, 2016
@benpatterson benpatterson deleted the christina/remove-hinter branch August 2, 2016 13:05
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.

4 participants