annotation tools #2188

danielcebrian · 2014-01-16T00:05:57Z

To add these components to your course in Studio:

Installing Modules:

Add "notes", "textannotation" and "videoannotation" to advanced_modules in Studio's Advanced Settings. Now on the unit page you can see the Advanced components.
In the same advanced settings. It is necessary to add the annotation backend url and the annotation consumer key.
2.1.) Type a url with the annotation external storage under “annotation_storage_url”
2.2.) Type a consumer key for the annotation external storage under “annotation_token_secret”

Using Modules:

When creating new unit, you can find Text and Video annotation modules under “Advanced” component
Make sure you have either Text or Video in one unit, but not both.
Annotations are only allowed on Live/Public version and not Studio.
The video can not be opened in the cms (only in the lms) with youtube video.

Will also need to specify an annotation_storage_url and token_secret in Advanced Settings. @danielcebrian @lduarte1991 can provide this information.

jzoldak · 2014-01-16T21:46:43Z

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -41,6 +41,7 @@ class InheritanceMixin(XBlockMixin):
        scope=Scope.settings,
    )
    xqa_key = String(help="DO NOT USE", scope=Scope.settings)
+    annotation_storage_url = String(help="Location of Annotation backend", scope=Scope.settings, default="http://your_annotation_storage.com", display_name="annotation_storage_url")


@Cale or @nedbat does it make sense to be adding annotation_storage_url here in inheritance.py?

To explain a little of what we were hoping to do: We want there to be an field in the Advanced Tools section where we can change the address to the server that will hold annotations and not build one into the code for obvious security issues.

When we added this line of code it allowed for this to occur and in videoannotation_module.py we are able to call it using self.annotation_storage_url during live testing, but not while running the unit test.

danielcebrian · 2014-01-17T11:58:22Z

I have created test files and I have already rebased the code to the last edx version in order to be merged

cahrens · 2014-01-17T17:44:41Z

cms/envs/common.py

@@ -65,7 +65,7 @@

    # If set to True, Studio won't restrict the set of advanced components
    # to just those pre-approved by edX
-    'ALLOW_ALL_ADVANCED_COMPONENTS': False,
+    'ALLOW_ALL_ADVANCED_COMPONENTS': True,


We don't want this set to True. It would appear from the comment that the process is to get this new component approved by edx.

Yes, this should definitely stay False. Instead, this block should be added to the list of ADVANCED_COMPONENTS.

Sounds good, we are making the change right now and the block is already part of the ADVANCED_COMPONENTS list.

cahrens · 2014-01-17T17:51:11Z

@danielcebrian Can you add a description of this feature or a link to a specification of it? Someone from the Studio team will review the Studio impact of this PR in the next sprint.

Please also follow up on the failing tests.

lduarte1991 · 2014-01-17T17:53:27Z

@cahrens I had a question above about the annotation_storage_url and inheritance.py issue. Is there any way you can help me or lead me to someone who can? Thank you!

nedbat · 2014-01-17T18:00:28Z

cms/urls.py

@@ -48,6 +48,9 @@
    # ajax view that actually does the work
    url(r'^login_post$', 'student.views.login_user', name='login_post'),
    url(r'^logout$', 'student.views.logout_user', name='logout'),
+
+    #added token


What is this comment supposed to mean?

This is to give information of a new permission to generate a token for the catch (annotation backend)

If I'm not mistaken, with a few changes we've made as of late this comment and the line below will be deleted momentarily.

cahrens · 2014-01-17T18:04:44Z

@nedbat or @cpennington would be good folks for the annotation_storage_url question.

Keep in mind that display_name is actually shown to users in the Studio UI. So it should title case and not just a variable name.

jzoldak · 2014-01-17T18:57:13Z

@mhoeber - Tagging you for doc implications.

lduarte1991 · 2014-01-17T19:24:35Z

We're working on the silly mistakes pointed out in the jenkins test. In the meantime does anyone have any idea about these two: https://jenkins.testeng.edx.org/job/edx-all-tests-manual-pr/543/SHARD=1,TEST_SUITE=unit/testReport/junit/xmodule.tests.test_textannotation/TextAnnotationModuleTestCase/test_get_html/
https://jenkins.testeng.edx.org/job/edx-all-tests-manual-pr/543/SHARD=1,TEST_SUITE=unit/testReport/junit/xmodule.tests.test_videoannotation/VideoAnnotationModuleTestCase/test_get_html/

cpennington · 2014-01-17T20:35:25Z

common/lib/xmodule/xmodule/textannotation_module.py

+            'element_id': self.element_id,
+            'instructions_html': self.instructions,
+            'content_html': self._render_content(),
+            'annotation_storage':self.annotation_storage_url


The annotation_storage_url field is used by this block, so it should be defined on this block as well. The definition in inheritance.py is only used to allow the attribute to be set on other parent blocks as well. This will also probably fix your failing xmodule tests.

so is it enough to do something along the lines of:
self.annotation_storage_url = self.annotation_storage_url if typeof(self.annotation_storage_url) != undefined else ""

We don't want to define the url directly here as that might change. For academic purposes, my question is still why inheritance.py doesn't define it here when running rake test, but it does define it when you have the VM running.

