Preparing rating XBlock for production -- adding tests and testability

[pmitros]

This prepares the rating XBlock for production. This reworks the XBlock for testing and testability. We have already gone through accessibility review. I believe with this PR it will be MVP shippable.

At some point, we may want to do further work in order to:

1. Improve UX.
2. Decide whether we want to stay with Unicode for happy/unhappy faces, or switch to icons.

[pmitros]

@cahrens @explorerleslie I made a PR to move the Rating XBlock to production. Dropthought -- who we were cloning -- just went out of business, and I thought it'd be nice to give courses a migration path.

It has different product goals from DoneXBlock, but it's actually very similar in function, structure, and tests, so it may save time and effort to review the two together.

Original review was here:
https://github.com/edx/edx-platform/pull/8597

The core change is the addition of tests.

A major limitation is that the only way to get data out is through data dumps. This is a little fundamental for the qualitative feedback; future iterations will likely improve this for quantitative. The UX could be better; we based it on how the LTI version of Dropthought looks, but now that Dropthought is no longer around, we could do better. The use of Unicode for faces has upsides and downsides. I think all of these would be good to explore, but not in the MVP.

[explorerleslie]

@pmitros thanks! I agree, I think this would be very valuable for our course teams. However, before wider rollout I think that both data output (possibly through a data download on the reports tab, or through the Xblock itself as a link that only staff sees to download data) and UX improvements are required for MVP (replace icons, general improvements to match existing edX patterns). I'm happy to help with this effort in whatever way you see fit -- let me know what support you need.

[pmitros]

@explorerleslie If there is capacity:

1. Review of the existing changes so we can get this merged. That way, we'd have a list of gaps that need to be addressed before prod. In addition, it would save a lot of time. PRs go stale after a while (where we need to rebase, etc.). In this case, the set of changes is pretty small. It's also easier to review several smaller PRs.
2. A quick UX review would be helpful. Generally, what we'd do in the past is we'd have Francis take 5-15 minutes of her time to provide feedback, and then I'd rework the UX based on that feedback. She's always been very insightful. I've been told too many people were leaning on Francis, so I'd backed off on this a little bit, and she has not done this for this block (hence the UX issues you referenced).

[explorerleslie]

@pmitros great, thanks! I know both Christina and Frances will want to see a sandbox -- could you please set one up?

@cahrens can you please create a ticket for this review -- scope is to review the tests and get it merged, but still not fully supported for edx.org, so no doc or anything.

@frrrances could you please create a UX ticket for reviewing this Xblock. The desired outcome is to give Piotr feedback on the ticket or on a PR on how to improve the existing UX, secondary is any FED feedback which would be great to get but lower priority. In particular, I hate the icons (could we use something in Font Awesome?) and the general UX doesn't match existing edX patterns.

[pmitros]

@cahrens While we're not bringing this to fully supported yet, you may consider giving feedback as to what would be needed to get this to production-quality. My experience has been that this adds little to initial review time (since you're reading the code either way), especially for a block this simple, but saves a ton of time in the final push to prod (where we just need to confirm the issues identified were addressed). Your call either way.

[cahrens]

