Mark blocks completed on view in render_xblock view. #18273

symbolist · 2018-05-28T13:07:35Z

Webviews in mobile apps do not encapsulate leaf-blocks in html for vertical blocks. Instead, they use the "chromeless-courseware" render_xblock view. This PR adds this missing completion functionality to render_xblock view.

Part of Completion API work.
Previous PR: https://github.com/edx/edx-platform/pull/16234.

JIRA tickets: OSPR-2441

Screenshots: No change to UI.

Sandbox URL: https://pr18273.sandbox.opencraft.hosting/

Merge deadline: None

Testing instructions:

In /admin/waffle/switch/ add and activate completion.enable_completion_tracking to turn on completion tracking.
Complete on view tracking happens when the user is visiting courseware pages and when the render_xblock view is displayed in the mobile apps.
To test courseware pages, visit a course section. When a vertical, which has XBlocks to which this completion strategy is applicable (e.g. html XBlock), is viewed the first time, a publish_completion ajax call is made to the server for each child XBlock when it has been viewed from start to end for 5 seconds.
To test render_xblock view, visit a url of the form /xblock/<block_id>. If this completion strategy is applicable to the XBlock, a publish_completion ajax call is made to the server when it is viewed for the first time from start to end for 5 seconds.
To help with testing, completion data can be reset via the following steps:

 ./manage.py lms shell
from completion.models import BlockCompletion
BlockCompletion.objects.all().delete()

Reviewers

openedx-webhooks · 2018-05-28T13:07:49Z

Thanks for the pull request, @symbolist! I've created OSPR-2441 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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

pomegranited · 2018-05-28T19:22:14Z

common/lib/xmodule/xmodule/vertical_block.py

The vertical block is still referencing the completion service, so it still wants completion.

pomegranited · 2018-05-28T19:43:55Z

lms/static/completion/js/CompletionOnViewService.js

Since we only include this service when enableCompletionOnViewService is set, it's ok to forego this check.

mduboseedx · 2018-05-29T14:48:08Z

Hi @symbolist, thank you for the contribution! Feel free to ping me when this is ready for review and passes all the tests.

Thanks!

symbolist · 2018-05-30T15:34:19Z

Hi @mduboseedx! Thanks. Yes, I'll let you know when we have completed the code review on our side.

symbolist · 2018-05-30T16:56:45Z

@pomegranited I have added the Python tests. I could not find tests for the courseware views so I have added those as well. There are a few small issues that need to be resolved. But otherwise you can review this now.

symbolist · 2018-05-30T18:41:41Z

jenkins run python

symbolist · 2018-05-30T19:01:40Z

jenkins run bokchoy

pomegranited · 2018-05-30T20:01:07Z

@symbolist I'm getting this JS error when I load the unit in LMS (same in chromeless view):

ReferenceError: markBlocksCompletedOnViewIfNeeded is not defined

Are you?

symbolist · 2018-05-30T20:08:44Z

@pomegranited Can you pull again? That is due to a wrong attempt at fixing a quality issue that I reverted. https://github.com/edx/edx-platform/pull/18273/files#diff-4a1680b05e9915f0d36cc9fe5f3173b4R26

(Btw you have tagged @UmanShahzad).

pomegranited · 2018-05-31T00:52:19Z

Sorry @symbolist , this still isn't working for me 😦 I am seeing the updated code in my browser, so the assets are rebuilding fine, but the markBlocksCompletedOnViewIfNeeded method is still undefined.

I couldn't find another example of a requireJS file on the platform that just exports a function in this way -- maybe if you structure it like the other requirejs files and encapsulate the function in an object/class, it will work?

symbolist · 2018-05-31T08:26:02Z

@pomegranited I suspect it has to do with stale caches. I can toggle it between not-working and working by adding and removing the default keyword.

If the default keyword is set, line 8 of static/bundles/CompletionOnViewService.js in the developer tools Sources tab will be:

