Annotation Tool: Urgent update for ChinaX AB Testing

[lduarte1991]

Background: ChinaX is launching June 12 and they would like to do some AB Testing. @singingwolfboy said it would be possible to merge in for next weeks release. Below you should not see any new files (i.e. they all already exist in master) and only changes to the functionality. There are some files (catch.js in particular) that are extremely messy and will be worked on in future PRs so I plead focus on the changes and not the overall messiness of the file. We have in our dock changes such as the template structure and separating it out into a more MVC model but are limited with timing and resources.

AB Testing: What will be tested is the following:

1. Group A will get an image annotation tool that does not have the annotation tool instantiated. It is merely an OpenSeaDragon viewer that you can zoom in deep.
2. Group B will get an image annotation tool that is readonly (except by the instructor) and will be able to see the instructor's annotations but not create any of their own.
3. Group C will get an image annotation tool that works as originally intended. Both instructor and students will be able to see and create annotations.

CMS Updates: The only changes in the backend are the introduction of 3 new variables in settings. One to set the default tab for the annotation table. One to select the "mode" in which it will be used, either read-only or read/write. One to add the email of the user that will be considered the "instructor" of the course.

LMS Updates: Most of these scenarios to keep in mind are only appropriate to the image tool:

1. the table should default to whatever tab is typed in to default_tab
2. If you are in annotation_mode 2 (everyone can edit)

• if you don't put in instructor then the instructor tab does not appear
• if you put in an instructor then the instructor tab appears
  1. If you are in annotation_mode 1 (only instructor can edit)
• if you don't put in instructor then the student should not see the table at all
• if you do put in instructor then you should see the table and view the instructor's annotations but the "create annotation" button on the image should be gone.

NOTE: This is a hackish way of doing this functionality. Because I'm the only developer on this and our backend person is not currently available I couldn't use an API call to the backend server automate most of of the instructor calls. The name "instructor_username" is used because eventually we want to pass in the username instead of the email, but for now that was the only thing the code let me do in such short notice. Thank you so much for giving it your time. This PR is high priority should go in before the one that used to be #3529. I'm still working on that one but had to deviate to get this one through.

[polesye]

@srpearce , @lduarte1991 Do we need to add some documentation here?

[polesye]

What does it means annotation_mode == "2" ?

[lduarte1991]

I noted in the PR description above that 2 = "everyone can edit"

[polesye]

Your description is perfect, but I want to see this notes in the code, because I'll forget about the description of this PR in a month. And then when I need to add some changes here, I'll spend the time to remember what it means.
Please leave comment here, or use helper methods that will improve code readability:

...
canAnnotateEveryone: function () {
  return this.options.viewer.annotation_mode == "1";
},
canAnnotateOnlyInstructor: function () {
  return this.options.viewer.annotation_mode == "2";
},
...

and then use them so:

if(this.canAnnotateOnlyInstructor() || this.options.viewer.flags){
  ...
}

Do not forget that other developers can work with this code, so the code should be clean and easy to read.

[srpearce]

Usually, we would need documentation for the course team - how do they enable and use this feature (e.g., does the user have to change any of the course advanced settings)? What are the available settings, and how are they set? However, if this is just for ChinaX, and that institution will be working closely with a PM to implement the feature, we may not need or want to make the documentation for it available to others. @Lyla-Fischer, what are your thoughts?

[Lyla-Fischer]

It is unclear that this feature is a fully supported edX product, and therefore releasing full documentation for it would be a little premature. It's under limited release for this one course, and lessons from that course will inform whether it will continue down the path for full productization, including detailed documentation.

That is to say: no, don't worry about docs right now.

[polesye]

👍 once my comments are addressed.

[sarina]

Please align w/ the following line

[sarina]

Actually just remove this, since this comment no longer applies to the string.

[sarina]

I do think that there is enough time to respond to these comments today, and merge this tomorrow afternoon. Please ping us when you've actually pushed up changes to address comments. Thanks.

[lduarte1991]

@sarina I have made almost all the changes but the code fails until I figure out how to do CommonAnnotateMixin from above.

[lduarte1991]

Also, any help on that front would be appreciated.

[sarina]

@lduarte1991 @auraz lives in Kiev so his responses won't occur until later tonight, unfortunately. I recommend being online and active very early tomorrow morning (7-8 am) if you want to have a robust back and forth with him and get all your questions answered.

[auraz]

👍

[lduarte1991]

Cool, so @sarina does that mean it's cool to merge? or do I need to get a second thumbs up? Thanks!

[andy-armstrong]

👍 from me, but I think you should get a thumbs up from @sarina too before merging.

[flowerhack]

I'm testing this locally right now—and I noticed that, if you're in group C and you've got >1 annotation, if you delete 1 of them, the others disappear from view. When you refresh the page, all your annotations (sans the one you explicitly deleted) reappear, so it seems like it's a post-deletion display issue rather than an actual data issue. Not sure if this bug should be a blocker or not.

Will comment here again once I've finished local testing...

[lduarte1991]

@flowerhack Thanks for that. Catch doesn't change between Text/Video/Image. I'll test to see if it's an issue there too or if it's something in the backend.

[sarina]

@lduarte1991 I think that bug should be fixed before merging. That's the only bug we found.

[lduarte1991]

@sarina I just added the bug fix and it should be ready to go whenever tests are done! I also merged in the code that @auraz wanted and helped me figure out.

[sarina]

On your next PR, please be sure to address teh Pylint violations this PR introduced: https://jenkins.testeng.edx.org/job/edx-platform-report/5140/Diff_Quality_Report/?

Otherwise, 👍

[lduarte1991]

@sarina Ooops! Absolutely will do. Adding it to our production management tool immediately.