@pmitros are there changes to the RateXBlock itself that we should review (https://github.com/pmitros/RateXBlock)? I created https://openedx.atlassian.net/browse/TNL-3855 for the TNL review.

[pmitros]

There are minimal changes/cleanups to the XBlock itself -- mostly formatting for PEP8/pylint, and slightly more information returned in AJAX for testability: pmitros/RateXBlock#9

Sandbox is currently provisioning as ratexblock.sandbox.edx.org (see: http://jenkins.edx.org:8080/view/Ansible/job/ansible-provision/4329/parameters/ for status).

[pmitros]

Works on sandbox. Add 'rate' to advanced settings, and add "Provide Feedback" from the advanced menu.

[frrrances]

@pmitros I took a quick look at the xblock and have a few small UX suggestions:

• Switch rating and text box - rating should be first as people will be most likely to complete that part and may be reluctant to spend the time to write feedback out
• Maybe add some intro text/context. You might want a bit of doc help with this, but here's a quick idea: “Help us improve this course! Let us know what you think.”
• Change the rating to say: "Rate this section"
• Use stars for rating (class="fa fa-star-o" for unselected, class="fa fa-star" for selected)
• Change the text box to: "Tell us why you chose that rating"
• Align everything left (just remove text-align: center), remove width 100% on button, remove border and fixed width on block

Here is a quick screenshot of those ideas:

Let me know if you have any questions or need more help!

[pmitros]

Your design is much nicer than what we have for the apparent goal of the tool, and follows better established UX patterns, but it doesn't quite line up with the instructor goals:

1. Provide students with a chance to reflect on the course. This is best served by a more structured question about a specific aspect of the course we'd like the students to reflect on.
2. Provide a way to collect feedback on the course itself for improvement in future runs. This is best served by asking more focused questions as well (length, difficulty, confusing/easy-to-follow, etc.). A simple good/bad is often not all that helpful.
3. Nice-to-have: Act as a drop-in replacement for Dropthought. Dartmouth started out with Dropthought. It didn't work very well, and LTI integration was obnoxious, so I built this so they could use this instead. I'd like to bring this to production since Dropthought went out of business. It is replacing DT in many places, and while not a strict requirement, continuity of user experience is a strong nice-to-have. Unfortunately, Dropthought went out of business, and I cannot find any images for their late UX as in the Dartmouth courses, but screenshots of their UX from before were:

My recollection was that the version used in Dartmouth (late 2015 UX) had five faces instead of four, and had more meaningful question (although it is difficult to tell now the DT is gone).

The MVP tool is relatively minimalist with two configuration options (the text above the Likert feedback section and the text above the faces). Internally, the tool has several bits of functionality which we'd like to expose in a future version (both of these are implemented, but not exposed in Studio as a user-facing feature):

1. The set of icons is configurable. For something like too short/short/appropriate/long/too long, for example, neither stars nor faces make sense. We'd like to maintain a consistent UX here.
2. The tool supports matrix sampling. The instructor can ask five different questions, and have 1/5 of students see each subset (or even 1/50 see each question, and the remaining 90% see nothing).

I'd like to expose these once the tool has proven itself useful, figure out authoring UX, fixed initial bits of feedback from the user experience, etc. I'd like the UX to generalize to there.

[pmitros]

@frrrances Missed tagging you above.

[explorerleslie]

@pmitros jumping in here on the UX -- what I'm seeing from Frances's comments are that they are relatively low effort changes that make a substantial difference in user experience. I don't agree with your assessment that they go against the instructor's goals.

I think putting the suggested text string changes as the defaults and letting course authors customized as they see fit would work well, because it will give instructors solid defaults while still allowing for more specific use cases.

Also, even if we are asking a more specific question, I still think using stars instead of smileys and flipping the stars and text box is a good idea, to Frances's point of, if someone isn't going to type feedback, at least you get their general rating. I also still just hate the current smileys. :)

Happy to talk more in person if it's helpful.

[pmitros]

Talking in person may be helpful. I've found in-person UX reviews to be much more helpful; you can actually get at what you're trying to do and end-user goals.

I hate the current smileys as well, although they're a little better under Ubuntu than under Mac. They come from Unicode, so they are system-dependent. The current UX is a little bit unpleasant, and I'm not opposed to seriously reworking it. It's mostly how it is since we were comparing performance to DropThought, and wanted a near-identical clone, modulo lack of styling.

But I do think star ratings solve a different problem than the one we'd like to solve. We'd like to give users a clear Likert scale, and many Likert scales don't really fit well into a star rating. I'll give a few other models I found online.

This one kills the iconograpy altogether and replaces it with text:

Some have slider. I like this less, since it doesn't give a clear definition to each rating. For example, for NPS, the text "definitely recommend" and "maybe recommend" are important:

(Right now, we do have the text, but only on mouseover)

And a few more:

[explorerleslie]

@pmitros got it, thanks for the context. If a Likert scale is really what we're going for, then we could use radio buttons. These have the built-in advantage that they're easy to make accessible, and then we don't have to worry about any cross-cultural differences with smileys or stars or whatever either. In the future, you'd also be able to give the course author the ability to customize the labels on the Likert scale if you use radio buttons.

[pmitros]

@explorerleslie The code currently has settings for:

• Custom prompts
• Custom labels (which are shown on mouseover, but should probably be shown all the time)
• Custom icons
• Matrix sampling/multiple prompts/subsampling

This is not surfaced to the user, since Studio UX would take a bit of thought, and the MVP doesn't need it.

From an accessibility perspective, the current UX behaves just like radio buttons.

[pmitros]

@explorerleslie @cahrens I believe this may be ready for review/merge, modulo one minor Studio styling issue. This includes a lot of improvements and debugging of the XBlock test framework. In particular, we can now validate the XBlock HTML as well (but not yet JS-side or Studio-side tests).

We provide aggregate statistics to instructors on usage.

Key flags of things to look at:

• I renamed from RateXBlock to FeedbackXBlock. We had user feedback about inconsistent naming. Given the major UX change, this allows courses which use RateXBlock to not break (it is moving into edx-private, and being deprecated).
• In the test framework, when we raise an exception, I added print statements to help debug. I'm not sure whether this is kosher. I'm not sure why it wouldn't be (it only happens on failure), but I wanted to raise it.
• Studio shows double scrollbars. I think fixing this would be a few minutes for @talbs
• It would be good to have another look at accessibility. @cptvitamin
• We still do not have support for client-side or Studio-side testing in the XBlock test framework. We have added support for validating HTML.

Changes to the block itself are shown in:

pmitros/FeedbackXBlock#1

[benpatterson]

jenkins run all

[cahrens]

@pmitros It's not clear to me how much of pmitros/FeedbackXBlock#1 we need to review- it is a large PR. Is there a way to see what has changed since I last reviewed the RateXBlock?

Note that you have some unit test failures.

[pmitros]

Oh. Hey. I didn't notice github didn't handle the file renames very well. I'll clean that up so it shows the changes. I'll make two PRs -- one with changes before the rename, and one with changes after, if that's okay.

I did notice the test case failure. It worked fine on localhost. I'll look into it as soon as devstack is fixed.

[pmitros]

All the changes prior to the rename: pmitros/FeedbackXBlock#2
Rename: pmitros/FeedbackXBlock@bb9d686
All the changes post-rename: pmitros/FeedbackXBlock#3

[pmitros]

@cahrens It might still be easier to have all of the comments in the first PR (not the split up ones).

[pmitros]

@cahrens Would it be possible to get a quick review, even if just on the changes to the XBlock test code in this PR (without merging the big changes in the ProgressXBlock)? I'm trying to add tests to a (test-free) XBlock and running into some of the issues this PR fixes.

[cahrens]

I'm sorry, @pmitros, but I have a Friday deadline for a feature, and the TNL team is down two members. I will try to review on Friday afternoon.

[pmitros]

@cahrens Friday will be fine. I'll work on some analytics in the meantime. Thank you for both the update and the timeline.

[cahrens]

For my own edification, why remove the point about moving the tests into the XBlocks themselves?

[pmitros]

There was logic, but in retrospect, not very sound. I'll add that back.

[cahrens]

@pmitros I reviewed the testing part of this PR. I'm a bit confused about extract_block (how can it assume that the xblock HTML is in the first sequential, and is the only thing there?). Otherwise just some nits.

[pmitros]

I am assuming that. I think better error handling and documentation there would make sense. I'll do that. I'm building on a few hacks:

1. We cannot construct courses from OLX yet. We do this through Studio AJAX calls, I believe. As a result, we have to follow the sequential/... hierarchy.
2. Sequentials are broken, and one of the side effects is it is hard to query by descriptor. That's in escaped HTML inside the page.

We need to fix both, since they're effecting many unrelated things, and the current code -- defining course structures in JSON -- is a stopgap until then. That code does not support general course structures, and we do assume that if we have multiple XBlocks, they are in independent learning sequences. I could put together a pile of code which would select from within a page by decoding/etc., but it'd be super-ugly code, and obsolete out-of-the-box. Once either of those issues is fixed, we're back in the running for the simple solution like the one in the comment.

[pmitros]

@cahrens Thank you for the prompt review! I implemented all of the changes to the test case code. The PR has two commits: (1) updating the test case. (2) Updating the FeedbackXBlock. I will make the changes to the latter, but it looks like it will take a while waiting on a11y, etc. Do you mind if I merge just the first commit (xblock_testcase.py) to master? That will allow other xblock work to follow.

[cahrens]

👍 to merging the test commit

[cahrens]

@pmitros can you rebase this PR?

[pmitros]

@cahrens Rebased.

[cahrens]

👍

[gsong]

Closing due to inactivity.

[AmauryVanEspen]

@pmitros I would like to know if you'r still working on the likert scale system please.

[pmitros]

@AmauryVanEspen Nope. Open edX is in a bit of an IP black hole, so I've moved onto other projects.

Glad to advise if you're planning to work on it.