Complete scored blocks #16186
Complete scored blocks #16186
Conversation
|
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. |
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
3 times, most recently
from
24979ef to
2b9e9c5
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
@jcdyer Whoops, I started reviewing this and thought I was reviewing "Create a completion emitter for default (non-scorable) blocks" since that WIP code is showing up in this PR. Could you please move the newer commits into a separate branch+PR?
I'll then give just the "complete scored" blocks part of this PR a final spin tomorrow.
There was a problem hiding this comment.
Don't forget to remove these console.logs :)
There was a problem hiding this comment.
Nit: inconsistent indentation here. Also, you can use trailing commas on the last element in JS arrays/objects, just like in python.
There was a problem hiding this comment.
Could we put completable into a constant, e.g. from lms.djangoapps.completion import CompletionMethod, then CompletionMethod.completable ?
There was a problem hiding this comment.
"the default method" is pretty vague - perhaps we should say "the default method (marking it as completed once it has been viewed)" or something ?
There was a problem hiding this comment.
This isn't an accurate description anymore - this is the set of descendent blocks that this vertical should mark as complete once the user views them. (i.e. it excludes completable blocks that are complete, and it excludes completable blocks that get marked complete via some other mechanism).
There was a problem hiding this comment.
Nit: Or six.text_type if you're changing this anyways?
There was a problem hiding this comment.
How about calling this watch_completables or something to distinguish it from a list of children which are completable? See past comment about this list not being simply a list of completable descendants.
There was a problem hiding this comment.
Should we also check that this block is in the set returned by get_completables_for_user ? Or do we not care if spurious completion events or premature ones are emitted (e.g. user calling this directly to mark a gradable item as completed, instead of completing the graded activity).
There was a problem hiding this comment.
I think it doesn't matter if it's in the set of get_completables_for_user. The main purpose of the specific user check is to reduce network traffic, and at this point, the request is already in the VPC (and will not even hit the DB if it doesn't change the value).
There was a problem hiding this comment.
Hmm. But it might be worth the looser check of whether it uses the default completion method. I'll think about that.
There was a problem hiding this comment.
Nit: I'm not a fan of this string "True" comparison. How about putting the data-completable attribute (with no value) on the element if and only if it should be marked as completed on view (or perhaps call it data-complete-on-view?), and then change this check here to use hasAttribute?
i.e. https://stackoverflow.com/questions/16864999/can-i-use-html5-data-attributes-as-boolean-attributes
There was a problem hiding this comment.
Sounds good. I don't love "stringly" typed data either. I'll do it that way.
I suppose another option would be to make the values "JSON like" (so data-bool='true' data-null='null' data-num='1' data-string='"string"' data-object='{"tweedle": "dee"}'), but you'd need to make that a site-wide policy to be worth doing anyway. (just musing out loud).
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
4 times, most recently
from
7c2efd9 to
6eb6b0d
Compare
|
Wow. I got my git branches in a muddled state. I think everything's back in shape now. |
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
from
6eb6b0d to
c10d304
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
@jcdyer I think you have an unnecessary test, but otherwise this looks great and works well :)
- I tested this: Followed testing instructions with a
problemblock - it nicely created a BlockCompletion entry (with completion=1), then when I deleted the student state completion went to 0. Re-answering set the completion back to 1. - I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
One potential issue though: I think we should we check for has_custom_completion and then not update the completion in that case. Right? Because the idea is that some XBlocks may wish to control their grade and their completion status separately. An example could be a block like ORA2 that wishes to emit an interim grade after the user has completed part of the requirements but not be considered complete until the user has finished everything.
There was a problem hiding this comment.
This test doesn't seem to be doing what it says (test_handler_resets_completion_on_score_delete) and it seems redundant given the ddt variants of the test_handler_submits_completion test above. Is this just a remnant from before a ddt refactor?
There was a problem hiding this comment.
Yeah. The DDT was a late addition, and I think I did it half way through copy-pasting this test (before revising it to match the name.
|
If you want to spin the |
|
@bradenmacdonald You're absolutely right about checking for Ready for another review. |
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
@jcdyer: Code looks good. Thanks.
A couple of questions:
- In the future, would it be possible to base the branch and PR off the other WIP branch before it merges, so the PR just has the changes meant to be reviewed? Are there issues with this?
- I see we are leaving the check for
has_custom_completionuntil later. We'll also need to checkcompletion_method=EXCLUDEDright? I may be missing something obvious, but is there anything blocking data from being written once this is merged? What is the plan for merging or delaying merges such that functionality is more complete before writing data (for example, including default completion for viewable blocks)?
That would be really nice, yep - especially since GitHub allows changing the target branch before merging a PR, so it can change from the previous PR to master before merging. However, I don't think @jcdyer has push access to this repo, and it would be too inefficient for you or I to keep pushing his changes from the |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
👍
- I tested this: Followed testing instructions with a
problemblock - it nicely created a BlockCompletion entry (with completion=1), then when I deleted the student state completion went to 0. Re-answering set the completion back to 1. - I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
2 times, most recently
from
90b3756 to
f0d536e
Compare
|
@jcdyer @bradenmacdonald: Regarding my earlier comment, I submitted a ticket to get @jcdyer push access. I'll let you know when I hear back. Point 2 is still open and waiting for a response. |
|
Good point, @robrap. Nothing is going to stop the block completions from being written. I was planning on allowing the BlockCompletions to be written, and then using COMPLETION_START_DATE to prevent using early BlockCompletions from affecting AggregateCompletions (once those are implemented, but it probably makes sense to have basic completion rendering in place, and relatively complete, so the aggregate code doesn't have to be quite as defensive. I'll introduce a waffle flag in #16112. |
|
Thanks for looking into getting me push access. Additionally, I'm hoping once these first two commits land (model and event handler), I won't need to stack PRs as much. |
|
@jcdyer: I still haven't thought this through, but I imagine it would be simpler if for any user, the timestamp of any completion data is the point at which that user has full completion data (e.g. default visibility completion, etc.). I guess we could truncate the table if and when we really wanted to achieve the same, but if the waffle flag were a part of this PR, we wouldn't get any data until this were turned on. Also, maybe this is a non-issue, but do we have any known scored blocks that will not track completion once the custom completion and exemption code is done? In other words, will we be writing bad data in certain cases with this PR? Lastly, it sounds like push access will be coming soon, but not certain until you have it. :) |
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
2 times, most recently
from
a507dc5 to
2f55112
Compare
|
jenkins run lettuce |
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
from
2f55112 to
8412e9f
Compare
|
@jcdyer: You now have push access. A couple of thoughts:
|
Of course.
This one depends on the changes in cliff/handle-block-completion (#16112). It was getting time consuming to keep rebasing this and #16234 whenever I added a commit to that PR, so it may be a little out of data ATM. Once that gets merged, I'll update this to match it, ensure all outstanding review issues are addressed, and ask for another review. |
|
Great. I'll wait for your prompt on this one. Thanks. |
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
3 times, most recently
from
16e51d1 to
12b6e66
Compare
* Add a handler to mark a block complete when a problem is scored.
* Also handle marking incomplete when user problem state is deleted.
* Add score_deleted to published providing_args for PROBLEM_{RAW,WEIGHTED}_SCORE_CHANGED
OC-3089
jcdyer
force-pushed
the
cliff/complete-scored-blocks
branch
from
12b6e66 to
69271d0
Compare
|
@robrap @bradenmacdonald This has been rebased against the master branch. Can I get a fresh review on it? I had to expand the waffle flag handling a bit to prevent breaking grading, and I accidentally did it during the rebase cleanup, so it's all still one commit. |
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
I wasn't too careful this pass. Just reviewing waffle. If you have any concerns, please point the way. Thanks!
|
@jcdyer: I can't find your earlier comment from one of these PR's with a "Xsoon" joke, but I appreciated it. |
|
@robrap Glad to hear it. I don't have any particular concerns on the latest implementation. I'll get this squashed and merged. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, October 30, 2017. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
5 participants
This contains the completion logic for scorable xblocks. When a block is scored, it should be marked complete (completion = 1.0). If an instructor resets the state of the xblock, it should also reset the completion of the xblock to 0.0.
JIRA tickets: N/A
Discussions: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up.
Dependencies: None
Screenshots: N/A
Sandbox URL: TBD
Merge deadline: None
Testing instructions:
You will be going back and forth between the django shell and interacting with the devstack through the browser, so log into the devstack twice, and run
paver devstack lmsin one of the sessions. Then:./manage.py lms --settings=devstack shellin the other window), ensure that there is no completion object for that block, or if it does exist, ensure the completion value is set to zero.completion_object.refresh_from_db(). Otherwisecompletion_object = BlockCompletion.objects.get(pk=completion_object.pk)will work. Ensure the completion value is set to 1.0.Reviewers:
Author notes and concerns: N/A
Settings: N/A