-
Notifications
You must be signed in to change notification settings - Fork 25
Shared Mixins to enable effortless Studio-editable XBlocks (OC-513) #5
Shared Mixins to enable effortless Studio-editable XBlocks (OC-513) #5
Conversation
@sarina @antoviaque FYI, although this repo does have tests, all the new code in this PR depends heavily on interactions with Studio. So in the long term, I'd like to move it to edx-platform which is the only environment where we could actually test it effectively. It's here for now so that we can use the new mentoring XBlock on the Birch release (not sure how important that is), and because I assume we do not have time for the extra work and review steps required for putting it into platform. |
@bradenmacdonald Hmm that's an interesting question. In the interest of getting it in, having it here makes sense, and then migrating to edx-platform later. My worry is if in the move to platform changes are requested that aren't backwards compatible with the xblock requiring a lot of stupid extra work on your part. @antoviaque let me know if I should make an ospr ticket for this pr, or what |
8406f56
to
79a0e38
Compare
@Kelketek Now that I've added a bunch of integration tests, this is ready for your review. I'd like to get it merged ASAP. n.b. To test in Studio (obviously important for this PR), you'll need to check out open-craft/xblock-mentoring#6 and create some of those mentoring blocks in Studio. That mentoring PR is not quite ready for review but the parts that you need to test studio editing are done. CC @antoviaque |
@sarina Created an OSPR ticket for the mentoring review, including the current PR: https://openedx.atlassian.net/browse/OSPR-422 @bradenmacdonald Cool - note that since this will also be part of the upstream review, it could make sense to leave the PR unmerged, and simply point the requirement in mentoring to the latest commit? This way if changes are needed they can also be included here. |
@bradenmacdonald @sarina FYI, the xblock-utils repo is now on the edx org (note that this changed the URL for this PR). |
dataType: "json", | ||
global: false, // Disable Studio's error handling that conflicts with studio's notify('save') and notify('cancel') :-/ | ||
success: function(response) { runtime.notify('save', {state: 'end'}); } | ||
}).fail(function(jqXHR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very interested to know if someone from upstream can comment on this-- The limitations here cause some pretty painful code duplication.
1b7425b
to
d02bbf8
Compare
Thanks for the pull request, @bradenmacdonald! I've created OSPR-422 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. We can't start reviewing your pull request until you've added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information. |
return self.browser.execute_script('return window.notifications.shift();') | ||
|
||
def wait_for_runtime_notification(self): | ||
wait = WebDriverWait(self.browser, self.timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradenmacdonald can the wait/synchronization be done through bok-choy? (instead of interacting with selenium directly)
http://bok-choy.readthedocs.org/en/latest/api_reference.html#bok_choy.page_object.PageObject.wait_for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgoldberg Yes, I could re-write StudioEditableBaseTest
as a bok_choy PageObject
. Though I think for this particular method that you commented on, there is no way to execute JavaScript using pure bok choy APIs (AFAIK?) so some Selenium must be used directly anyways. I don't think I'll be able to get to that change this week though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradenmacdonald a bok-choy PageObject has a reference to the underlying selenium webdriver instance (self.browser). So you can execute script through that without the need to import selenium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgoldberg personally I wouldn't consider this to be a blocker to merge, though - these tests won't be running in edx-platform main repo.
e2cc91e
to
1b8f21a
Compare
@bradenmacdonald The inclusion of static files seem to be missing in setup.py - if this isn't installed in development mode the static files are missing from the package. |
@bradenmacdonald @antoviaque : in talking with Leslie, she flagged the potential similarities between this work and https://github.com/edx/edx-platform/pull/6028 - can you take a look over that pull request and let me know how it might be similar/different from this work? Christina has spent a lot of time thinking about the Studio XBlock editing interface, and we want to make sure we're not having a bunch of competing approaches in this arena. If you have questions for Christina, please ping her on that PR. |
@antoviaque Missing static files should be fixed now. @sarina We had touched base before the recent hackathon but thanks for the reminder. I've pinger her on that PR. |
…ider returns an empty list
efb0ce3
to
071565b
Compare
071565b
to
7ec76da
Compare
if info["type"] in ("list", "set"): | ||
info["value"] = [json.dumps(val) for val in info["value"]] | ||
if info["default"] is None: | ||
info["default"] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff-cover reports this line - and a few others- are untested.
xblockutils.studio_editable 80%
137, 141, 149, 158-162, 201, 255-257, 272-286, 298-304, 311-313, 320
I don't think 100% coverage is required, just want to make sure you've looked over the list of untested lines and made a judgement for each instance as to whether or not it's worth adding an additional test.
@bradenmacdonald @antoviaque I think this is looking great and we're about ready to merge it (Dave O looked over it last week and is OK with the approach architecturally and didn't have much to add). Before merging, I'd like responses to:
|
|
I have improved the test coverage. Caught a bug while I was at it, so that was good. The remaining uncovered lines require the Studio environment, so we aren't able to write integration tests for them here. |
@bradenmacdonald ok, that makes sense. Can you check out the test failures? Once they're passing this is good to merge. |
194b002
to
ca86d73
Compare
@sarina Ah, I didn't notice that. Was a simple issue and easy to fix. Merging now. Thanks! |
Shared Mixins to enable effortless Studio-editable XBlocks (OC-513)
This PR contains two very useful mixin classes available for use in XBlocks:
StudioEditableXBlockMixin
This mixin will automatically generate a working
studio_view
form that allows content authors to edit the fields of your XBlock. To use, simply add the class to your base class list, and add a new class field callededitable_fields
, set to a tuple of the names of the fields you want your user to be able to edit.That's all you need to do. The mixin will read the optional
display_name
,help
,default
, andvalues
settings from the fields you mention and build the editor form as well as an AJAX save handler.If you want to validate the data, you can override
validate_field_data(self, validation, data)
and/orclean_studio_edits(self, data)
- see the source code for details.Supported field types:
field_name = Boolean(display_name="Field Name")
field_name = Float(display_name="Field Name")
field_name = Integer(display_name="Field Name")
field_name = String(display_name="Field Name")
field_name = String(multiline_editor=True, resettable_editor=False)
field_name = String(multiline_editor='html', resettable_editor=False)
field_name = List(list_style='set', list_values_provider=some_method, default=[])
List
, the editor UI will be a JSON textarea.List
declaration must include the propertylist_style='set'
to indicate this.List
declaration must also define alist_values_provider
method which will be called with the block as its only parameter and which must return a list of possible values.list_field = List(display_name="Normal List", default=[])
dict_field = Dict(display_name="Normal Dict", default={})
Supported field options (all field types):
values
can define a list of possible options, changing the UI element to a select box. Values can be set to any of the formats defined in the XBlock source code:[1, 2, 3]
[ {"display_name": "Always", "value": "always"}, {"display_name": "Past Due", "value": "past_due"}, ]
{"min": 0 , "max": 10, "step": .1}
resettable_editor
- defaults toTrue
. SetFalse
to hide the "Reset" button used to return a field to its default value by removing the field's value from the XBlock instance.Changes compared to the existing XModule editing support in Studio:
editable_fields
) rather than blacklist (non_editable_metadata_fields
)""
and a field using the default value of""
. Studio's XModule field editor cannot.List
fields that are really sets (unordered, unique items)Basic screenshot:
![Basic screenshot](https://cloud.githubusercontent.com/assets/945577/6341782/7d237966-bb83-11e4-9344-faa647056999.png)
Screenshot of the list/set editor:
![Screenshot of the list or set editor](https://cloud.githubusercontent.com/assets/945577/6404805/f03c91b8-bdd0-11e4-8310-5b7090fa8a6a.png)
StudioContainerXBlockMixin
This mixin helps with creating XBlocks that want to allow content authors to add/remove/reorder child blocks. By removing any existing
author_view
and adding this mixin, you'll get editable, re-orderable, deletable child support in Studio. To enable authors to add new children, simply overrideauthor_edit_view
and setcan_add=True
when callingrender_children
- see the source code. To enable authors to add only a limited subset of children requires custom HTML.An example is the mentoring XBlock:
![screen shot 2015-02-23 at 5 44 05 pm](https://cloud.githubusercontent.com/assets/945577/6341803/d0195ec4-bb83-11e4-82f6-8052c9f70690.png)
Other changes
child_isinstance()
helper method, which uses only the public XBlock API to test a block's class given a usage_id, without the expensive operation of instantiating the block.SeleniumBaseTest
by removing the initial step of loading the Workbench home pageSeleniumXBlockTest
. It is faster and more flexible thanSeleniumBaseTest
because it doesn't need to load a whole folder of XML scenarios.SeleniumBaseTest
is kept to preserve compatibility for XBlock tests that currently use it.