Complete default blocks #16234

jcdyer · 2017-10-13T21:02:24Z

Add code to submit completions from verticals on behalf of descendent blocks that aren't scorable and don't implement a custom completion method.

JIRA tickets: N/A (Part of Completion API work)

Discussions: See Completion API document above.

Dependencies: #16112 #16047

Screenshots: N/A

Sandbox URL: Upon request.

Merge deadline: None

Testing instructions:

Create a waffle switch named completion.enable_completion_tracking with is_active set to True.
Find a user that has no completions in the demo course.
from django.contrib.auth.models import User
from lms.djangoapps.completion.models import BlockCompletion
pick_from = [u for u in User.objects.all() if not BlockCompletion.objects.filter(user=u).exists()]
Register for the demo course (if needed)
Visit "Example Week 1: Getting Started -> Lesson 1 - Getting Started -> Getting Started"
Once the page has fully rendered, verify that BlockCompletion.objects.filter(user=user, course_key=CourseKey.from_string('course-v1:edX+DemoX+Demo_course') returns a completion object with a completion value of 1.0 for the HTML block and Discussion block on that page. (A later ticket will exclude discussion blocks from completion tracking).
Visit the second-to-last vertical in the sequential: 3. Visit "Example Week 1: Getting Started -> Lesson 1 - Getting Started -> Interactive questions".
Completion of scorable XBlocks is handled separately. Verify that visiting the page doesn't create a completion for the capa problem on the page.

Author notes and concerns:

There may be more work that could be done to extract this into a CompletionContainerMixin, so implementations could add it to a different block type than verticals if appropriate for their installation, but that feels like an optimization that should be deferred.
I considered pushing the functionality to submit completions to the runtime service as well, but since the runtime itself is needed to publish events, and the runtime is directly available to the block, it didn't seem necessary.
Is there a better way to include the runtime service than hard coding it on the LMS runtime, and making it optional on the vertical?
This will need a follow-up to be more specific about when to complete blocks. It currently marks them complete when the vertical is first visited, but it should do it when the block has has been viewed, which will require checking coordinates of the block's containing element in the viewport. We initially said that it should be when the block
I tried to convert the vertical file to ES6, as suggested by the front-end guidelines, but I couldn't make the import of the bookmark button work. I tried several ways of specifying the path, and asked on the openedx slack instance, but couldn't get it working. I don't think it's worth me putting any more time in, but I imagine it would be pretty straightforward for someone with more javascript understanding.

Reviewers

@mtyaka / @bradenmacdonald
edX reviewer @robrap

FYI: @e-kolpakov

Settings: N/A

TODO:

Squash commits
Create a follow-up ticket for improving completion timing (See Author notes Ensure that the original error gets preserved by the ErrorModule #4 above).

openedx-webhooks · 2017-10-13T21:02:31Z

Thanks for the pull request, @jcdyer! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

jcdyer · 2017-10-18T15:52:38Z

@bradenmacdonald I thought so, but I was getting lint errors about const and let being reserved keywords. See: https://build.testeng.edx.org/job/edx-platform-quality-pr/41098/Diff_Quality_Report/

CC @andy-armstrong What's the state of ES6 in edx-platform?

arizzitano · 2017-10-18T17:52:01Z

@jcdyer @bradenmacdonald is right -- es6/es2015 should be used for new js development. You can't mix es5 and es2015 within the same file, though, so if you are unable to convert the entire file to an es2015 module (transpiled through webpack) it may be a moot point.

jcdyer · 2017-10-18T19:02:04Z

Thanks @arizzitano . Converting the whole file shouldn't be hard, though I've got a follow-up question about how to configure it on slack: https://openedx.slack.com/archives/C0EUBSV7D/p1508347878000087.

jcdyer · 2017-10-18T19:19:27Z

jenkins run python

robrap

@jcdyer: Sorry if this semi-complete review is premature. The ES6 discussion got me confused about what was active.

robrap · 2017-10-19T15:39:51Z

common/lib/xmodule/xmodule/vertical_block.py

Do you think the following is any simpler to read?

getattr(block, 'completion_method', 'completable') == 'completable' and not getattr(block, 'has_custom_completion', False) and not block.has_score

Also, I thought I saw @bradenmacdonald comment somewhere on 'completable' being a named constant, but not seeing that, so just noting it again.

This is a const-soon. See above.

@robrap Here's the -soon joke.

Ah yes. Thank you again. I like.

robrap · 2017-10-19T15:46:18Z

common/lib/xmodule/xmodule/vertical_block.py

How about get_view_based_completables, or something of that nature?

robrap · 2017-10-19T15:48:06Z

common/lib/xmodule/xmodule/vertical_block.py

Minor change just in case someone wants the full set and doesn't realize that some are filtered out?:

Return a set of descendent blocks that this vertical still needs to mark complete upon viewing.

robrap · 2017-10-19T15:49:14Z

common/lib/xmodule/xmodule/vertical_block.py

Can you add a little explanation? Something like?

# Exclude completed blocks to reduce traffic from client

robrap · 2017-10-19T15:50:24Z

common/lib/xmodule/xmodule/vertical_block.py

Possibly update name of 'watched_completables' based on other name change.

Changing to "watched_completable_blocks." I think the "watched" part is still an appropriate descriptor, especially at the template level. These are the blocks that the completion system needs to watch for completion.

robrap · 2017-10-19T16:18:37Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

Maybe renaming completable to block would make this less confusing?

robrap · 2017-10-19T16:26:57Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

Two possible (untested) name changes captured below.
completable.dataset.completable => block.dataset.completableByViewing

See stackoverflow post for where I took naming conversion from data-completable-by-viewing => dataset.completableByViewing.

robrap · 2017-10-19T16:31:56Z

common/lib/xmodule/xmodule/seq_module.py

Can we have a string constant somewhere? Also, not seeing this used anywhere.

There will be a string constant in the XBlock repo. I'll update this to use that once it lands (possibly a later commit, depending on how long it takes to get this one merged).

robrap · 2017-10-19T16:50:12Z

common/lib/xmodule/xmodule/vertical_block.py

Same questions/concerns as earlier about rollout. Can this be waffle protected? Maybe we can discuss elsewhere.

robrap · 2017-10-19T16:54:11Z

lms/djangoapps/completion/services.py

Note to self: This is where I am leaving off for now because: 1) I have limited time allocated to this and 2) I may have jumped into this review prematurely. I'm getting confused around what reviews are needed when, and I should probably wait for the explicit messages from @jcdyer asking for review. :)

