Handle completion events

[jcdyer]

Handle completion events

Take events of type "completion" or "progress" and create or update BlockCompletion objects from them.

Reviewers

• OpenCraft: @bradenmacdonald
• edX: @robrap

Steps to test

1. Create a course with a block that emits the existing "progress" events (ooyala player, for instance).
2. View the block.
3. Confirm that it creates BlockCompletion objects.

Note: Blocks do not yet exist that emit the new "completion" events, so they cannot be tested directly without creating such a block manually, but there is a test to demonstrate the behavior with a minimal test block that submits completion events in this PR.

Reference issues

OC-3087, MCKIN-5893

Before merge

• Squash commits
• Rebase against edx:master after cliff/block-completion-model merges.

[openedx-webhooks]

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.

[bradenmacdonald]

LGTM and seems to work :) Had a couple comments/questions, but nothing blocking.

👍

• I tested this: Modified problem-builder to emit a progress event, and confirmed it created a correct BlockCompletion entry
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: docstrings, yes

[bradenmacdonald]

Can you also please just add explicitly that "New XBlocks should use the completion event instead of this deprecated progress event."

[robrap]

Maybe additionally rename to handle_deprecated_progress_event?

[bradenmacdonald]

Would it be better to declare this special_event mapping outside of the publish method?

Also, do you think it might be worth still calling track_function() for these types of events? I'm assuming that's what writes the event out to the tracking log, and I would think that having completion events in the tracking log would be desirable.

[jcdyer]

Well, this isn't actually a class, so publish isn't technically a method. It's a weird jumbo function that declares a bunch of inner functions (closures, technically, since they depend on variables declared within the outer function. So I can create another inner function to pick the appropriate special handler, but I can't make it, for instance, a class variable.

Do you think the else branch should not be an else, but just the thing that happens unconditionally after the special handling? That seems sensible, but it's a change from how grades were being handled. @robrap, do you have an opinion on this?

[bradenmacdonald]

Yep, I get it. I was just suggesting you move this local dictionary declaration up one level (still inside the outer function, but not inside this particular inner function) - it would still be in scope to be able to reference the closures like handle_grade_event. It's not a big deal though.

Do you think the else branch should not be an else, but just the thing that happens unconditionally after the special handling?

Yep, that would make more sense to me. I just don't know the reason why it wasn't the case for grades.

[jcdyer]