/* WEBPACK VAR INJECTION */(function($) {/* harmony export (immutable) */ __webpack_exports__["default"] = markBlocksCompletedOnViewIfNeeded;

If the default keyword is NOT set, it is:

/* WEBPACK VAR INJECTION */(function($) {/* harmony export (immutable) */ __webpack_exports__["markBlocksCompletedOnViewIfNeeded"] = markBlocksCompletedOnViewIfNeeded;

If you are seeing the former, can you try doing a git clean -dfx before updating assets and make sure Chrome Developer Tools > Network > Disable cache is enabled?

symbolist · 2018-05-31T23:01:47Z

@pomegranited In case it is helpful I have refreshed the sandbox with the latest commits and enabled completion on it: https://pr18273.sandbox.opencraft.hosting/.

pomegranited

👍 Thanks for your patience, and for setting up the sandbox @symbolist ! Turns out my issue locally was due to docker-sync, and so my devstack wasn't seeing your updated files. Sorry it took me so long to find it!

Just one nit, and tests need to pass, but this is good to go upstream after that.

I tested this using the PR test instructions, and also tested videos to ensure that they are not sending completion until the last few seconds of the video.
I read through the code
~~I checked for accessibility issues~~
Includes documentation

pomegranited · 2018-05-30T20:58:48Z

lms/djangoapps/courseware/tests/test_views.py

nit: could you remove most of these blank lines? Don't need so much spacing, I think.

pomegranited · 2018-05-31T23:03:09Z

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

👍 for making these tests better 😄

symbolist · 2018-06-01T18:53:08Z

jenkins run all

mduboseedx · 2018-06-04T12:58:49Z

@symbolist It looks like tests are passing. Is this ready for review?

bradenmacdonald · 2018-06-04T17:08:06Z

@jcdyer FYI, can you take at least a quick look at this too?

jcdyer · 2018-06-04T20:03:02Z

@bradenmacdonald Will do.

symbolist · 2018-06-04T20:46:46Z

@mduboseedx thanks for keeping track! :) We will let you know as soon as this is ready for review from the edX side (though this is part of a series of work and @jcdyer may already have a reviewer in mind).

symbolist · 2018-06-08T13:23:27Z

@jcdyer I have pushed the bokchoy tests to this branch and the tests have passed.

jcdyer

Thanks for this @symbolist. It looks good to me.

jcdyer · 2018-06-08T15:18:15Z

@staubina @iloveagent57 This is ready for an upstream review. I'm not sure who is planning to take this one. This is to resolve the issue of completions not getting submitted when viewing pages on mobile.

schenedx

I got a few question in this PR.
Rest looks pretty good to me.

schenedx · 2018-06-11T16:40:15Z

lms/djangoapps/courseware/views/views.py

Why using string to specify Boolean values?
If the values here are meant for the javascript, can we use Boolean to string str()?

I have changed this to a Boolean.

schenedx · 2018-06-11T16:42:56Z

openedx/core/lib/completion.py

Should these methods be part of the https://github.com/edx/completion repo?

@jcdyer Should these methods be moved onto the CompletionService? I see there are a couple of similar helper methods on it.

@symbolist The CompletionService looks like it would be a good place for it. 👍

Moved to CompletionService.

schenedx · 2018-06-11T16:43:24Z

openedx/core/lib/tests/test_completion.py

Same with this test, shouldn't this test be part of https://github.com/edx/completion?

edx/completion isn't really the right place, since these tests need modulestore stuff. I put all of the integration tests between the completion repo and edx-platform here: https://github.com/open-craft/edx-platform/tree/9a6c4033f383f4e05c3390210853ef60d40d4b6a/openedx/tests/completion_integration

Some tests were moved to the completion package and others to penedx/tests/completion_integration/test_services.py.

jcdyer · 2018-06-11T19:49:01Z

openedx/core/lib/xblock_utils/__init__.py

I like the way this lets us put more of the state directly on the completable xblocks without requiring XBlock authors to be explicitly aware of how completion works, while minimizing the specialness of verticals. 👍

iloveagent57

Nice work, @symbolist ! This looks pretty good, just a couple of small nits from me (and an echo of comments shared by others about relocating some code and tests). Thanks!

iloveagent57 · 2018-06-11T19:22:43Z

common/lib/xmodule/xmodule/vertical_block.py

nit: completion_service.get_completion_by_viewing_delay_ms() will return the same thing on each call, so you could just store it in a variable outside your for loop.

I have updated this.

iloveagent57 · 2018-06-11T19:42:32Z

openedx/core/lib/completion.py

Nit: consider renaming this to something like can_block_be_completed_on_view. Just because a block meets the conditions in the body of this function, doesn't mean it should be marked complete yet - we still have to know if it's already been completed (as you check for below).

Renamed to can_block_be_completed_on_view().

iloveagent57 · 2018-06-11T19:48:47Z

openedx/core/lib/tests/test_completion.py

edx/completion isn't really the right place, since these tests need modulestore stuff. I put all of the integration tests between the completion repo and edx-platform here: https://github.com/open-craft/edx-platform/tree/9a6c4033f383f4e05c3390210853ef60d40d4b6a/openedx/tests/completion_integration

symbolist · 2018-06-18T11:27:04Z

The test failures seem to be because putting the git url for the completion repo did not work correctly. It would just be simpler to wait till the completion package update is released and then update this in one go.

staubina · 2018-06-18T11:54:39Z

Just checking in on this PR, I think @iloveagent57 would be a better reviewer on this ticket as he did work on the original features. If @iloveagent57 is not available I can provide support.

symbolist · 2018-06-18T13:20:17Z

@staubina Thanks! @iloveagent57 is already reviewing it.

schenedx · 2018-06-19T16:35:55Z

@symbolist I am available to do another round of review also. Please ping us when you believe this is now good to be reviewed again

symbolist · 2018-06-19T19:43:04Z

jenkins run all

symbolist · 2018-06-19T21:26:26Z

Ah looks like I was updating the dependency in the wrong file.

@schenedx I was waiting for the changes that were moved to the completion package in openedx/completion#19 to get merged and released to PyPi. They were earlier today. I have rebased this PR and updated the edx-completion version. Will tag you as soon as the tests pass.

symbolist · 2018-06-20T14:15:49Z

@schenedx This is ready for another round of review from you. You can look at the commits starting from "Move utils to CompletionService."

schenedx

Nice work! 👍

iloveagent57 · 2018-06-21T17:08:53Z

@symbolist Looks good! Could you please rebase/squash, then I'll merge this? Thanks!

edx-status-bot · 2018-06-22T14:45:15Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2018-06-22T18:46:06Z

@symbolist 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2018-06-22T20:59:00Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, June 25, 2018.

edx-pipeline-bot · 2018-06-25T15:18:30Z

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

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 28, 2018

pomegranited reviewed May 28, 2018

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 29, 2018

symbolist force-pushed the symbolist/chromeless-xblock-completion branch from ddcc920 to a151cc7 Compare May 30, 2018 15:27

symbolist force-pushed the symbolist/chromeless-xblock-completion branch from 0da91a4 to c6f3075 Compare May 30, 2018 19:38

pomegranited approved these changes May 31, 2018

View reviewed changes

pomegranited changed the title ~~[WIP] Mark blocks completed on view in render_xblock view.~~ Mark blocks completed on view in render_xblock view. Jun 1, 2018

jcdyer approved these changes Jun 8, 2018

View reviewed changes

jcdyer requested a review from staubina June 8, 2018 15:18

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jun 11, 2018

schenedx reviewed Jun 11, 2018

View reviewed changes

jcdyer reviewed Jun 11, 2018

View reviewed changes

iloveagent57 reviewed Jun 11, 2018

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Jun 11, 2018

symbolist mentioned this pull request Jun 15, 2018

Add helper methods for complete-on-view to Service openedx/completion#19

Merged

12 tasks

symbolist force-pushed the symbolist/chromeless-xblock-completion branch from 9a6c403 to 532ac86 Compare June 15, 2018 21:01

jcdyer mentioned this pull request Jun 18, 2018

Backport completion-by-viewing logic edx/edx-solutions-edx-platform#1104

Merged

1 task

symbolist force-pushed the symbolist/chromeless-xblock-completion branch 2 times, most recently from c9e87e5 to 29f1555 Compare June 19, 2018 21:23

symbolist force-pushed the symbolist/chromeless-xblock-completion branch from adcf2e5 to d936874 Compare June 20, 2018 11:41

schenedx approved these changes Jun 21, 2018

View reviewed changes

Add complete on view support to render_xblock view.

adaa21b

symbolist force-pushed the symbolist/chromeless-xblock-completion branch from d936874 to adaa21b Compare June 22, 2018 14:19

iloveagent57 merged commit 8b1aad0 into openedx:master Jun 22, 2018

bradenmacdonald deleted the symbolist/chromeless-xblock-completion branch June 30, 2018 03:29

openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 22, 2021

Mark blocks completed on view in render_xblock view. #18273

Mark blocks completed on view in render_xblock view. #18273

Uh oh!

Conversation

symbolist commented May 28, 2018 • edited by iloveagent57 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented May 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mduboseedx commented May 29, 2018

Uh oh!

symbolist commented May 30, 2018

Uh oh!

symbolist commented May 30, 2018

Uh oh!

symbolist commented May 30, 2018

Uh oh!

symbolist commented May 30, 2018

Uh oh!

pomegranited commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symbolist commented May 30, 2018

Uh oh!

pomegranited commented May 31, 2018

Uh oh!

symbolist commented May 31, 2018

Uh oh!

symbolist commented May 31, 2018

Uh oh!

pomegranited 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!

symbolist commented Jun 1, 2018

Uh oh!

mduboseedx commented Jun 4, 2018

Uh oh!

bradenmacdonald commented Jun 4, 2018

Uh oh!

jcdyer commented Jun 4, 2018

Uh oh!

symbolist commented Jun 4, 2018

Uh oh!

symbolist commented Jun 8, 2018

Uh oh!

jcdyer left a comment

Choose a reason for hiding this comment

Uh oh!

jcdyer commented Jun 8, 2018

Uh oh!

schenedx 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!

symbolist commented May 28, 2018 •

edited by iloveagent57

Loading

pomegranited commented May 30, 2018 •

edited

Loading

pomegranited left a comment •

edited

Loading

jcdyer Jun 11, 2018 •

edited

Loading