fix: the get_completion JS handler should not ignore FBE blocks #27422

mikix · 2021-04-26T18:44:22Z

Commit message:

Because xblock handlers normally get a block tree that already has inaccessible blocks stripped, they don't see FBE gated blocks at all. So the get_completion handler would return True for FBE units incorrectly. Leading to a visual bug as an audit user went through their units in the courseware.

In order to let the handler know about the full tree, I've added a new attribute you can set on your xblock handlers:
my_handler.will_recheck_access = True

This will tell the top-level handler code to get the full tree for you.

As part of this, I've also changed the sequence xblock handlers into proper xblock handlers (not old-style xmodule handlers). This changes their URLs slightly. I've kept the old URLs for now as well, but they can be removed after Maple.

https://openedx.atlassian.net/browse/AA-409

Full set of changes with explanations:

I've added support for xblock handlers to define will_recheck_access. The toplevel handler code in module_render.py will now peek at the xblock to see if it should strip inaccessible blocks or not.
Rather than pipe that new attribute through the xmodule handler shim code, I've converted the sequence handlers to new-style xblock handlers. I've also left support for the old handlers in there, which we can remove after Maple.
I've removed the metadata handler altogether. This was added for the learning MFE's benefit, but then never properly used because that team later added a new API request that then internally called the handler. I think they needed different module lookup flags, much like I did (they seem to both disable debug info and also set will_recheck_access). The handler is not actually called externally from the learning MFE.
To ease piecemail conversion of old xmodule code to modern code, I've also passed the JS runtime down to old style module JS (see change to XBlockToXModuleShim in xmodule.js). This lets old JS easily get the proper handler URL the same way that modern JS does.
The actual bug fix of get_completion.will_recheck_access = True.

This will also need changes to the learning MFE to call the new locations of these handers.

mikix · 2021-04-26T18:47:29Z

lms/djangoapps/courseware/module_render.py

The surrounding little refactor was all to just expose _get_usage_key_for_course and _get_descriptor_by_usage_key so that we could call them outside the context of get_module_by_usage_id.

MatthewPiatetsky

I know we use the will_recheck_access argument in other places, so this is fine, though otherwise I wonder if you could have an argument that more clearly communicates the purpose like strip_inacessible_blocks_from_tree or something
were you able to test that the bug was fixed locally for a FBE example?
do you think it would be worthwhile to add a test with some FBE problems that confirms the completion is calculated correctly?

Dillon-Dumesnil

From what I could understand, yeah, makes sense. Thanks for the PR comment

lms/djangoapps/courseware/module_render.py

Dillon-Dumesnil · 2021-04-26T20:11:23Z

common/lib/xmodule/xmodule/js/src/sequence/display.js

            this.num_contents = this.contents.length;
            this.id = this.el.data('id');
-            this.ajaxUrl = this.el.data('ajax-url');
+            this.getCompletionUrl = runtime.handlerUrl(element, 'get_completion');


I think this is a part I don't have a great understanding of. The runtime already has a method defined to handle this? I don't see any other changes related.

I don't fully understand this concept of a runtime either. It's a versioned JS specific runtime that only seems to have this method?

But it's how the rest of the modern xblock JS code gets handler URLs.

https://github.com/edx/edx-platform/blob/master/lms/static/lms/js/xblock/lms.runtime.v1.js#L32

mikix · 2021-04-26T20:26:25Z

I know we use the will_recheck_access argument in other places, so this is fine, though otherwise I wonder if you could have an argument that more clearly communicates the purpose like strip_inacessible_blocks_from_tree or something

Well that's how I'm using it, but some future handler might actually want to check access itself. I wanted to keep as close to the existing usage of the phrase as possible (show up in searches for that flag, etc).

were you able to test that the bug was fixed locally for a FBE example?

Yeah it seems to work. You know I ain't touching xblock code without testing it. :P

do you think it would be worthwhile to add a test with some FBE problems that confirms the completion is calculated correctly?

Yeah I hadn't gotten to the test part of this PR, honestly. Either fixing or adding. Probably makes sense to add some sort of test somewhere for this new flag / handler behavior.

Because xblock handlers normally get a block tree that already has inaccessible blocks stripped, they don't see FBE gated blocks at all. So the get_completion handler would return True for FBE units incorrectly. Leading to a visual bug as an audit user went through their units in the courseware. In order to let the handler know about the full tree, I've added a new attribute you can set on your xblock handlers: my_handler.will_recheck_access = True This will tell the top-level handler code get the full tree for you. As part of this, I've also changed the sequence xblock handler's into proper xblock handlers (not old-style xmodule handlers). This changes their URLs slightly. I've kept the old URLs for now as well, but they'll be removed after Maple. AA-409

edx-status-bot · 2021-04-27T20:02:08Z

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

mikix · 2021-04-27T20:06:00Z

Heads up that I fixed tests and added some new ones. No actual "real code" changes so a re-review is not necessary, but if you're bored you can look over the tests.

ormsbee · 2021-05-03T17:56:52Z

@mikix: Thanks for pinging @kdmccormick and myself on this. I'm good with the architectural issues. In the longer term, I definitely have concerns about the level at which FBE is implemented, and the mixing of concerns that is happening by introducing markup HTML at the access permissions layer. But as we discussed earlier, I agree that implicitly passing it along as a parameter of the handler via the .recheck_access attribute is the least invasive/risky way of getting the behavior you need, and that a larger refactoring of the code is not warranted at this time.

Some other high level questions/observations:

