Fixes check for Image Annotation Tool to include ID from image server and fix scrolling bug

[lduarte1991]

This addresses the bug [TNL-443].
Affected Courses: 1 course is affected (Visualizing Japan) with a total enrollment of ~10,000 students.
Ideal Fix Deployment: Week of Sept 29th

Background: As explained in TNL-443, it was notified early this morning that when users clicked on the thumbnails within the tables they were not able to scroll back up and be zoomed/panned to the proper locations. After a little more fiddling around it was discovered that the issue affected every item after the first "failure." This code changes the way that you verify that the thumbnail/annotation are from the image currently being shown on the same page. It checks that the thumbnail contains the substring of the id as per OpenSeaDragon's API.

Studio Changes: None.

LMS Changes: 1) Removed old way of checking (not sure why that method was chosen by my predecessor, but it seems like a super bad idea) if annotation referred to the image on the current page. 2) Added the fix @sarina asked me to make at the end of the las PR related to the release candidate this week. 3) Added a few more checks for annotation.highlights attribute that sometimes does not get made (i.e. replies). These are not code-breaking but the code is now more correct than it used to be and that's always a plus.

Testing:

1. Create a sandbox instance WITHOUT this fix/PR
2. Install the tarball as usual.
3. Make a few random image annotations.
4. This part is a little tricky: you need to simulate the current bug by changing the annotation.target.src attribute so that it breaks, please ask me to do it for you or talk you through it as you do.
5. Try clicking the thumbnail and you should not see anything happen. This breaks any annotations below the one you broke as well.
6. Switch to this PR branch and try Preprocess assets as Django management command #5 again and you should see it move up and zoom/pan to the correct location

[edx-jira]

Thanks for the pull request, @lduarte1991! I've created OSPR-51 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Supporting information that is good to add there is anything that can help Product understand the context for the PR - supporting documentation, edx-code email threads, timeline information ('this must be merged by XX date', and why that is), partner information (this is for a course on edx.org, for example), etc.

All techincal communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

[sarina]

@lduarte1991 - before I start with making sandboxes, are your commits rebased atop the most recent version of master (from at least noon today, Friday)? There's been some stuff merged in this morning which you need to rebase and pick up if I'm to successfully build a sandbox.

[lduarte1991]

I don't think so actually. I'll rebase and upload momentarily.

[singingwolfboy]

This is better than it was, but still not great. Can we narrow the scope of what exceptions are being caught? (Same with the other instances of this change in other files.)

(This comment is optional: as I said, this is better than it was. I just wish it could be even better.)

[lduarte1991]

@sarina I can't replicate the error anymore, can you doublecheck that "AttributeError" is the appropriate exception for this?

[sarina]

@lduarte1991 - I'm not sure how to double check this. Personally I'm fine
with "except Exception" - I understand it makes coding nicer, but the fact
is, we don't actually care what the exception here is because what we want
to do is we want to just move forward.

On Tue, Sep 23, 2014 at 1:21 PM, lduarte1991 notifications@github.com
wrote:

In common/lib/xmodule/xmodule/imageannotation_module.py:

@@ -115,7 +115,7 @@ def init(self, _args, *_kwargs):
if self.runtime.get_real_user is not None:
try:
self.user_email = self.runtime.get_real_user(self.runtime.anonymous_student_id).email

•        except:  # pylint: disable=broad-except
  

•        except Exception:  # pylint: disable=broad-except
  

@sarina https://github.com/sarina I can't replicate the error anymore,
can you doublecheck that "AttributeError" is the appropriate exception for
this?

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/5305/files#r17923634.

[sarina]

@lduarte1991 : we are not going to have a regular release this week, but if we can get through review this week, hopefully it can be deployed next week. Are you going to be available to respond to the existing review comments?

[lduarte1991]

Yes, absolutely! I'll get on this tomorrow. For some reason I didn't get email notifications from Friday's comments.

[sarina]

@adampalay could someone on your team please review?

[adampalay]

nit: if isSource is true, then isContainer is true -> we don't need to check for isContainer here

EDIT: no need to fix in this PR; consider for future refactoring

[sarina]

@lduarte1991 the release schedule is changed and I would like to get this merged as soon as possible; if there are a lot of delays, this likely won't make it to prod until the week of Oct. 6th. Please answer my out-of-band email ASAP.

[adampalay]

@lduarte1991 , I'm not able to reproduce the original bug on master

[adampalay]

The thumbnail still scrolls me up to the image

[sarina]

For me the thumbnail scrolls me to the same area, but the image itself is no longer highlighted and I can't interact with / edit my annotation

[adampalay]

Ok, able to reproduce! the behavior is fixed on this branch, but now I'm seeing this javascript error in the console:

[sarina]

I was able to reproduce on my sandbox, switched to this branch and issue was fixed.

I'm not actually seeing the same js console error that Adam sees.

[lduarte1991]

I'm not able to reproduce @adampalay's error to clear overlay. Are y'all still getting it?

[sarina]

@lduarte1991 I can't reproduce it either (I am on a sandbox). @adampalay what environment were you in where you saw that error.

[adampalay]

It was local

[adampalay]

It didn't appear to affect functionality tho

[sarina]

OK - since I can't reproduce in a sandbox I feel like the risk is minimal

[adampalay]

👍

[sarina]

@lduarte1991 : I would recommend reverting the except AttributeError back to except Exception; we don't need to needlessly increase the risk here. 👍 after that.

[lduarte1991]

Sounds good to me. I'll do that immediately.

[sarina]

@lduarte1991 ; just for clarification you have 3 commits here, only the first commit addresses the actual bug, correct?