Add block completion value as optional field in course_blocks.api. #16674

tomaszgy · 2017-11-24T18:04:40Z

Allow `completion` in `requested_fields`

This PR extends Course Blocks API to allow user to pass completion into requested_fields. API returns block level completion data for each block (0.0 - 1.0 or None if not completable).

JIRA tickets: Implements OC-3114.

Discussions: This PR is a part of the implementation of the Completion API.

Dependencies: It updates version xblock in requirements to 1.1.1.

Sandboxes URLs:
LMS: https://pr16674.sandbox.opencraft.hosting
Studio: https://studio-pr16674.sandbox.opencraft.hosting/

Merge deadline: TBD

Testing instructions:
I

Run included tests from lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py which is a part of this PR.

II

Go into Django shell.
Create entries for the completion using BlockCompletion as in Block completion model #16047:

>>> from lms.djangoapps.completion.models import BlockCompletion
>>> from django.contrib.auth.models import User
>>> from opaque_keys.edx.keys import CourseKey, UsageKey
>>> BlockCompletion.objects.submit_completion(user=User.objects.get(...), course_key=CourseKey.from_string('...'), block_key=UsageKey.from_string('...'), completed=0.5)

Test that the following API call returns blocks with proper completion values:
http://localhost:18000/api/courses/v1/blocks/?course_id=course-v1:edX%2BDemoX%2BDemo_Course&all_blocks=true&requested_fields=completion&depth=10

(Make sure the id in the course_id is the same as course_key in point 2)

Author notes and concerns: None

Reviewers

@bradenmacdonald
@jcdyer
edX reviewer[s] TBD

Settings
As setup.py file has been changed to register new transformer you'll need to do python setup.py install inside your container.

EDIT:
Rebased on master from 1902951aeb3c0e049e221683e8dc11985102a7ee.
Squashed and then rebased from db3940d onto master.
Signed-off-by: Tomasz Gargas tomasz@opencraft.com

openedx-webhooks · 2017-11-24T18:04:51Z

Thanks for the pull request, @tomaszgy! 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.

bradenmacdonald

Looks like this is on the right track :)

bradenmacdonald · 2017-11-24T19:11:31Z

lms/djangoapps/course_api/blocks/transformers/block_completion.py

Yep, please go ahead and do that now :)

bradenmacdonald · 2017-11-24T19:14:32Z

lms/djangoapps/course_api/blocks/transformers/block_completion.py

Could we just call this "completion" ? Then it will match the request parameter, and we can use the same field for storing aggregate completions in the future once we implement that.

bradenmacdonald · 2017-11-24T19:16:23Z

lms/djangoapps/course_api/blocks/transformers/block_completion.py

Nit: I recommend putting trailing commas on lines like this.

bradenmacdonald · 2017-11-29T22:33:55Z

lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py

Try using BlockCompletion.objects.submit_completion(...) instead.

Thanks! When I used map_into_course in the transform() method as you suggested in OC-3114, the problem is solved.

@tomaszgy Great - but even still, I think we should still be using submit_completion(...) here, so that BlockCompletion objects get created consistently using that method everywhere in the codebase. You may also need to use this decorator on this test method, to enable the completion API.

bradenmacdonald · 2017-12-01T19:02:21Z

@tomaszgy You've still got a quality issue:

23:42:12 lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py (99.1%):
23:42:12     107: C0330: (bad-continuation), : Wrong hanging indentation before block.

bradenmacdonald · 2017-12-01T19:24:32Z

lms/djangoapps/course_api/blocks/transformers/blocks_api.py

@tomaszgy Adding this new transformer to BlocksAPITransformer causes it to generate its data every time a course's block structure is created, which results in one additional MySQL query per course when using the course blocks API. That is why a number of tests are failing.

Since we generally won't need to read this data and it doesn't affect block visibility or anything, I think it would be better to only execute this transformer when its field has been explicitly requested, which would involve adding logic here to only add this new transformer when completion is in requested_fields (or pass requested_fields to this class's init method and keep the logic here).

@nasthagiri does that make sense?

jcdyer · 2017-12-04T15:47:22Z

jenkins run quality

jcdyer

The code looks good, with a couple of minor issues mentioned. When I try to access the API, though, I get a 500 error: "TransformerException at /api/courses/v1/blocks/: The following requested transformers are not registered: set(['blocks_api:completion'])"

Do I need to configure something?

jcdyer · 2017-12-04T16:04:49Z

lms/djangoapps/course_api/blocks/api.py

    include_special_exams = False
+    include_completion = requested_fields is not None and 'completion' in requested_fields
    if requested_fields is not None and 'special_exam_info' in requested_fields:
        include_special_exams = True


Nit: There are multiple lines here checking if requested_fields is not None. Also, include_completion is injected into the middle of the logic for checking special_exams (L54 & LL57-58). Consider changing LL54-58 to

if requested_fields is None: requested_fields = [] include_completion = 'completion' in requested_fields include_special_exams = 'special_exam_info' in requested_fields

jcdyer · 2017-12-04T16:09:48Z

lms/djangoapps/course_api/blocks/transformers/block_completion.py

+    COMPLETION = 'completion'
+
+    def __init__(self):
+        pass


Are you deliberately trying to suppress the behavior of the superclass's __init__() method, or is this just vestigial code? This could either use an explanatory comment, or should be deleted.

Well spotted :-) Actually superclass doesn't define __init__ and inherits itself from object but I'll add a call to superclass's constructor.

