-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add "Edit" button to each leaf xblock on the container page #2981
Changes from all commits
7770d9d
4f85228
be8e994
df32f10
2425aab
636fa19
ee42902
052949d
e18dd7f
dce075a
415fe8d
07c54dc
b23f17d
230c0b5
5ab159b
b779b8a
88d95d1
a6aae72
9de6f10
2ae28c5
d55ad61
a3e26e6
b78508a
2bf64d7
df0973e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
from terrain.steps import reload_the_page | ||
from common import type_in_codemirror | ||
from selenium.webdriver.common.keys import Keys | ||
from cms.envs.common import FEATURES | ||
|
||
|
||
@world.absorb | ||
|
@@ -105,9 +106,16 @@ def click_component_from_menu(category, component_type, is_advanced): | |
|
||
@world.absorb | ||
def edit_component_and_select_settings(): | ||
world.wait_for(lambda _driver: world.css_visible('a.edit-button')) | ||
world.css_click('a.edit-button') | ||
world.css_click('#settings-mode a') | ||
world.edit_component() | ||
world.ensure_settings_visible() | ||
|
||
|
||
@world.absorb | ||
def ensure_settings_visible(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not fail if settings are not visible. Is it ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. There are cases where the settings button isn't shown when the only possible editor is the settings editor, so I needed to make the code not fail if the button didn't exist. I should make sure in the else clause that it is the settings editor being shown. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is good idea to make it clearer. |
||
# Select the 'settings' tab if there is one (it isn't displayed if it is the only option) | ||
settings_button = world.browser.find_by_css('.settings-button') | ||
if len(settings_button) > 0: | ||
world.css_click('.settings-button') | ||
|
||
|
||
@world.absorb | ||
|
@@ -116,14 +124,25 @@ def edit_component(): | |
world.css_click('a.edit-button') | ||
|
||
|
||
@world.absorb | ||
def select_editor_tab(tab_name): | ||
editor_tabs = world.browser.find_by_css('.editor-tabs a') | ||
expected_tab_text = tab_name.strip().upper() | ||
matching_tabs = [tab for tab in editor_tabs if tab.text.upper() == expected_tab_text] | ||
assert len(matching_tabs) == 1 | ||
tab = matching_tabs[0] | ||
tab.click() | ||
world.wait_for_ajax_complete() | ||
|
||
|
||
def enter_xml_in_advanced_problem(step, text): | ||
""" | ||
Edits an advanced problem (assumes only on page), | ||
types the provided XML, and saves the component. | ||
""" | ||
world.edit_component() | ||
type_in_codemirror(0, text) | ||
world.save_component(step) | ||
world.save_component() | ||
|
||
|
||
@world.absorb | ||
|
@@ -173,14 +192,14 @@ def verify_all_setting_entries(expected_entries): | |
|
||
|
||
@world.absorb | ||
def save_component(step): | ||
world.css_click("a.save-button") | ||
def save_component(): | ||
world.css_click("a.action-save") | ||
world.wait_for_ajax_complete() | ||
|
||
|
||
@world.absorb | ||
def save_component_and_reopen(step): | ||
save_component(step) | ||
save_component() | ||
# We have a known issue that modifications are still shown within the edit window after cancel (though) | ||
# they are not persisted. Refresh the browser to make sure the changes WERE persisted after Save. | ||
reload_the_page(step) | ||
|
@@ -189,7 +208,7 @@ def save_component_and_reopen(step): | |
|
||
@world.absorb | ||
def cancel_component(step): | ||
world.css_click("a.cancel-button") | ||
world.css_click("a.action-cancel") | ||
# We have a known issue that modifications are still shown within the edit window after cancel (though) | ||
# they are not persisted. Refresh the browser to make sure the changes were not persisted. | ||
reload_the_page(step) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,24 +209,21 @@ def check_text_in_the_captions(_step, text): | |
|
||
@step('I see value "([^"]*)" in the field "([^"]*)"$') | ||
def check_transcripts_field(_step, values, field_name): | ||
world.click_link_by_text('Advanced') | ||
world.select_editor_tab('Advanced') | ||
field_id = '#' + world.browser.find_by_xpath('//label[text()="%s"]' % field_name.strip())[0]['for'] | ||
values_list = [i.strip() == world.css_value(field_id) for i in values.split('|')] | ||
assert any(values_list) | ||
world.click_link_by_text('Basic') | ||
world.select_editor_tab('Basic') | ||
|
||
|
||
@step('I save changes$') | ||
def save_changes(_step): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Call save_component method above (with identical code). |
||
save_css = 'a.save-button' | ||
world.css_click(save_css) | ||
world.wait_for_ajax_complete() | ||
world.save_component() | ||
|
||
|
||
@step('I open tab "([^"]*)"$') | ||
def open_tab(_step, tab_name): | ||
world.click_link_by_text(tab_name.strip()) | ||
world.wait_for_ajax_complete() | ||
world.select_editor_tab(tab_name) | ||
|
||
|
||
@step('I set value "([^"]*)" to the field "([^"]*)"$') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,14 +317,17 @@ def container_handler(request, tag=None, package_id=None, branch=None, version_g | |
while parent and parent.category != 'sequential': | ||
ancestor_xblocks.append(parent) | ||
parent = get_parent_xblock(parent) | ||
|
||
ancestor_xblocks.reverse() | ||
|
||
unit = ancestor_xblocks[0] if ancestor_xblocks else None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we may have talked about this before, but I forget.... Should this code throw an error if there is no unit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intention is that the container page can also work for units themselves, so that we can deprecate the old unit page. In that scenario, unit will be null. |
||
unit_publish_state = compute_publish_state(unit) if unit else None | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a stylistic comment: for code readability, you may want to put the exceptional case (None) at the end, and the normal case in the beginning. This way, we can easily see what we are assigning to in the non-exceptional case. So it would be: unit = ancestor_xblocks[0] if ancestor_xblocks else None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that's a good suggestion. I cut-and-pasted this logic from somewhere else, but I agree that it is easier to read with the non-exception first. |
||
return render_to_response('container.html', { | ||
'context_course': course, | ||
'xblock': xblock, | ||
'xblock_locator': locator, | ||
'unit': None if not ancestor_xblocks else ancestor_xblocks[0], | ||
'unit': unit, | ||
'unit_publish_state': unit_publish_state, | ||
'ancestor_xblocks': ancestor_xblocks, | ||
}) | ||
else: | ||
|
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.
Do you really want this to say "xblocks" as opposed to something more generic like "components"?
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 just took the JIRA title as I thought that's what we'd settled on. It's funny because in some ways xblocks are more generic, in that not all xblocks are components. However, from a non-technical standpoint, component is the more generic term. In the code there really is no notion of components, so all my new classes are called xblocks. From a user standpoint, though, the name xblock should never be seen.