Skip to content
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

Merged
merged 25 commits into from
Apr 10, 2014
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
7770d9d
Implement editing from the Studio container page.
andy-armstrong Mar 17, 2014
4f85228
Make tab name comparison case-insensitive
andy-armstrong Apr 1, 2014
be8e994
making chrome scroll up when showing the component menu on the Studio…
Apr 1, 2014
df32f10
Refactor test logic for switching editor tabs
andy-armstrong Apr 1, 2014
2425aab
maintain scroll position after closing the component editing modal in…
Apr 2, 2014
636fa19
small ui fixes in Studio - textbooks upload and loading font-size
Apr 2, 2014
ee42902
removed edit-dialog and uploads css, changed component edit icon to p…
Apr 2, 2014
052949d
Fix unit tests
andy-armstrong Apr 2, 2014
e18dd7f
fixed mixin sass
Apr 2, 2014
dce075a
Changes based upon code review comments
andy-armstrong Apr 2, 2014
415fe8d
Clean up the logic for the edit modal's title
andy-armstrong Apr 2, 2014
07c54dc
Fix up edit_component so that it doesn't take an unnecessary step arg…
andy-armstrong Apr 2, 2014
b23f17d
removed commented out lines
Apr 3, 2014
230c0b5
Add more improvements from the review.
andy-armstrong Apr 3, 2014
5ab159b
Try to fix the failing JavaScript test
andy-armstrong Apr 3, 2014
b779b8a
Add unit tests for custom tabs and modal titles
andy-armstrong Apr 3, 2014
88d95d1
Rebuild the way modals are constructed to support nesting.
andy-armstrong Apr 3, 2014
a6aae72
Address the next round of review comments.
andy-armstrong Apr 8, 2014
9de6f10
Fix typo in textbook_spec.js
andy-armstrong Apr 8, 2014
2ae28c5
Improve unit testing of xblock editing
andy-armstrong Apr 8, 2014
d55ad61
Make the LaText source compiler link visible
andy-armstrong Apr 9, 2014
a3e26e6
Implement next round of code reviews
andy-armstrong Apr 9, 2014
b78508a
Indicate LaTex problems with a specific class on the editor
andy-armstrong Apr 9, 2014
2bf64d7
Remove feature flag for testing on container pages
andy-armstrong Apr 9, 2014
df0973e
addressing review feedback
Apr 9, 2014
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes,
in roughly chronological order, most recent first. Add your entries at or near
the top. Include a label indicating the component affected.

Studio: Add edit button to leaf xblocks on the container page. STUD-1306.
Copy link

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"?

Copy link
Contributor Author

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.


Blades: Add LTI context_id parameter. BLD-584.

Blades: Update LTI resource_link_id parameter. BLD-768.
Expand Down
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/features/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ def attach_file(filename, sub_path):

def upload_file(filename, sub_path=''):
attach_file(filename, sub_path)
button_css = '.upload-dialog .action-upload'
button_css = '.wrapper-modal-window-assetupload .action-upload'
world.css_click(button_css)


Expand Down
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/features/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,11 @@ def change_display_name(step, display_name):
world.edit_component_and_select_settings()
index = world.get_setting_entry_index(DISPLAY_NAME)
world.set_field_value(index, display_name)
world.save_component(step)
world.save_component()


@step(u'I unset the display name')
def unset_display_name(step):
world.edit_component_and_select_settings()
world.revert_setting_entry(DISPLAY_NAME)
world.save_component(step)
world.save_component()
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -54,6 +55,11 @@ def click_new_component_button(step, component_button_css):

world.css_click(component_button_css)

if FEATURES['USE_CONTAINER_PAGE_FOR_TESTING']:
unit_url = world.browser.url
container_url = unit_url.replace('/unit/', '/container/')
world.visit(container_url)


def _click_advanced():
css = 'ul.problem-type-tabs a[href="#tab2"]'
Expand Down Expand Up @@ -105,9 +111,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():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not fail if settings are not visible. Is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand All @@ -116,14 +129,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
Expand Down Expand Up @@ -173,14 +197,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)
Expand All @@ -189,7 +213,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ Feature: CMS.Discussion Component Editor

Scenario: User can view discussion component metadata
Given I have created a Discussion Tag
And I edit and select Settings
And I edit the component
Then I see three alphabetized settings and their expected values

# Safari doesn't save the name properly
@skip_safari
Scenario: User can modify display name
Given I have created a Discussion Tag
And I edit and select Settings
And I edit the component
Then I can modify the display name
And my display name change is persisted on save
5 changes: 5 additions & 0 deletions cms/djangoapps/contentstore/features/discussion-editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,8 @@ def i_see_only_the_settings_and_values(step):
['Display Name', "Discussion", False],
['Subcategory', "Topic-Level Student-Visible Label", False]
])


@step('I edit the component$')
def i_edit_and_select_settings(_step):
world.edit_component()
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/features/html-editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def perform_action_in_plugin(action):

@step('I save the page$')
def i_click_on_save(step):
world.save_component(step)
world.save_component()


