Installing Image Annotation Tool for Upcoming June 2014 Course

[lduarte1991]

Installation: To install the image annotation tool go to Advanced Settings for the course. Under advanced_modules input "imageannotation". Under annotation_storage_url input the given test url link (note that this is different between http/https) -- feel free to email for information regarding this and the annotation_secret_token. Now head over to create a unit and there should now be an option for "Advanced" that includes "Image Annotation." Once selected you can go to Edit->Settings to change the blob that holds the image. A sample one is provided but as long as you have access to a IIIF server, any blob can go in.

Background: This was a compromise made between me, @singingwolfboy, and @nedbat in order to meet a hard deadline for the image annotation tool for an upcoming course. This PR replaces #3744 except it is now not dependent on #3480 and #3529. It contains some of the same flaws that the image annotation tool and video annotation tool contain, but as can be seen by the consistent updating and pull-requesting, I am committed to make those changes outside of this time constraint. Nitpicks should be noted, but if they have no adverse effect on the overall functionality, I'd plead to have them wait and added onto #3480 or #3529.

Studio Updates: The new annotation tool looks very similar to the other two tools. There are no annotations done in the studio side of things. The settings should allow for a new image blob and a a place for prepopulated tags.

LMS Updates: The new annotation tool contains the same grid effect as the other two tools. It uses OpenSeadragon to contain a deep image zoom using a IIIF server maintained by HarvardX and referenced in the blob. It contains a fullscreen mode that still allows for creation of annotations. The grid now contains a thumbnail of the image highlighted/annotated (thanks to our IIIF server) and clicking on it should zoom/pan the image to show it off.

Testing: Testing should happen the same way as the rest of the tools. zoom/pan through the image, make sure you can create/edit/delete an annotation. They should populate through the table at the bottom of the screen. In that table you should be able to edit/delete as well as reply to the annotation. Clicking on the thumbnail should take you directly to the highlighted image within the openseadragon view at the top.

[cahrens]

@lduarte1991 please address pep8/pylint issues.

https://jenkins.testeng.edx.org/job/edx-platform-report/4866/Diff_Quality_Report/?

[lduarte1991]

@cahrens Absolutely, was in the process of getting through them. All done now!

[andy-armstrong]

There is a styling issue when editing the xblock. I'm going to talk to @frrrances to see what might be going on. It isn't critical by any means, just a little ugly looking.

[andy-armstrong]

When I zoom in to an image, I'm seeing a lot of display artifacts. I'm using the latest Chrome on the Mac. Not a blocker for this merge, but just FYI.

[andy-armstrong]

I get an error when I try to add an annotation to the image.

[lduarte1991]

Oh damn! I forgot to change the default image. That one is not using the latest IIIF API. I'll send you the proper one momentarily.

[andy-armstrong]

Thanks @lduarte1991. There are also three unit test failures in your new tests (plus one spurious looking failure in our video tests).

[lduarte1991]

Absolutely, working on those and the default fix. Just sent you replacement html for the correct default.

[andy-armstrong]

👍 Thanks for separating this out into a smaller PR as it is much easier to deal with. The code looks good to me, and it works fine in Studio. I have some quibbles about the UI itself in LMS, but since this will be an undocumented advanced module I don't think those should hold it up.

Note that the code still needs review by someone from the LMS team before it can be merged.

[lduarte1991]

Thanks! And I'd love to hear the quibbles so feel free to email/message me wherever possible!

[lduarte1991]

I'm assuming that's one of those spurious looking failures again?

[andy-armstrong]

Yep, looks like another random failure. I've reported them both to our test engineering team. For now, I've kicked off the build again.

[andy-armstrong]

Now I'm really confused. We now have three brand new acceptance test failures. Our builds are not usually this variable. Let me try rebuilding one more time...

[andy-armstrong]

@lduarte1991 I had one question for you. Do you have a maximum number of annotations that you are expecting on a single component? My expectation is that for a Harvard course you could get thousands of users all adding annotations to the same component over time.

I'm a bit nervous that it won't scale very well on the client-side to more than a couple of dozen. The annotations are all rendered one after the other with no paging or scrolling (or did I miss that?). Have you considered adding either paging or scrolling? I'm going to go try it out now with a few hundred annotations to see what happens.

[jzoldak]

@lduarte1991 regarding the failures, we are actively working on test hardening and hope to have that problem solved soon.

[lduarte1991]

