Step Builder: Allow re-adding children after removing them and (un)lock unit navigation when appropriate

[itsjeyd]

This PR fixes the following issues:

1. When removing a Review Step child from a Step Builder block, the button for adding it stays greyed out, making it impossible to remove and then re-add a Review Step block (without reloading the page). The same issue also affects buttons for adding different types of message children to Review Step block.
2. In supporting runtimes (i.e., Apros), arrow buttons for navigating between units should be disabled while completing Step Builder blocks; they should become available again when user reaches review step.

Testing

For the first issue:

1. Add a Step Builder block to a unit.
2. Add a Review Step block to Step Builder. (Observe that the corresponding button is greyed out.)
3. Remove the Review Step block. Observe that the corresponding button is re-enabled.
4. Click the button to make sure that it is possible to re-add a Review Step to the Step Builder block.
5. Repeat steps 2-5 for the different message types that can be added to Review Step blocks.

For the second issue:

1. Add a unit to a subsection with some existing units.
2. Add a Step Builder block to the unit.
3. Add a couple of Mentoring Step blocks and a Review Step block.
4. Add a question to each Mentoring Step.
5. Complete the block in Apros. Observe that while you are looking at individual steps, the arrow buttons to the left and right of the question(s) are disabled. When you get to the review step, the arrows become clickable.

[mtyaka]

@itsjeyd 👍 The code looks good and I verified it works. It would be nice if we could add some tests for these issues, although I'm not sure how easy that is. I don't think we currently have any way to test the Studio sections of the code. Would it be possible to add a test for navigation events?

[itsjeyd]

@mtyaka Thanks for reviewing!

Would it be possible to add a test for navigation events?

I'm not sure. Is there a way to inject a fake notify method into the workbench runtime for the purpose of testing? I tried this in one of the integration tests for Step Builder:

with patch.object(WorkbenchRuntime, 'notify', create=True) as patched_method:
    step_builder, controls = self.load_assessment_scenario("step_builder.xml", params)
    self.assertTrue(patched_method.called)

This works to the extent that the notify method is accessible from the Python portion of the code: If I insert a call to notify into StepBuilder.student_view, the test will pass. But the method doesn't seem to be accessible from the JS portion of the code. If I reduce this:

    function notify(name, data) {
        // Notification interface does not exist in the workbench.
        if (runtime.notify) {
            runtime.notify(name, data);
        }
    }

to this:

    function notify(name, data) {
        runtime.notify(name, data);
    }

to make sure that the JS code tries to call notify, I get this:

TypeError: runtime.notify is not a function

in the Browser console.

[mtyaka]

@itsjeyd I never tried this, but it might be possible to patch patch the RuntimeProvider object from https://github.com/edx/xblock-sdk/blob/master/workbench/static/workbench/js/runtime/1.js to return a runtime with a stubbed notify method. You could use selenium's driver.execute_script to install the patched RuntimeProvider before running the test.

To be honest, it's probably not worth doing that on this PR. Ideally the ability to patch the custom JS runtime would be provided by the base test classes from xblock-utils.

[itsjeyd]

@mtyaka OK, I'm going to go ahead and merge this PR then.

Ideally the ability to patch the custom JS runtime would be provided by the base test classes from xblock-utils.

What would be a good next step here? Should we create a separate ticket for this in the backlog? There is a ticket for extending test coverage for Step Builder; out of the existing tickets that would probably be the best place to put a note about this. However, it might not be a good idea to further increase the scope of that ticket without doing some discovery first.

[mtyaka]

@itsjeyd I would add it to the existing ticket, I wouldn't worry about extending the scope while the ticket is still in the backlog.

[itsjeyd]

@mtyaka OK thanks, done.