What I meant was, you need the field definition in AnnotatableFields. (annotation_storage_url = String(...)). Without it, the AnnotationModule isn't usuable unless the InheritanceMixin is applied (which is not the case in tests, for instance). You want to be able to test the module in isolation.

Got it, and will fix it with daniel as soon as possible. Thank you all so much for helping us through this ordeal.

@cpennington So my question then would be when the code is changed and we define it in AnnotationModule does the InheritanceMixin then apply the url used in advanced settings for the course? or will the instructor have to input the link every time they create a module instead of once per course? i.e. does adding the code above override the variable in inheritance and thus make it useless?

Yes. Defining the Field in the AnnotationModule is what allows you to use the module in any runtime. Adding the definition in InheritanceMixin is what allows Studio/LMS to use the value defined at the course level in Studio to set the value on all of the modules anywhere in the course.

A user would be able to override the course-level value on a particular module by setting an explicit value on the module, but if they don't do that, then it'll just default to the course level value.

danielcebrian · 2014-01-21T18:47:32Z

We've done another push and we think the errors are fixed and if the coverage rule could be lenient because of time issues

cahrens · 2014-01-21T18:53:28Z

To clarify-- someone from Studio will review this by next Friday (January 31st). We have it as a story in our current sprint. A description of the feature (spec) would be most helpful in the review process.

cahrens · 2014-01-22T17:05:48Z

@danielcebrian Please take a look at the quality and coverage reports.

https://jenkins.testeng.edx.org/job/edx-platform-report-manual/248/Diff_Coverage_Report/?
https://jenkins.testeng.edx.org/job/edx-platform-report-manual/248/Diff_Quality_Report/?

lduarte1991 · 2014-01-22T17:24:02Z

Are 99 and 92% not good enough? What is the ideal target?

cahrens · 2014-01-22T18:00:45Z

For quality, no pep8 violations. I think pylint is less strict, but you should still fix everything that can be fixed. We don't want our pep8 and pylint violations growing.

For coverage, make sure that everything that is not covered is not covered for a good reason. A lot of our PRs have 100% coverage, but that is not a requirement (I don't know what the official number is, but I would guess 90%). In particular, "exceptional" code paths often aren't covered by our tests. And unfortunately our integration (Selenium) tests currently do not count towards code coverage for technical reasons.

cahrens · 2014-01-22T18:17:54Z

cms/envs/common.py

@@ -304,6 +314,25 @@
        'output_filename': 'js/cms-modules.js',
        'test_order': 1
    },