I'm having trouble juggling all the PRs right now, too. I'm working toward getting the early ones merged, so there's less to focus on.

robrap · 2017-10-23T16:54:45Z

@jcdyer: As a reminder, I did not review all the code. Please leave an explicit comment when and if @bradenmacdonald and I should review like you did on the others. Thanks.

jcdyer · 2017-10-26T18:41:41Z

This is ready for an (opencraft) review. @mtyaka you are listed as reviewer on this, but you signed up for that in a different sprint, so I understand if you don't have time for reviewing this. I'll let you and @bradenmacdonald decide how to handle this.

@robrap You can hold off until I get thumbs from one of the above, if you prefer.

jcdyer · 2017-10-26T19:06:17Z

@mtyaka @bradenmacdonald Cancel that earlier request. It looks like I'm missing some test coverage on the new waffle flag. Let me get that in place, and I'll update with a new review request.

jcdyer · 2017-10-26T20:20:40Z

@bradenmacdonald @mtyaka This is ready for review by one of you.

jcdyer · 2017-10-27T16:08:38Z

There were some bokchoy errors, because I didn't realize that library content is rendered using the vert_module.html template. I set it up not to track completion. We can discuss if that needs to be handled differently, and perhaps create a new ticket if it does. I suspect the library content module will need to be treated as a custom completable, and track completion of its children independently.

bradenmacdonald