Do we have to worry about tracking event data being passed along properly with this change to the sequence AJAX handler (FYI @doctoryes)?
FYI to @symbolist, in case there is any impact on upcoming modulestore cleanup work or LabXchange's use of the runtime.

mikix · 2021-05-03T20:36:41Z

Do we have to worry about tracking event data being passed along properly with this change to the sequence AJAX handler?

I'm not sure I understand the question. Xblock handlers do some default event tracking? (I didn't notice it, but I believe it) And the worry would be that we have some number crunching somewhere that would be interrupted by this handler URL change?

ormsbee · 2021-05-04T13:53:58Z

I'm not sure I understand the question. Xblock handlers do some default event tracking? (I didn't notice it, but I believe it) And the worry would be that we have some number crunching somewhere that would be interrupted by this handler URL change?

Some of the events are explicitly emitted, others are derived from server tracking logs. But I think we're safe here because the ones around sequence navigation look like they're explicitly emitted at the JS layer to the tracking endpoint: https://edx.readthedocs.io/projects/devdata/en/stable/internal_data_formats/tracking_logs.html#seq-goto-seq-next-and-seq-prev

@doctoryes: Does that sound right to you?

doctoryes · 2021-05-04T18:43:54Z

@ormsbee Correct - the sequence events are emitted explicitly. For example, seq_goto is emitted by JS code to the LMS here:

https://github.com/edx/edx-platform/blob/5f569a424e0b8c2c915997393463b0133cd08f86/common/lib/xmodule/xmodule/js/src/sequence/display.js#L314-L321

and then transformed into seq_goto in the tracking logs here:

https://github.com/edx/edx-platform/blob/5f569a424e0b8c2c915997393463b0133cd08f86/common/djangoapps/track/transformers.py#L274-L286

That flow isn't being changed in this PR.

And these events can be seen in Snowflake using queries like this one:

SELECT * FROM PROD.TRACKING_EVENTS.TRACKING_EVENTS_WITH_METADATA
WHERE event_name = 'seq_goto' 
limit 100;

You could use Snowflake queries post-deploy to ensure that the proper events are still being emitted.

mikix · 2021-05-04T19:01:59Z

Thank you for looking, Julia! OK, I'll merge this in 30min or so if no one has any objections

edx-pipeline-bot · 2021-05-04T21:10:53Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-05-04T21:36:09Z

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

symbolist · 2021-05-13T21:24:25Z

common/lib/xmodule/xmodule/js/src/xmodule.js


        try {
-            module = new window[moduleType](element);
+            module = new window[moduleType](element, runtime);


@mikix @MatthewPiatetsky This seems to have caused a regression for the Conditional Block. After cherry-picking it, the conditional_get handler is not being called causing the ConditionalBlock to display empty. This is because calledElId here gets set to runtime and then the render() on line 17 is not called.

See https://openedx.atlassian.net/browse/TNL-8318 for details.

Ah geeze sorry - let me work on a fix.

PR for fix: https://github.com/edx/edx-platform/pull/27625 (needs some more local testing, but I think that's the idea)

Thanks! 🙂

mikix commented Apr 26, 2021

View reviewed changes

mikix force-pushed the mikix/handler-will-recheck-access branch from 63f4495 to f8c2a5b Compare April 26, 2021 19:06

mikix mentioned this pull request Apr 26, 2021

fix: use new xblock handler URLs for sequence blocks openedx/frontend-app-learning#423

Merged

MatthewPiatetsky reviewed Apr 26, 2021

View reviewed changes

Dillon-Dumesnil approved these changes Apr 26, 2021

View reviewed changes

MatthewPiatetsky approved these changes Apr 26, 2021

View reviewed changes

mikix force-pushed the mikix/handler-will-recheck-access branch from f8c2a5b to f89e988 Compare April 27, 2021 16:19

mikix marked this pull request as ready for review April 27, 2021 16:20

mikix force-pushed the mikix/handler-will-recheck-access branch from f89e988 to b90039b Compare April 27, 2021 19:17

mikix merged commit 78c6be8 into master May 4, 2021

mikix deleted the mikix/handler-will-recheck-access branch May 4, 2021 19:54

symbolist reviewed May 13, 2021

View reviewed changes

fix: the get_completion JS handler should not ignore FBE blocks #27422

fix: the get_completion JS handler should not ignore FBE blocks #27422

Uh oh!

Conversation

mikix commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikix Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

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

Dillon-Dumesnil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dillon-Dumesnil Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

mikix Apr 26, 2021 • 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!

mikix commented Apr 26, 2021

Uh oh!

edx-status-bot commented Apr 27, 2021

Uh oh!

mikix commented Apr 27, 2021

Uh oh!

ormsbee commented May 3, 2021

Uh oh!

mikix commented May 3, 2021

Uh oh!

ormsbee commented May 4, 2021

Uh oh!

doctoryes commented May 4, 2021

Uh oh!

mikix commented May 4, 2021

Uh oh!

edx-pipeline-bot commented May 4, 2021

Uh oh!

edx-pipeline-bot commented May 4, 2021

Uh oh!

symbolist May 13, 2021

Choose a reason for hiding this comment

Uh oh!

mikix May 14, 2021

Choose a reason for hiding this comment

Uh oh!

mikix May 14, 2021

Choose a reason for hiding this comment

Uh oh!

symbolist May 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mikix commented Apr 26, 2021 •

edited

Loading

MatthewPiatetsky left a comment •

edited

Loading

mikix Apr 26, 2021 •

edited

Loading