-
Notifications
You must be signed in to change notification settings - Fork 4.3k
TNL-930: Turn off Student Notes when Harvard Annotation Tool is enabled. #6340
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -323,13 +323,27 @@ def generate_uid(): | |
|
|
||
| def is_feature_enabled(course): | ||
| """ | ||
| Returns True if the edxnotes app is enabled for the course, False otherwise. | ||
| Returns True if Student Notes feature is enabled for the course, | ||
| False otherwise. | ||
|
|
||
| In order for the app to be enabled it must be: | ||
| In order for the application to be enabled it must be: | ||
| 1) enabled globally via FEATURES. | ||
| 2) present in the course tab configuration. | ||
| 3) Harvard Annotation Tool must be disabled for the course. | ||
| """ | ||
| tab_found = next((True for t in course.tabs if t["type"] == "edxnotes"), False) | ||
| feature_enabled = settings.FEATURES.get("ENABLE_EDXNOTES") | ||
|
|
||
| return feature_enabled and tab_found | ||
| return (feature_enabled and tab_found) and not is_harvard_notes_enabled(course) | ||
|
|
||
|
|
||
| def is_harvard_notes_enabled(course): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about just using Harvard's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It doesn't check for 'textannotation', 'imageannotation', 'videoannotation' fields. So, we cannot use it. |
||
| """ | ||
| Returns True if Harvard Annotation Tool is enabled for the course, | ||
| False otherwise. | ||
|
|
||
| Checks for 'textannotation', 'imageannotation', 'videoannotation' in the list | ||
| of advanced modules of the course. | ||
| """ | ||
| modules = set(['textannotation', 'imageannotation', 'videoannotation']) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @polesye There is a fourth field, http://www.equtek.com/clients/edX/annotation/setup.php I do not know if it will work independently if none of 'textannotation', 'imageannotation', 'video annotation' are present. But perhaps we should also add it to the fields?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looks like they are independent modules. I do not see any reasons to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
OK, thanks for looking into it. |
||
| return bool(modules.intersection(course.advanced_modules)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,25 +104,33 @@ def test_edxnotes_enabled(self, mock_generate_uid, mock_get_id_token, mock_get_t | |
| @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) | ||
| def test_edxnotes_disabled_if_edxnotes_flag_is_false(self): | ||
| """ | ||
| Tests if get_html is wrapped when feature flag is on, but edxnotes are | ||
| Tests that get_html is wrapped when feature flag is on, but edxnotes are | ||
| disabled for the course. | ||
| """ | ||
| self.assertEqual("original_get_html", self.problem.get_html()) | ||
|
|
||
| @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False}) | ||
| def test_edxnotes_disabled(self): | ||
| """ | ||
| Tests if get_html is not wrapped when feature flag is off. | ||
| Tests that get_html is not wrapped when feature flag is off. | ||
| """ | ||
| self.assertEqual("original_get_html", self.problem.get_html()) | ||
|
|
||
| def test_edxnotes_studio(self): | ||
| """ | ||
| Tests if get_html is not wrapped when problem is rendered in Studio. | ||
| Tests that get_html is not wrapped when problem is rendered in Studio. | ||
| """ | ||
| self.problem.system.is_author_mode = True | ||
| self.assertEqual("original_get_html", self.problem.get_html()) | ||
|
|
||
| def test_edxnotes_harvard_notes_enabled(self): | ||
| """ | ||
| Tests that get_html is not wrapped when Harvard Annotation Tool is enabled. | ||
| """ | ||
| self.course.advanced_modules = ["videoannotation", "imageannotation", "textannotation"] | ||
| enable_edxnotes_for_the_course(self.course, self.user.id) | ||
| self.assertEqual("original_get_html", self.problem.get_html()) | ||
|
|
||
|
|
||
| @skipUnless(settings.FEATURES["ENABLE_EDXNOTES"], "EdxNotes feature needs to be enabled.") | ||
| class EdxNotesHelpersTest(ModuleStoreTestCase): | ||
|
|
@@ -179,6 +187,22 @@ def test_edxnotes_not_enabled(self): | |
| self.course.tabs = [] | ||
| self.assertFalse(helpers.is_feature_enabled(self.course)) | ||
|
|
||
| def test_edxnotes_harvard_notes_enabled(self): | ||
| """ | ||
| Tests that edxnotes are disabled when Harvard Annotation Tool is enabled. | ||
| """ | ||
| self.course.advanced_modules = ["foo", "imageannotation", "boo"] | ||
| self.assertFalse(helpers.is_feature_enabled(self.course)) | ||
|
|
||
| self.course.advanced_modules = ["foo", "boo", "videoannotation"] | ||
| self.assertFalse(helpers.is_feature_enabled(self.course)) | ||
|
|
||
| self.course.advanced_modules = ["textannotation", "foo", "boo"] | ||
| self.assertFalse(helpers.is_feature_enabled(self.course)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @polesye is checking a position of harvard module is required here or it possible to check only when harvard is present in advanced modules? like here: harvard_modules = ["videoannotation", "imageannotation", "textannotation"]
for module in harvard_modules:
self.course.advanced_modules = [module]
self.assertFalse(helpers.is_feature_enabled(self.course))
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@olmar I want to make sure that position and other modules doesn't affect the result. |
||
|
|
||
| self.course.advanced_modules = ["textannotation", "videoannotation", "imageannotation"] | ||
| self.assertFalse(helpers.is_feature_enabled(self.course)) | ||
|
|
||
| def test_edxnotes_enabled(self): | ||
| """ | ||
| Tests that edxnotes are enabled when the course tab configuration contains | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@polesye I would rather write:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.