Skip to content

Delete hinter manager. - #11752

Merged
cahrens merged 2 commits into
masterfrom
christina/remove-hinter
Mar 7, 2016
Merged

Delete hinter manager.#11752
cahrens merged 2 commits into
masterfrom
christina/remove-hinter

Conversation

@cahrens

@cahrens cahrens commented Mar 4, 2016

Copy link
Copy Markdown

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

nedbat commented Mar 4, 2016

Copy link
Copy Markdown
Contributor

Is this something we should mention in Eucalyptus notes?

@pmitros

pmitros commented Mar 4, 2016

Copy link
Copy Markdown
Contributor

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

cahrens commented Mar 4, 2016

Copy link
Copy Markdown
Author

The bok choy test failure is a known flaky bug.

@cahrens

cahrens commented Mar 4, 2016

Copy link
Copy Markdown
Author

@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
Copy Markdown
Contributor

👍 Works for me.

@pmitros

pmitros commented Mar 4, 2016

Copy link
Copy Markdown
Contributor

@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

pmitros commented Mar 4, 2016

Copy link
Copy Markdown
Contributor

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

@cahrens
cahrens force-pushed the christina/remove-hinter branch from 8dfc31b to b57cd35 Compare March 4, 2016 18:41
@cahrens

cahrens commented Mar 4, 2016

Copy link
Copy Markdown
Author

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.

@cahrens
cahrens force-pushed the christina/remove-hinter branch from b57cd35 to 59f9076 Compare March 4, 2016 18:47
Comment thread common/static/js/src/logger.js Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@cahrens
cahrens force-pushed the christina/remove-hinter branch from 59f9076 to d2db58b Compare March 4, 2016 20:35
@cahrens

cahrens commented Mar 4, 2016

Copy link
Copy Markdown
Author

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

pmitros commented Mar 4, 2016

Copy link
Copy Markdown
Contributor

👍 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
cahrens force-pushed the christina/remove-hinter branch from d2db58b to 5432ef3 Compare March 5, 2016 22:14
@cahrens

cahrens commented Mar 6, 2016

Copy link
Copy Markdown
Author

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
Copy Markdown
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