@jcdyer I had some minor feedback, but nothing blocking. This looks good and works well.

I tested this: per the nice testing instructions
I read through the code
I checked for accessibility issues: shouldn't be any, but we'll have to be careful about a11y when adding the "complete on scroll into viewport" functionality.
Includes documentation: docstrings and comments at least

This will need a follow-up to be more specific about when to complete blocks. It currently marks them complete when the vertical is first visited, but it should do it when the block has has been viewed, which will require checking coordinates of the block's containing element in the viewport.

Yep. I think it makes sense to defer that until we have a UI for indicating completion status though: otherwise, assuming we collect completion data for some time and then later display it via a UI, users may be surprised to see "uncompleted" blocks scattered throughout units they've already completed, simply because they didn't scroll all the way down the page to see that last discussion block, etc.

bradenmacdonald · 2017-10-31T01:30:20Z

common/lib/xmodule/xmodule/tests/test_vertical.py

This docstring doesn't seem to explain very clearly what this method does.

bradenmacdonald · 2017-10-31T01:32:16Z

common/lib/xmodule/xmodule/tests/test_vertical.py

I think this test could use a docstring - it's not immediately obvious exactly what it's doing.

bradenmacdonald · 2017-10-31T01:39:28Z

lms/djangoapps/courseware/module_render.py

Are these log lines just for development?

Yes. I've removed them, and included a comment to explain the early return.

bradenmacdonald · 2017-10-31T02:14:45Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

If I view a sequential like "Lesson 1 - Getting Started" in the demo course and click from the first vertical to another vertical and then back to the first, I see that it still POSTs a publish_completion event to the server - it has "forgotten" that it already marked the block as complete.

I was going to suggest calling delete block.dataset.completableByViewing; after the AJAX request completes successfully, but I think that each vertical gets rendered from an HTML string when switching verticals in a sequential, so that approach wouldn't work.

If you think it's worth doing, maybe add a global variable to track already-submitted completion events? Or we can ignore the duplicates, as it's not a huge optimization.

robrap · 2017-10-31T13:12:16Z

I have not yet reviewed this, but just wanted to leave some high-level comments:

I imagine that edX is going to need a batch call before this can be turned on because there are some courses with huge verticals.
There was discussion of a timer before marking (not sure if that was implemented or future). For future required features, it might be helpful to have a TODO with a description of what is coming and/or a link to a Confluence page or JIRA ticket.
For scrolling/focus based events, would each one have a timer or would a quick scroll mark things as complete? This will relate both to the need for batch eventing and/or the implementation of batch eventing. Also, I'm not sure if trying to time this details with also having a UI will work, unless you want to add another flag for whether or not to do this.

One of the assumptions I had was that the Completion API would be implemented and merged such that it could be turned on for edX.org without performance issues. It is ok if work is being chunked to get smaller PRs. But I also want to ensure that some details won't get dropped.

Thanks.

jcdyer · 2017-11-10T21:01:44Z

@bradenmacdonald Some of your comments were attached to a particular commit. I rebased against the latest master to resolve merge issues & those comments (about the vertical_student_view.js file) got lost, but I've addressed the issues you mentioned. Ready for another round.

jcdyer · 2017-11-13T15:49:44Z

jenkins run bokchoy

jcdyer · 2017-11-13T15:50:20Z

Failure due to: https://build.testeng.edx.org/job/edx-platform-bok-choy-pr/45642/

Trying rerun.

bradenmacdonald · 2017-11-13T19:55:26Z

@jcdyer I just gave this another spin. It seems to be working very well; there's no duplicate events when changing tabs in the sequential, nor are there repeat events when refreshing the whole page.

Code in your recent commits looks good to me too. Thanks!

So my 👍 stands.

jcdyer · 2017-11-15T15:24:55Z

Great! Thanks, @bradenmacdonald .

@robrap This is now ready for your review.

robrap · 2017-11-15T21:36:03Z

@jcdyer: I'm working with the team to get this work assigned and scheduled. I will let you know as soon as I hear.