(I'm not sure I understand what the asides are for, so I can't judge rightly whether all that extra event context processing should be performed on grades and completions, except to the extent that I don't see any reason why they should be different)

[jcdyer]

Making that change caused several test errors, so I'm going to leave it as an either/or thing for now.

[robrap]

Thanks @jcdyer. Just a handful of minor comments.

[robrap]

Nit: Depending what happens with masquerading code, I'd consistently mention or not mention masquerading in the doc string (see handle_completion_event).

[robrap]

Maybe additionally rename to handle_deprecated_progress_event?

[robrap]

I think you wanted:

if user_id != user.id:

[robrap]

1. Could we check here for is_masquerading_as_specific_student(user, course_id) if it is a reasonable default check for future events to ensure it is not forgotten? Would it hurt to have that check for 'progress' events?
2. In all cases, is there a test for the user checks? Do you need one for the special progress handler user check as well?

[jcdyer]

I made the check general. There could be future events that would need to be run even when masquerading, but that would be an easy refactor later.

[robrap]

Some questions to possibly lead to a more clear name:
What does special_events mean? Are these just events_with_handlers? Why do these particular events get handled here?

[jcdyer]

1. They are special, just because they are handled in a particular way other than just passing them along to be logged.
2. Effectively, yes.
3. Because they are emitted from xblock javascript, and need to be handled by the backend, and this is the place where they come through. I had suggested refactoring this into a system where you could register a handler with a particular event type, and it would get run here, which would basically allow the handlers to live with the grades/completion/progress apps. The downside would be that it would spread the behavior around more, but the upside is that we could reduce the coupling to other apps. Essentially, the registry could be a class with a signgle instance that has a register() method that attaches functions to event names, and then a send() method that sends events to the appropriate handler(s).

[robrap]

Old doc string from grades?

[robrap]

I see this is test code, but maybe add something like?:

""" Progress is deprecated.  New code should use complete. """

I would add a similar note to almost any use of progress to both clarify what it is and remind people not to continue with it.

[robrap]

What does "Stateless" refer to in this context? I'm confused by it, so not sure yet if it is appropriate or not. Maybe the doc string will clarify.

[jcdyer]

I'm not sure, I was copypasting from the Graded one. How about "StubCompletableXBlock," since it's a testing stub with no real functionality.

[jcdyer]

I've looked over the most recent comments, and have a few questions. I need to run off to take care of kids, but I'll be able to put in another couple hours this weekend, and should be able to get this resolved.

[robrap]

Thanks @jcdyer. Just some minor clean up requested.

[robrap]

Sentence died off.

[robrap]

Comment still has "grade" in it, so I'm guessing this should be updated.

[robrap]

Thanks for adding this. The name test_no_handlers... is a little confusing to me. Would test_skip_handler... be accurate?

[robrap]

Are you intentionally leaving this, or was this just for debugging?

[jcdyer]

Squashed. This just needs to wait for #16047 to merge.

[jcdyer]

jenkins run js

[robrap]

@jcdyer @bradenmacdonald: IMPORTANT: Before this or any open PRs merge, can we please add the Waffle flag to ensure that no completion events will be written as these PRs land. That means protecting this method, and possibly handle_completion_event, depending on where you add the protection.

[bradenmacdonald]

@robrap Good thing you said something now as this PR is about to merge. I don't remember that part of the discussion. Why don't we want to start collecting data now? Is it because of the unknown performance implication?

[robrap]

There are a couple of reasons:

1. There are lots of dependencies between these various PRs, and we will definitely be writing partial data, and could even be writing bad data, if we don't turn it on after the work is complete.
2. Migrations, if needed, could be more of an issue with data.
3. I don't think there will be performance issues, but we don't know yet.

So, I think it is great that we are doing the development piecemeal, and agreeing to move certain cases to later stories, but a flag will allow us to know everything is properly complete (no pun intended) before we actually turn this on. :)

[bradenmacdonald]

Ok, that makes sense!

[robrap]

@jcdyer: When we do add the flag, it may make sense to sprinkle some tests with the flag turned off to ensure that no data is written.

[jcdyer]

@robrap @bradenmacdonald: Ready for a hopefully final review

I've added the waffle switch. It's currently on the model manager directly, to prevent use of submit_completion(), and I'll add it to cliff/complete-default-blocks (#16234) to prevent spurious network requests from the front-end. I don't think I need to block anything else, though.

[bradenmacdonald]

Nit: featureis

[bradenmacdonald]

Too bad this doesn't work as a class decorator.

[jcdyer]

I know. I tried. For what it's worth, it does work as a (function) decorator in versions of python after 3.2. Even then I don't think it would work on the life cycle of a class.

[robrap]

If you do go down the Waffle Flag route, I'm not sure if there would yet be a simple way to do this.

[bradenmacdonald]

I would expect that submit_completion(..., completion=x)[0].completion == x is an invariant, but I guess it makes sense that if the switch is disabled, we're not saving an entry, so the result is equivalent to setting completion to 0. Which would break fewer assumptions - returning zero or returning the value requested, even though the DB wasn't updated?

Do we need to care that if an entry was somehow already saved in the database, this would return an unsaved object with completion=0 even though the resulting (unchanged) completion value may be nonzero? I'm guessing this is not important, but worth asking.

[robrap]

• Could someone save this and accidentally create a record? I don't know enough about Django. Does that expose some risk?
• Are we thinking it is too painful to raise an exception here and keep all the calls from actually being made?

