TNL-213: Student Notes

[polesye]

Description
Adds the ability to make highlights and notes in courseware (only in html components at the moment) and to refer back to later.
Student Notes
Student Notes Testing
sandbox: http://edxnotes.m.sandbox.edx.org/
Please make sure you're on MIT VPN to test the sandbox
(that requirement would go away once better deployment script OPS-224is created)

• Platform team review of the XBlock wrapper implementation. XBlock Asides is in the process of merging and we'd planned earlier to use this feature for notes when ready. The scope of the review would be to ensure that the overall implementation from XBlock perspective is still aligned with that goal, and to identify any other concerns related to current/future work on XBlock APIs.
  @dmitchell please review
• TNL overall review - this would be simply to socialize the new functionality and provide an opportunity to identify any conflicts or major questions - NOT a detailed line-for-line review.
  @jimabramson , @andy-armstrong , @cahrens, @srpearce please review.

TBD:

• Ensure Harvard Annotations and Student Notes can't be turned on at the same time TNL-930 TNL-930: Turn off Student Notes when Harvard Annotation Tool is enabled. #6340
• Eventing for Student Notes TNL-931 TNL-931: Eventing for Student Notes. #6358
• Highlight the searched-for text TNL-973
• UI/UX improvements.

@olmar , @tymofij , @jmclaus FYI.

[srpearce]

Will an end user/student see this error message? If so, I'd change it to indicate what "save your state" means - for example, "We can't currently save your note. Make sure that you are connected to the Internet, and then try refreshing the page."

Also: I've added this to the Google doc so all the UI text recommendations would be in one place.

[polesye]

Will an end user/student see this error message? If so, I'd change it to indicate what "save your state" means - for example, "We can't currently save your note. Make sure that you are connected to the Internet, and then try refreshing the page."

Yes, it'll. This functionality is responsible for toggling visibility of the notes (https://openedx.atlassian.net/browse/TNL-661). It is not about note saving.

[srpearce]

OK, I'd go with this (assuming we don't want to just use the generic "An error has occurred," which I think would be fine here):

We couldn't <show/hide> your notes. Make sure that you are connected to the Internet, and then try refreshing the page.

[jmclaus]

@srpearce

Changed the error message to:
We couldn't <show/hide> your notes. Make sure that you are connected to the Internet, and then try refreshing the page. Just to be sure, the < > around show/hide are intentional?

Also, did the same change to the error message in toggle_notes_factory_spec.js

[nasthagiri]

@polesye Please see the discussion in the "dev" channel on HipChat (Dec 30, 9:26am). It seems there may be a database version compatibility issue with the introduction of a new tab type.

[cahrens]

I don't understand why there is a second method (which looks very much like _config_course_advanced_components). Please explain why there are 2 methods, and make sure the comments in this method are all correct (there are references to advanced settings, but I don't think this method is supposed to be about advanced settings).

[polesye]

I don't understand why there is a second method (which looks very much like _config_course_advanced_components). Please explain why there are 2 methods, and make sure the comments in this method are all correct (there are references to advanced settings, but I don't think this method is supposed to be about advanced settings).

_config_course_advanced_components is more specific and works only with advanced_modules field. It adds/removes appropriate tab when specific module is added.
_config_course_settings has the similar logic, but works with course fields.

We'll update names and comments.

[tymofij]

this has been reworked in edx@c384ee5

[andy-armstrong]

Was it a conscious decision to make the Enter key close the note modal? I expected to be able to create multi-line notes and couldn't find a way to do it.

[andy-armstrong]

I'm finding the tooltip to not work very well (I'm using Chrome). I find that there's a long delay where it shows an earlier tooltip and doesn't pick up that I've moved on to a different section. Even when I click on the region to select it, the old tooltip is the one shown.

[andy-armstrong]

The notes I'm making on the sandbox do not seem to be persisting. When I click on the "Notes" tab I don't see any, and when I click back to the courseware my notes have gone. I did once see a black error bar with a message about the server but not every time.

[andy-armstrong]