robrap

Thanks @jcdyer. This should be relatively quick.

robrap · 2017-11-16T16:42:54Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

+// Using a global variable for SEEN_COMPLETABLES because this needs to track
+// completables between separate loads of the same vertical (when a learner
+// goes from one tab to the next, and then navigates back within a given
+// sequential).


It would be nice if this comment more fully stood on its own for someone reading this out of context. For example:

// The vertical marks blocks that are completable by viewing as complete as they // are seen. The global variable SEEN_COMPLETABLES tracks blocks between // separate loads of the same vertical (when a learner goes from one tab to the // next, and then navigates back within a given sequential) to protect against // duplicate calls to the server.

Copy pasting this with a slight tweak to the first sentence to simplify the grammatical structure.

robrap · 2017-11-16T16:52:05Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

+            if (block.dataset.completableByViewing === undefined) {
+                return;
+            }
+            if (blockKey && !SEEN_COMPLETABLES.has(blockKey)) {


Can you add a TODO comment regarding outstanding work, possibly including ticket id(s)?
Outstanding work:

Limiting server calls via a combo of batching or only viewing in focus/viewport to protect server against verticals with large numbers of blocks.

Configurable delay that must expire before counting as complete, so quick navigation doesn't automatically mark everything.

See earlier comment around this:
https://github.com/edx/edx-platform/pull/16234#issuecomment-340757576

robrap · 2017-11-16T16:55:27Z

lms/djangoapps/completion/services.py

+
+    def get_completions(self, candidates):
+        """
+        Given an iterable colletion of block_keys in the course, returns a


typo: colletion => collection

robrap · 2017-11-16T17:00:55Z

lms/templates/vert_module.html

 <div class="vert-mod">
 % for idx, item in enumerate(items):
-  <div class="vert vert-${idx}" data-id="${item['id']}">
+<div class="vert vert-${idx}" data-id="${item['id']}" \


Can you add back indent?

robrap · 2017-11-16T17:08:38Z

requirements/edx/base.txt

 # Support for plugins
 web-fragments==0.2.2
-xblock==1.0.0
+git+https://github.com/edx/XBlock@xblock-1.1.0#egg=XBlock==1.1.0


Working on updating. Will be 1.1.1.

https://pypi.python.org/pypi/XBlock/1.1.1 is available.

jcdyer · 2017-11-17T15:59:32Z

@robrap. Ready for another look.

jcdyer · 2017-11-17T16:15:22Z

Tests don't seem to be running right now. I'll try to get them running again in an hour or so. (FYI @edx/testeng)

michaelyoungstrom · 2017-11-17T16:46:17Z

Sorry @jcdyer , we've been having issues with Jenkins over the last 2 days. Your tests are running right now, and we'll continue monitoring.

robrap

Thanks @jcdyer. Minor addition when you squash.

robrap · 2017-11-17T21:27:08Z

common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js

@jcdyer: Thanks. Can you please change this first line to when you squash?:

// TODO: EDUCATOR-1778

In the future, I may ask you to use "TODO: EDUCATOR-1778" so we can track blockers to enabling for edx.org. Thank you!

Great idea.

pomegranited · 2017-11-20T02:39:37Z

@jcdyer Is there documentation for the edx-platform waffle flags anywhere? Would be worth updating it to include this new one.

jcdyer · 2017-11-20T16:34:10Z

@pomegranited I actually don't know if there is a central documentation source for the waffle flags and switches, though I agree it's a little hard to work with them as the prefixing makes it so the full switch name doesn't actually show up anywhere. I'll at least as the full name in a comment on the switch itself, and if @robrap knows of a place to document it, I'll add it there too.

* Vertical marks blocks completed when viewed. * Functionality is hidden behind a waffle switch * Submissions from front-end are limited to known-incomplete blocks * Upgrades xblock to version 1.1.1 * Related future requirements listed in TODO tagged with EDUCATOR-1778 and relevant opencraft OC-* ticket IDs. OC-3088

edx-pipeline-bot · 2017-11-29T21:46:09Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, November 30, 2017.

edx-pipeline-bot · 2017-11-30T16:24:19Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-pipeline-bot · 2017-11-30T16:39:43Z

EdX Release Notice: This PR has been rolled back from the production environment.

jcdyer force-pushed the cliff/complete-default-blocks branch from c34f29a to d7b2147 Compare October 17, 2017 18:30

e-kolpakov mentioned this pull request Oct 18, 2017

CompletionXBlockMixin + tests openedx/XBlock#368

Merged

2 tasks

jcdyer force-pushed the cliff/complete-default-blocks branch 2 times, most recently from 75caa96 to 5ee299b Compare October 18, 2017 10:45

robrap reviewed Oct 19, 2017

View reviewed changes

jcdyer mentioned this pull request Oct 20, 2017

Handle completion events #16112

Merged

4 tasks

jcdyer force-pushed the cliff/complete-default-blocks branch 5 times, most recently from 9f6a836 to 5856a1b Compare October 23, 2017 15:04

jcdyer mentioned this pull request Oct 23, 2017

Complete scored blocks #16186

Merged

2 tasks

jcdyer force-pushed the cliff/complete-default-blocks branch 3 times, most recently from 31cb2fa to be7e979 Compare October 26, 2017 18:39

jcdyer force-pushed the cliff/complete-default-blocks branch 2 times, most recently from a33011c to 3311e11 Compare October 27, 2017 14:48

bradenmacdonald approved these changes Oct 31, 2017

View reviewed changes

jcdyer force-pushed the cliff/complete-default-blocks branch from 072d1d9 to 74a90df Compare November 10, 2017 20:48

jcdyer force-pushed the cliff/complete-default-blocks branch from ef9e6bd to a587933 Compare November 10, 2017 22:04

jcdyer force-pushed the cliff/complete-default-blocks branch from a587933 to 8314a6e Compare November 15, 2017 20:05

jcdyer mentioned this pull request Nov 15, 2017

Define custom completion for VideoModule #16569

Merged

4 tasks

robrap reviewed Nov 16, 2017

View reviewed changes

robrap approved these changes Nov 17, 2017

View reviewed changes

jcdyer force-pushed the cliff/complete-default-blocks branch from c3ab5b6 to 5dbb084 Compare November 17, 2017 21:46

bradenmacdonald mentioned this pull request Nov 22, 2017

MCKIN-5897 - Added Completion for Ooyala Player openedx-unsupported/xblock-ooyala#78

Merged

2 tasks

jcdyer force-pushed the cliff/complete-default-blocks branch 3 times, most recently from e419350 to 11f8d51 Compare November 27, 2017 15:08

jcdyer force-pushed the cliff/complete-default-blocks branch from 11f8d51 to b8202e4 Compare November 28, 2017 20:07

jcdyer merged commit c1e878f into openedx:master Nov 28, 2017

bradenmacdonald deleted the cliff/complete-default-blocks branch November 29, 2017 22:21

symbolist mentioned this pull request May 30, 2018

Mark blocks completed on view in render_xblock view. #18273

Merged

2 tasks

Complete default blocks #16234

Complete default blocks #16234

Uh oh!

Conversation

jcdyer commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Oct 13, 2017

Uh oh!

jcdyer commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arizzitano commented Oct 18, 2017

Uh oh!

jcdyer commented Oct 18, 2017

Uh oh!

jcdyer commented Oct 18, 2017

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap commented Oct 23, 2017

Uh oh!

jcdyer commented Oct 26, 2017

Uh oh!

jcdyer commented Oct 26, 2017

Uh oh!

jcdyer commented Oct 26, 2017

Uh oh!

jcdyer commented Oct 27, 2017

Uh oh!

bradenmacdonald left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap commented Oct 31, 2017

jcdyer commented Oct 13, 2017 •

edited

Loading

jcdyer commented Oct 18, 2017 •

edited

Loading

bradenmacdonald left a comment •

edited

Loading