If we do stick with this type of implementation, would "isnew = False" below be better?

[robrap]

@jcdyer: I'm responding to your comment here.

Basically, I want to ensure that when the code is disabled: 1) we don't write any data, and 2) we don't break active functionality. I don't think there is a best way, so I just want to lay out my thoughts on the options, and you can decide if you want to stick with this implementation or pivot.

It seems like there are 3 options:

1. Raise an error.
2. Return a value that would make other code happy if it were using the return value (current implementation), or
3. Return a value like None that might break code looking for a proper tuple, but wouldn't cause an error otherwise.

If you raise an error here, it would ensure the code isn't doing anything we don't want it to do, but it "might" require extra code to avoid certain calls. This seems the safest, but it depends how much additional code/conditionals you need, because each additional bit of code adds more room for error.

The current implementation, where we return a legitimate value, seems the most deceptive because it may make a caller happy, but we don't know who that caller is and whether or not we wish that caller to be happy.

The third is to return a value like None. This is sort of a compromise that has some benefits and disadvantages of each of the earlier methods. It also means no one could accidentally do anything with the return valie.

Let me know what you think. Ignore my earlier comment about "isnew". My wish is that this value would be totally unimportant in this case, otherwise, we might have a bug.

[bradenmacdonald]

Nit: I'd prefer to have this override as a method decorator, like you're already doing for XBlock.register_temp_plugin - I believe waffle's override method supports that, and then the indentation is nicer though. Consider this totally optional!

[jcdyer]

It works for waffle directly, but not the edX waffle wrapper. (Until python 3.2, which gives that behavior for free with the contextlib.contextmanager decorator). Let's leave it for now.

[robrap]

I thought there was a decorator? I know there is one for our Waffle Flags. :)

[bradenmacdonald]

Tested the waffle switch. Confirmed that no BlockCompletion records get created until I enabled the completion.enable_completion_tracking switch in the django admin.

Left some feedback but it's optional / not blocking for merge.

[jcdyer]

Let's go ahead and merge, once tests pass again. I fixed the typo, but left the waffle context manager.

[jcdyer]

lms-unit-4 has been timing out. Gets hung up on one test for almost an hour. https://build.testeng.edx.org/job/edx-platform-test-subset/961961/

I rebased to the latest master, and pushed again. We'll see if it helps.

[bradenmacdonald]

jenkins run python

[robrap]

@jcdyer: The old Mercury team tended to use Waffle Flags (rather than switches) for everything. I like that the flags enable you to turn things on for single users, or beta-testers, or an individual course (see CourseWaffleFlag), that isn't available with switches. Also, I think our waffle_utils code for flags is a bit cleaner than with switches.

Would you mind reviewing some of the flags here and let let me know what you think?
https://github.com/edx/edx-platform/blob/5ae2bee17f77e345fadef3a0935dc85a890ec405/openedx/features/course_experience/__init__.py

[jcdyer]

Unless I misunderstand something, waffle flags require a request object to be present. A lot of this code is happening in signals and on models, and so doesn't have a request available. Would you recommend refactoring to block things at the view level or passing a fake request object with the relevant user attached to it?

[robrap]

We had debated using RequestCache.get_current_request() internally and taking request out of the signature, but wanted to continue to make this explicit.

Would it work to use RequestCache.get_current_request()? I've seen a couple of other examples of using the request cache in models.

[jcdyer]

Unless you think it's very important, I'd rather not do that. Another client had me spend the last several weeks removing code that was doing something similar in a different codebase, because it was making refactoring and testing a nightmare. There isn't always a current request available (tests, celery tasks, and management commands are three examples), and then to use any of this code in any of those contexts, a fake request needs to be set up by the caller and injected into the request cache. Worse, the fact that this is needed is not evident from the signatures of the methods that check the waffle flag, so you can't work with the code without intimate knowledge of its internals.