I just got the message again that said "Sorry, the Annotations service is down. Contact your instructor and try again later.". I guess that's why nothing is persisting for me on the sandbox.

[polesye]

It's not at all compatible w/ Asides, but I don't know if you can help it. To see examples of Aside views xmodule.modulestore.tests.test_asides.AsideTestType.student_view_aside or a fuller one acid.acid.AcidAside.aside_view where the view renders just the aside and not the block it decorates. It does receive the block it decorates but doesnt' render the block.

@dmitchell Yes, it can be hard to make it compatible with Asides at the moment and we can do some refactoring during migrating to Asides.

[tymofij]

@andy-armstrong @cahrens your comments have been addressed, please take a look.

[dmitchell]

afaict, this is simpler and the same

while xblock:
   parent = xblock.get_parent()
   if parent is None:
       return None
   if parent.category == 'sequential':
       return xblock
   xblock = parent

[tymofij]

indeed.

[andy-armstrong]

Unfortunately you have to update all your icons to use the new font-awesome scheme (fa-check). If you rebase you'll find that all these icons are broken, as the new version of font-awesome was merged last week.

[polesye]

Unfortunately you have to update all your icons to use the new font-awesome scheme (fa-check). If you rebase you'll find that all these icons are broken, as the new version of font-awesome was merged last week.

Ah, we forgot to update the icons in our test fixtures. Let me fix that.

[andy-armstrong]

👍 once the font-awesome icons are fixed. Awesome work!

[tymofij]

Thank you for your hard work reviewing it, Andy. Just how much should we squash it before merging?

[andy-armstrong]

@tymofij It is a matter of taste but IMO I would want to have each commit deliver some real functionality. I would squash all related UX commits into one, all bug fixes into the commit that they are fixing etc. One problem for a feature branch is that sometimes the fixes span commits in which case it is hard to choose which commit to squash them into. In that case I'd just leave the commit alone.

For example, I wouldn't want to see commits like "Clean up the code", "Fix Jasmine test" or "REVERT THIS COMMIT BEFORE MERGE" (should the latter actually be reverted, btw?). Commits like "TNL-655: Add/Edit/Remove notes." I would not merge away.

[tymofij]

yes, the "REVERT ME" commit indeed must go, for it enabled the feature unconditionally. We needed it to make bok-choy server run with feature enabled on Jenkins.

[cahrens]

Note that you can't merge this to master until performance is worked out on the get_parent changes. Stay in touch with @jimabramson about this, as he is leading up the performance work.

Also note that the release candidate for this week has not yet been cut. We don't want the get_parent infrastructure changes in this week's release.

[tymofij]

rebased on master.

[andy-armstrong]

The code looks right to me, but I don't know the pitfalls. I don't think you can merge with this, though, without having unit tests that verify that it does work as expected. Can you reuse the tests from Jim's branch?

[andy-armstrong]

For example, does the cache get cleared appropriately when an xblock is reparented?

[tymofij]

as the cache is stored in request_cache, it is cleared after each request completes.

[andy-armstrong]

I know, but what happens if code accesses the parent after a reparent within the same request. I guess that's more of a problem for @jimabramson's generic solution than for yours, since you know where you are using this method.

Anyway, there need to be tests of the basic cases. What happens when called on a block which hasn't been parented yet? What happens with the boundary between a draft child and a non-draft parent? Are there gotchas in split vs old mongo?

[andy-armstrong]

Thinking some more, I suppose you don't have to worry about drafts as your method is only in LMS...

[tymofij]

a test was added edx@19dc8ec

[tymofij]

@andy-armstrong I'd do the same tests twice

Done. edx@91c7167

Also other tests of other functions that use it show no side effects.

[andy-armstrong]

👍 Please squash as many commits as you can before merging.

[carsongee]

Sorry to zombie post, but are there docs out there for how to wire this up with a backend? Just turning on the feature flag seems to crater the LMS with ImproperlyConfigured since it is missing an oauth2 token, but I couldn't find any docs on how to set up that token to work. I'm guessing https://github.com/edx/configuration/pull/1784 is the setup for the backend, but it was also a bit light on detail for setup with oauth2