Refactored Step Builder Messages

[bradenmacdonald]

This refactors step builder to:

• Make the Review step code simpler
• Split the parts of the review screen up into XBlocks that are more flexible and re-orderable.
• Make the Step Builder block preview/work in Studio as students would see it in the LMS

Example: Editing a review step:

Example: A Conditional Message:

[smarnach]

Isn't it the implicit behaviour to fall back to the student view? (No need to change anything, it's fine to keep this explicit in any case. I'm just curious.)

[bradenmacdonald]

Yes, but if it renders student_view as a fallback, Studio will wrap the XBlock in a bunch of HTML (header, edit controls) that we don't want. Studio looks at the name of the view that's being rendered when deciding to wrap the element or not.

[smarnach]

@bradenmacdonald Nice, this makes the code indeed more readable! I've done a first past through the code, but haven't done any testing yet. To facilitate getting this merged until tomorrow morning, I'll do the testing after the meeting, so if anything comes up you will still have a bit of time to address any issues.

How much time will you need for the tests? If you can finish them soon, I might be able to give a thumbs up today. (I'll be available until about 22:00 UTC.)

[bradenmacdonald]

@smarnach Thanks for the initial review! Yes I'm happy with how this has improved the code :-)

I'll work on addressing your comments and updating the tests right after the meeting, but I'm not sure how long it will take. A couple hours perhaps?

[smarnach]

@bradenmacdonald OK, we'll see then. Either I'm still available to take a final look, or it will have to wait until tomorrow.

[bradenmacdonald]

@smarnach I've addressed your initial review comments and fixed a small bug. Now I'm just continuing to get the tests passing again.

[smarnach]

When adding a new Per-Question Feedback block and cliking "Edit" on it, I get this error:

(This is on a relatively current version of the solutions devstack.)

[bradenmacdonald]

@smarnach Good catch. Fix just pushed: ee49a27

(I also just pushed two other fixes and will "soon" push a commit that makes the tests pass, hopefully)

[smarnach]

Does the new version have to be backwards compatible to existing course data? When loading my pre-existing Step Builder example in Studio, one of the messages was displayed with the heading "INVALID MESSAGE". I also still had the old "Message shown when complete/incomplete" blocks, and things didn't properly work when tested in the LMS. I'll try again with a newly created version.

[bradenmacdonald]

@smarnach I'm aware of that, but I think it's fine. I don't believe this is yet in use in any courses. The important thing is that the authors are able to do the conversion.

[smarnach]

If you add multilple conditional messages and view them from the Step Builder top level in studio, it's hard to see which condition belongs to which message, since the condition is separate from the message by a horizontal rule, while the messages aren't separated from each other at all:

[smarnach]

I reviewed everything that's there up to now. The code is looking good, and everything is working fine with the latest changes as well. So the only thing left is to make the tests pass. :)

I'll take a final look tomorrow morning, and will merge and do the hash update on the Solutions fork of edx-platform if everything looks good.

[bradenmacdonald]

Thanks a lot, @smarnach :)

[smarnach]

All looking good, 👍.