If we have enough information available inside all the contexts where the flag is checked to be able to create a fake response inside the public interfaces, then it would make sense. I haven't looked closely enough at waffle to know exactly what it needs from a request. I know we'll have a user and a course available, because it's already part of the submit_completion signature. But I don't know if that's enough, or if not, how much extra work it will be to create a suitable request.

A waffle switch gives us the simple ability to turn the feature on and off without the extra complication, and my understanding was that this was all we needed for this--to be sure we don't start saving data until the feature is complete (or complete enough).

[robrap]

Got it. A switch covers the most important need for now, and hopefully the only need. :)

[robrap]

I'm done reviewing. I just got my earlier comments in fast because I saw a comment somewhere about merging.

I'm doing my best to not be a blocker, and I think as long as we have a flag (or switch) with no bugs, it leaves us in a better place for merging faster.

[robrap]

• Could someone save this and accidentally create a record? I don't know enough about Django. Does that expose some risk?
• Are we thinking it is too painful to raise an exception here and keep all the calls from actually being made?

If we do stick with this type of implementation, would "isnew = False" below be better?

[robrap]

If you do go down the Waffle Flag route, I'm not sure if there would yet be a simple way to do this.

[robrap]

I thought there was a decorator? I know there is one for our Waffle Flags. :)

[jcdyer]

@robrap I can't reply directly to your review comments for some reason. Regarding the block record, someone could save the returned model object, but I don't believe there is any risk in that; a non-existent completion object is specifically defined in the design doc as being equal to a completion value of 0.0.

As it stands, the return value isn't actually used anywhere. I've included it so as to maintain a "create_or_update"-like interface.

As for whether isnew should be true or false, I opted for "True" to indicate that there was no value already present in the database at the time of call, but mostly just put it there because it needed some value to match the expected return type. I've almost never found a use for that feature on get_or_create anyway. If you have a particular use case for which one value or the other would be more applicable, I'd be happy to use that. I'd also be just as happy to only return the object, or not to return anything at all.

[robrap]

Adding as a review comment for ease of replying.

[robrap]

@jcdyer: I'm responding to your comment here.

Basically, I want to ensure that when the code is disabled: 1) we don't write any data, and 2) we don't break active functionality. I don't think there is a best way, so I just want to lay out my thoughts on the options, and you can decide if you want to stick with this implementation or pivot.

It seems like there are 3 options:

1. Raise an error.
2. Return a value that would make other code happy if it were using the return value (current implementation), or
3. Return a value like None that might break code looking for a proper tuple, but wouldn't cause an error otherwise.

If you raise an error here, it would ensure the code isn't doing anything we don't want it to do, but it "might" require extra code to avoid certain calls. This seems the safest, but it depends how much additional code/conditionals you need, because each additional bit of code adds more room for error.

The current implementation, where we return a legitimate value, seems the most deceptive because it may make a caller happy, but we don't know who that caller is and whether or not we wish that caller to be happy.

The third is to return a value like None. This is sort of a compromise that has some benefits and disadvantages of each of the earlier methods. It also means no one could accidentally do anything with the return valie.

Let me know what you think. Ignore my earlier comment about "isnew". My wish is that this value would be totally unimportant in this case, otherwise, we might have a bug.

[jcdyer]

I think I've resolved the hanging test suite. I just moved the context manager from setUpClass to setUp, and used the .addCleanup() handler instead of putting the __exit__() in tearDown().

[jcdyer]

@robrap, @bradenmacdonald This is ready for another round of review.

[robrap]

Thanks @jcdyer. Minor comments that you could do or skip as you see fit when you squash. Thanks.

[robrap]

Nit: => CompletionDisabledTestCase

[robrap]

Consider having a test for waffle disabled for these first two tests. It is somewhat low risk given that you are protected by raising the error at this point, so not a big deal. It just means if a bug skips in you may find it a little later.

[bradenmacdonald]

👍 Still good with me. Did a quick test of the functionality again on my devstack.

🚢 🤡

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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