If you're not doing anything else, just delete the definition altogether. If you leave it out, it will search the superclasses automatically, just like for any other method.

jcdyer · 2017-12-04T16:18:21Z

lms/djangoapps/course_api/blocks/views.py

+          * completion: (float or None) The level of completion of the block.
+            Its value can vary between 0.0 and 1.0 or be equal to None
+            if block is not completable.
+


This should mention that the value is only included if "completion" included in requested_fields. See graded or format below.

nasthagiri · 2017-12-05T20:54:50Z

Hey there! Thanks for these changes. Overall, they look great.

When you have a moment, please read the Transformer Version Updates section of the wiki.

Essentially, if you can merge these changes in 2 phases: initially, setting the WRITE_VERSION=1 and then following up with setting the READ_VERSION=1 - it will allow for smoother releases into environments (such as edx.org) with blue-green deployments. FYI @edx/devops

Also, you'll want to update setup.py to include the new Transformer.

nasthagiri · 2017-12-05T20:56:35Z

lms/djangoapps/course_api/blocks/transformers/__init__.py

+    SupportedFieldType(
+        BlockCompletionTransformer.COMPLETION,
+        BlockCompletionTransformer,
+        'completion'


Future note to self (and others): It would be great if we can make this pluggable, so new Transformers don't need to manually update this list - and can be automatically extracted from registered transformers.

@nasthagiri I remember implementing a registry for something once, and I found two basic ways of doing it: The first is by creating a registry class with a register() decorator function:

class Registry(object): def __init__(self): self.registered = [] def register(self, registrant): self.registered.append(registrant) return transformer transformers = Registry() @transformers.register class MyTransformer(BlockTransformer): pass

The other uses a metaclass defined on the superclass. Then when the class gets constructed, it is automatically registered in the metaclass. Django models use this method.

The major differences are:

that the decorator is visible to the programmer, while the metaclass is hidden.

programmers can opt-out of the decorator by not including it, but not the metaclass (unless the metaclass explicitly provides a hook, like writing):
class Registerable(Registry):
register = False
The downside of allowing programmers to opt out (or more accurately opt not to opt-in) is that you also make it possible to forget to opt-in.

that decorators are easier to read and write than metaclasses.

nasthagiri · 2017-12-05T20:58:25Z

lms/djangoapps/course_api/blocks/api.py

    # create ordered list of transformers, adding BlocksAPITransformer at end.
    transformers = BlockStructureTransformers()
    include_special_exams = False
+    include_completion = requested_fields is not None and 'completion' in requested_fields


Future note to self (and others): If there are any great ideas of making this automatically pluggable as well, welcoming ideas/contributions.

tomaszgy · 2017-12-06T17:15:27Z

@jcdyer and @nasthagiri - thanks for your reviews! I've just addressed your comments. Please have a look and if all's good I'll squash the commits.

@jcdyer, thanks for spotting TransformerException - it was due to my transformer not being registered in setup.py as @nasthagiri pointed out. I should have spotted it before but in its initial version my transformer was living inside BlocksAPITransformer which has already been registered.

@nasthagiri, I've removed for now READ_VERSION=1, leaving just WRITE_VERSION=1. Do I understand correctly than I should open another PR or re-open this one, more than 24h after this PR is merged?

nasthagiri · 2017-12-06T22:12:35Z

Thanks @tomaszgy
I double checked with our devops team here. We'd like to follow the following process for now:

First PR: Merge your PR with READ_VERSION=0 and WRITE_VERSION=1
Second PR: Create and be ready to merge your 2nd PR with READ_VERSION=1
Wait until you see a message on the First PR that says:

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

Merge your Second PR

We hope that works for your team. Thanks.

jcdyer · 2017-12-07T20:34:19Z

@tomaszgy There are a bunch of test failures. Do you know what's causing them?

tomaszgy · 2017-12-09T22:19:38Z

@nasthagiri, thanks for the scenario!

@jcdyer, indeed, there're lots of tests failing. Many of them (e.g. this one) fail due to TransformerException('Version attributes are not set on transformer {0}.', transformer.name()) which is raised here and is an effect of READ_VERSION=0. Would you have any suggestions on that, @nasthagiri?

nasthagiri · 2017-12-09T23:18:25Z

@tomaszgy thanks for bringing up the exception issue. Can you change that particular block_structure code to remove the check for READ_VERSION=0? You are running into it since you are the first to introduce a new transformer since we designed the cache-invalidation strategy. We used that strategy for updating existing transformers, but not (yet) for introducing a new one. Thanks.

tomaszgy · 2017-12-09T23:26:11Z

Thanks for the suggestion, @nasthagiri! I've just removed the check.

tomaszgy · 2017-12-11T16:38:11Z

@nasthagiri one of the bokchoy tests (acceptance.tests.lms.test_teams.BrowseTopicsTest.test_sort_topics_2___team_count___True_ ) failed for this PR with:
Timed out waiting to load page '<common.test.acceptance.pages.lms.teams.BrowseTopicsPage object at 0x7fba5a549490>' at URL 'http://localhost:8003/courses/course-v1:test_org+306648626320279492720022822088892457631+test_run/teams/#browse'

In the meantime I opened Second PR (#16859) with READ_VERSION=1 and all tests passed there. Shall we investigate why it fails here?

CC @jcdyer

mtyaka · 2017-12-12T07:56:34Z

jenkins run bokchoy

tomaszgy · 2017-12-12T13:08:55Z

Thanks @mtyaka! :)

Signed-off-by: Tomasz Gargas <tomasz@opencraft.com>

…ansformer. Signed-off-by: Tomasz Gargas <tomasz@opencraft.com>

tomaszgy · 2017-12-13T11:32:12Z

@nasthagiri Just a status update: I've squashed and rebased this PR, now bokchoy tests pass, too. If there's anything else I should change, please let me know. If all's good, please feel free to merge it :)

Second PR with READ_VERSION=1 is in #16859.

nasthagiri · 2017-12-13T15:38:27Z

@tomaszgy Sounds good. Merging now.
FYI @edx/devops We'll want to run the generate_course_blocks management command AFTER this PR goes to stage and prod and BEFORE #16859 does. I'll create a devops ticket for this.

tomaszgy · 2017-12-13T16:13:19Z

@nasthagiri, thanks! Just rebased Second PR to make it ready for merge.

nasthagiri · 2017-12-13T16:14:05Z

Devops ticket created: https://openedx.atlassian.net/browse/DEVOPS-6904

edx-pipeline-bot · 2017-12-13T18:18:46Z

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

nasthagiri · 2017-12-13T20:17:41Z

Updating here: We've run the generate_course_blocks command on Stage. I've verified there are no exceptions raised by the new transformer.

edx-pipeline-bot · 2017-12-14T20:40:31Z

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

nasthagiri · 2017-12-15T18:30:55Z

Update: The management command was run on production after yesterday's release. We're good to move forward with the next PR.

bradenmacdonald reviewed Nov 24, 2017

View reviewed changes

bradenmacdonald reviewed Nov 29, 2017

View reviewed changes

tomaszgy changed the title ~~WIP: Add block completion value as optional field in course_blocks.api.~~ Add block completion value as optional field in course_blocks.api. Nov 30, 2017

bradenmacdonald reviewed Dec 1, 2017

View reviewed changes

tomaszgy force-pushed the tomaszgy/completion_in_blocks_api branch from 1902951 to 465e0a6 Compare December 4, 2017 13:57

jcdyer reviewed Dec 4, 2017

View reviewed changes

nasthagiri reviewed Dec 5, 2017

View reviewed changes

tomaszgy force-pushed the tomaszgy/completion_in_blocks_api branch from f487dc0 to d79a9ea Compare December 6, 2017 17:09

This was referenced Dec 11, 2017

WIP: Bump READ_VERSION of BlockCompletionTransformer to 1. open-craft/openedx-platform#89

Closed

Bump READ_VERSION of BlockCompletionTransformer to 1. #16859

Merged

tomaszgy added 2 commits December 12, 2017 14:22

Add block completion value as optional field in course_blocks.api.

64c4ae4

Signed-off-by: Tomasz Gargas <tomasz@opencraft.com>

Remove a check for READ_VERSION==0 in BlockStructureBlockData._add_tr…

d4edf33

…ansformer. Signed-off-by: Tomasz Gargas <tomasz@opencraft.com>

tomaszgy force-pushed the tomaszgy/completion_in_blocks_api branch from db3940d to d4edf33 Compare December 12, 2017 13:24

nasthagiri merged commit c1d0252 into openedx:master Dec 13, 2017

bradenmacdonald deleted the tomaszgy/completion_in_blocks_api branch December 19, 2017 22:05

Add block completion value as optional field in course_blocks.api. #16674

Add block completion value as optional field in course_blocks.api. #16674

Uh oh!

Conversation

tomaszgy commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Allow completion in requested_fields

Uh oh!

openedx-webhooks commented Nov 24, 2017

Uh oh!

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

bradenmacdonald commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer commented Dec 4, 2017

Uh oh!

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

jcdyer Dec 5, 2017 • 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!

nasthagiri commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer Dec 6, 2017 • 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!

tomaszgy commented Dec 6, 2017

Uh oh!

nasthagiri commented Dec 6, 2017

Uh oh!

jcdyer commented Dec 7, 2017

Uh oh!

tomaszgy commented Dec 9, 2017

Uh oh!

nasthagiri commented Dec 9, 2017

Uh oh!

tomaszgy commented Dec 9, 2017

Uh oh!

tomaszgy commented Nov 24, 2017 •

edited

Loading

Allow `completion` in `requested_fields`

jcdyer Dec 5, 2017 •

edited

Loading

jcdyer Dec 6, 2017 •

edited

Loading