In terms of limiting annotations, there is a limit to the number of annotations first loads in. 50 I think? Then there's a "load more" button. We want to add paging later on. We do have a filter to switch between your own annotations and those of all people in the class. And searching via tags (less strict than users) and the text itself. 

—
Sent from Mailbox

On Thu, May 22, 2014 at 03:41 PM, Jay Zoldaknotifications@github.com, wrote:

@lduarte1991 regarding the failures, we are actively working on test hardening and hope to have that problem solved soon.

—
Reply to this email directly or view it on GitHub.

[andy-armstrong]

Thanks @lduarte1991. That seems reasonable for now.

[sarina]

Prefer that pylint disables use the actual name of the pylint warning/error, rather than the number - so one would do # pylint: disable=invalid-name as opposed to # pylint: disable=C0103.

However I don't think you need these disables; if I remove them I don't get a Pylint violation (I removed them to figure out what E1103 was).

[sarina]

why did the indentation change? the original seems more in line w/ the preceding line

[sarina]

2 more things:

1. Make sure everyone who's worked on this code is present in AUTHORS file.
2. Please add a CHANGELOG entry!

[flowerhack]

Tested the functionality of it, and it seems to work well. Can look again when Sarina's comments are addressed

[lduarte1991]

Ok, I believe I covered all the fixes asked so far. Modified AUTHORS and CHANGELOG. Ran rake test locally and it was clean. Ran quality tests and cleared the pep8/pylint errors. Ran the code locally on my machine and on a server running the production stack and it worked as intended. So I guess if @flowerhack could look at it again now, it'd be greatly appreciated.

[lduarte1991]

Seems to be another spurious failure right?

[sarina]

As per https://groups.google.com/forum/#!topic/edx-code/3f2U0RA2SlA you'll now need to rebase onto master. Then you will need to re-test locally. You'll need to first run migrations. Thanks!

[lduarte1991]

I'm on it!

[lduarte1991]

@sarina @flowerhack the code has been rebased and tested locally and everything seems to be working perfectly.

[sarina]

👍 once tests pass and @flowerhack does one more round of manual testing!

[sarina]

Also - we're trying to move the the direction of having commits that have good names - that is, names that, outside of a PR, are understandable. For your commits, one like "Bug fix: Add component to view" doesn't make a lot of sense; more sense might be "Image Annotation Tool: Add component to view".

Along those lines, we request that you squash commits that have commit messages like "todo: fix this", "address comments" etc, into a different commit.

I'm happy to help you with this, it's called interactive rebasing. You can read about it https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request#squash-your-changes-optional in the section "Squash your commits". During the interactive rebase, you can also use the "reword" option to reword a commit.

[flowerhack]

🎋 🎋 ⛵ ship it! looks good to go

[lduarte1991]

Tests all passed which is a great thing!

I've done a little bit of squashing before but yes some help would be appreciated. In particular what kind of things should be squashed in terms of what I've committed above. Thanks for everything!

[sarina]

@lduarte1991 sure - I don't think you have much to squash. Here's how I'd run a rebase on this branch (s = squash, r = reword):

r acebcc2 Added helper functions and fixed pep8/pylint errs
s 865ea3f Small plugin fixes
r f8394ca Installing image annotations
r c0177bf Added static images for openseadragon navigation
f fbd46de Bug fix: Added component to view
r 2e89465 Bug Fix: Added correct images for OSD
r fcaecaa Bug Fix: path to codemirror missing a slash
r dd69a24 Added Linkback functionality to image thumbnails
r c7288b9 Bug Fix: Tinymce fullscreen loads now
s 600e7ad Pep8/Pylint Fixes
r d6d9081 Bug Fix: Default Image and call to super
s 709a220 Changes requested in PR

For all the reworded ones, I'd just add "Image Annotation:" to the start of the commit message. This is important especially for ones you're calling "Bug Fix"; it's important to note what feature(s) the bug fix addresses.

[sarina]

Of course, if the "Bug Fix" commits are super tiny, there's nothing wrong with squashing them all into one commit with a message like this:

Image Annotation Tool Fixes

- Added component to view
- Added correct images for OSD
- Fix path to codemirror
- Tinymce fullscreen now working
- Default Image and call to super

[lduarte1991]

@sarina I believe that should do it?

[sarina]

✨ 🚀 😁 looks amazing! I'll merge this once the tests run again!

[lduarte1991]

Perfect! Thanks so much for everyone's help!