@step('the page text contains:')
Expand Down
5 changes: 2 additions & 3 deletions cms/djangoapps/contentstore/features/pages.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,13 @@ def click_edit_or_delete(step, edit_or_delete):

@step(u'I change the name to "([^"]*)"$')
def change_name(step, new_name):
settings_css = '#settings-mode a'
settings_css = '.settings-button'
world.css_click(settings_css)
input_css = 'input.setting-input'
world.css_fill(input_css, new_name)
if world.is_firefox():
world.trigger_event(input_css)
save_button = 'a.save-button'
world.css_click(save_button)
world.save_component()


@step(u'I drag the first static page to the last$')
Expand Down
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/features/problem-editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,5 +286,5 @@ def set_weight(weight):


def open_high_level_source():
world.css_click('a.edit-button')
world.edit_component()
world.css_click('.launch-latex-compiler > a')
11 changes: 4 additions & 7 deletions cms/djangoapps/contentstore/features/transcripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link

Choose a reason for hiding this comment

The 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 "([^"]*)"$')
Expand Down
11 changes: 5 additions & 6 deletions cms/djangoapps/contentstore/features/video-editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def check_header(self, name, value):
def success_upload_file(filename):
upload_file(filename, sub_path="uploads/")
world.css_has_text('#upload_confirm', 'Success!')
world.is_css_not_present('.wrapper-dialog-assetupload', wait_time=30)
world.is_css_not_present('.wrapper-modal-window-assetupload', wait_time=30)


def get_translations_container():
Expand Down Expand Up @@ -112,11 +112,10 @@ def set_show_captions(step, setting):
# Prevent cookies from overriding course settings
world.browser.cookies.delete('hide_captions')

world.css_click('a.edit-button')
world.wait_for(lambda _driver: world.css_visible('a.save-button'))
world.click_link_by_text('Advanced')
world.edit_component()
world.select_editor_tab('Advanced')
world.browser.select('Transcript Display', setting)
world.css_click('a.save-button')
world.save_component()


@step('when I view the video it (.*) show the captions$')
Expand Down Expand Up @@ -161,7 +160,7 @@ def correct_video_settings(_step):

@step('my video display name change is persisted on save$')
def video_name_persisted(step):
world.css_click('a.save-button')
world.save_component()
reload_the_page(step)
world.wait_for_xmodule()
world.edit_component()
Expand Down
8 changes: 6 additions & 2 deletions cms/djangoapps/contentstore/views/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ def unit_handler(request, tag=None, package_id=None, branch=None, version_guid=N
'context_course': course,
'unit': item,
'unit_locator': locator,
'xblocks': xblocks,
'locators': locators,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just say the following:
'xblocks': xblocks
since xblocks is already a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recollection is that I tried that and it didn't serialize unless I made a new list. However, trying it now this doesn't seem to be the case, so I'll make the change.

'component_templates': component_templates,
'draft_preview_link': preview_lms_link,
Expand Down Expand Up @@ -317,14 +318,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
Copy link

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
unit_publish_state = compute_publish_state(unit) if unit else None

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
Expand Down
3 changes: 3 additions & 0 deletions cms/djangoapps/contentstore/views/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

__all__ = ['edge', 'event', 'landing']

EDITING_TEMPLATES = [
"basic-modal", "modal-button", "edit-xblock-modal", "editor-mode-button", "upload-dialog", "image-modal"
]

# points to the temporary course landing page with log in and sign up
def landing(request, org, course, coursename):
Expand Down
4 changes: 3 additions & 1 deletion cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

from .access import has_course_access
from .helpers import _xmodule_recurse
from contentstore.utils import compute_publish_state, PublishState
from contentstore.views.preview import get_preview_fragment
from edxmako.shortcuts import render_to_string
from models.settings.course_grading import CourseGradingModel
Expand Down Expand Up @@ -224,11 +225,12 @@ def xblock_view_handler(request, package_id, view_name, tag=None, branch=None, v
})
elif view_name in ('student_view', 'container_preview'):
is_container_view = (view_name == 'container_preview')
component_publish_state = compute_publish_state(component)
is_read_only_view = component_publish_state == PublishState.public

# Only show the new style HTML for the container view, i.e. for non-verticals
# Note: this special case logic can be removed once the unit page is replaced
# with the new container view.
is_read_only_view = is_container_view
context = {
'runtime_type': 'studio',
'container_view': is_container_view,
Expand Down
3 changes: 1 addition & 2 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from __future__ import absolute_import

import logging
import hashlib
from functools import partial

from django.conf import settings
Expand Down Expand Up @@ -170,7 +169,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
"""
# Only add the Studio wrapper when on the container page. The unit page will remain as is for now.
if context.get('container_view', None) and view == 'student_view':
locator = loc_mapper().translate_location(xblock.course_id, xblock.location)
locator = loc_mapper().translate_location(xblock.course_id, xblock.location, published=False)
template_context = {
'xblock_context': context,
'xblock': xblock,
Expand Down
Loading