+    'main_vendor': {


Instead of putting these dependencies here, it would be better to update CMS' RequireJS configuration. See /cms/templates/base.html.

FYI, LMS does not use RequireJS.

These files aren't really used in CMS. You should not be able to create annotations in Studio mode, only do the set up and actually annotate in the LMS. Where would they go if not in this file if we only needed them for the LMS?

This file is specifically for CMS. It does not impact LMS at all.

LMS has a similar file, and I believe it already has the libraries listed there (in this PR).

Oh I'm sorry, you are correct. We'll remove it from here and the line in cms/urls.py that is associated with it as well.

Yes, you are right. At the beginning, we thought to use annotator in the CMS and finally we decided to put in the LMS. But we are going to remove it.

Can you please look over the rest of the PR and make sure the files are what you want in the final version? When you have addressed all the outstanding issues and feel that the PR is good shape for review, add a comment and I will start reviewing again.

Ok. I am working on this.

cahrens · 2014-01-22T18:38:37Z

@danielcebrian is there a URL for annotation_storage_url that we can use for testing? I'm starting to manually test the branch.

cahrens · 2014-01-22T18:44:40Z

@danielcebrian When I add "textannotation" and "videoannotation" to the advanced_modules in Studio, they still don't show up in the advanced components on the unit page. I think you need to add them to ADVANCED_COMPONENT_TYPES in edx-platform/cms/djangoapps/contentstore/views/component.py (and update unit test).

Stop using JavaScript to set the body's CSS class

This does not include some of the testing files. The textannotation and videoannotation test files are not ready. waiting for an answer on the issue.

It seems like feedback.js is only for CMS…so I’m not sure what to do in this case. I’ve removed commented out code and left alert and confirm as it appears in LMS. i.e. confirming delete of a discussion thread post.

Conflicts: common/lib/xmodule/xmodule/tests/test_textannotation.py common/lib/xmodule/xmodule/tests/test_videoannotation.py lms/templates/textannotation.html lms/templates/videoannotation.html

lduarte1991 · 2014-02-06T18:26:53Z

@nedbat Hi there, so we ran into some serious rebasing issues. Every time I try to rebase it gives me dozens of conflicts that start out from various previous commits. Once I go through and manually address each conflict and continue the rebase, it says it's fine, but when I try to rebase again it gives me all the same conflicts once more...can you point us in the right direction? or can @danielcebrian delete his fork and have him fork a new version or will that mess up this PR?

jzoldak · 2014-02-06T18:29:34Z

@lduarte1991 it'll be easier to rebase if you squash your commits.

lduarte1991 · 2014-02-06T18:35:43Z

@jzoldak I get the same issue. As per the "How to Rebase a Pull Request" on here I tried to squash my commits but I still have to do git rebase --interactive. At the end it performs the same merge with conflicts that I spent hours trying to resolve before. It keeps telling me error: could not apply 5c8b9cd... annotation tools which is the patch that contains the squashed commits from the other PR.

lduarte1991 · 2014-02-06T18:38:55Z

If I may venture an explanation as to what the issue might be, i'd say it has to do with the fact that I forked from this version of daniel's code. Squashed all the commits he'd made since the beginning of this PR #2188 so now Daniel's code is trying to merge and squash the commits that have already been squashed as well as rebase with the squashed commits it is trying to commit...if that made no sense, I can see why git is also confused.

Conflicts: lms/templates/textannotation.html lms/templates/videoannotation.html

danielcebrian · 2014-02-07T15:55:41Z

Please, let continue the pull request here https://github.com/edx/edx-platform/pull/2515

* fix add note to Zendesk help openedx#2194 (openedx#2202) * fix add library option, and library links to the course. openedx#2181 (openedx#2205) * second fix. add library option, and library links to the course. openedx#2210 (openedx#2211) * third fix. add library option, and library links to the course. openedx#2213 (openedx#2214) * Add the validation of the due date before 1900 openedx#2188 (openedx#2208) * Fix bug for biz playback page title openedx#2195 (openedx#2203) * fix test case. add library option, and library links to the course. openedx#2213 (openedx#2220) * Add note to page of Password reset confirm. openedx#2216 (openedx#2217) * Add login code to Playback Status page when the contract using login code openedx#2193 (openedx#2209)

jzoldak mentioned this pull request Jan 16, 2014

Annotation tools #2024

Closed

jzoldak reviewed Jan 16, 2014
View reviewed changes

cahrens reviewed Jan 17, 2014
View reviewed changes

nedbat reviewed Jan 17, 2014
View reviewed changes

cpennington reviewed Jan 17, 2014
View reviewed changes

cahrens reviewed Jan 22, 2014
View reviewed changes

talbs and others added 19 commits February 6, 2014 11:28

Merge pull request #2476 from ionis-education-group/bodyclass-without-js

a41bdb4

Stop using JavaScript to set the body's CSS class

Change the element id back to the unit id tab

b8e1e74

annotation tools

e21118f

First set of fixes from the pull request

ebe0749

This does not include some of the testing files. The textannotation and videoannotation test files are not ready. waiting for an answer on the issue.

Deleted token line in api.py and added test for token generator

c876e33

Added notes_spec.coffee

174604c

fixed unit test

5773fe3

remove spec file

85598fc

fix W0223 warning

e6591d1

Used url parse to get extension

471b081

Got rid of more console.log from tools going into production

a904358

Trying to do feedback_prompt for confirming deletion

2027bcd

fixed firefox bug

1e6ee71

fixed redirect between units

f9cfaa1

fixed a warning of annotator undefined in cms

18307e6

fixed conflict between Annotator and internationalization

2a5e932

Not possible to do feedback.js in LMS

64a8e5a

It seems like feedback.js is only for CMS…so I’m not sure what to do in this case. I’ve removed commented out code and left alert and confirm as it appears in LMS. i.e. confirming delete of a discussion thread post.

Changed tags variable so as not to conflict with XBlock's tag variable

638abf7

Merge branch 'master' of https://github.com/danielcebrian/edx-platform

b88bc4b

Conflicts: common/lib/xmodule/xmodule/tests/test_textannotation.py common/lib/xmodule/xmodule/tests/test_videoannotation.py lms/templates/textannotation.html lms/templates/videoannotation.html

lduarte1991 added 2 commits February 6, 2014 14:40

Merged Fixes

220f914

Merge branch 'pr/3'

18c7787

Conflicts: lms/templates/textannotation.html lms/templates/videoannotation.html

danielcebrian closed this Feb 7, 2014

cahrens mentioned this pull request Feb 13, 2014

annotation tools #2515

Closed

lduarte1991 mentioned this pull request Apr 29, 2014

Annotation Tool UI Fixes and Move to Xmodule/js #3529

Closed

annotation tools #2188

annotation tools #2188

Uh oh!

Conversation

danielcebrian commented Jan 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcebrian commented Jan 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Jan 17, 2014

Uh oh!

lduarte1991 commented Jan 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Jan 17, 2014

Uh oh!

jzoldak commented Jan 17, 2014

Uh oh!

lduarte1991 commented Jan 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcebrian commented Jan 21, 2014

Uh oh!

cahrens commented Jan 21, 2014

Uh oh!

cahrens commented Jan 22, 2014

Uh oh!

lduarte1991 commented Jan 22, 2014

Uh oh!

cahrens commented Jan 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Jan 22, 2014

Uh oh!

cahrens commented Jan 22, 2014

Uh oh!

lduarte1991 commented Feb 6, 2014

Uh oh!

jzoldak commented Feb 6, 2014

Uh oh!

lduarte1991 commented Feb 6, 2014

Uh oh!