From e4d355a686ec324b7415ebc9ae3e8d103cdd3fa4 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Tue, 21 Mar 2017 16:55:40 -0400 Subject: [PATCH] Address code review feedback --- .../test/acceptance/pages/lms/course_home.py | 7 ++ .../test/acceptance/pages/lms/courseware.py | 8 +- .../acceptance/tests/lms/test_bookmarks.py | 111 ++++++++---------- common/test/acceptance/tests/lms/test_lms.py | 7 ++ .../courseware/course_navigation.html | 2 +- .../course_bookmarks/fixtures/bookmarks.html | 2 +- .../js/spec/bookmarks_list_view_spec.js | 2 +- .../templates/bookmarks-list.underscore | 2 +- .../course_bookmarks/course-bookmarks.html | 23 ++-- .../views/course_bookmarks.py | 12 +- .../course_experience/course-home.html | 4 +- 11 files changed, 92 insertions(+), 88 deletions(-) diff --git a/common/test/acceptance/pages/lms/course_home.py b/common/test/acceptance/pages/lms/course_home.py index 1b34680e6e..0c12fb71f9 100644 --- a/common/test/acceptance/pages/lms/course_home.py +++ b/common/test/acceptance/pages/lms/course_home.py @@ -4,6 +4,7 @@ LMS Course Home page object from bok_choy.page_object import PageObject +from common.test.acceptance.pages.lms.bookmarks import BookmarksPage from common.test.acceptance.pages.lms.course_page import CoursePage from common.test.acceptance.pages.lms.courseware import CoursewarePage @@ -25,6 +26,12 @@ class CourseHomePage(CoursePage): # TODO: TNL-6546: Remove the following self.unified_course_view = False + def click_bookmarks_button(self): + """ Click on Bookmarks button """ + self.q(css='.bookmarks-list-button').first.click() + bookmarks_page = BookmarksPage(self.browser, self.course_id) + bookmarks_page.visit() + class CourseOutlinePage(PageObject): """ diff --git a/common/test/acceptance/pages/lms/courseware.py b/common/test/acceptance/pages/lms/courseware.py index f2f8ba3137..0712b0c7c1 100644 --- a/common/test/acceptance/pages/lms/courseware.py +++ b/common/test/acceptance/pages/lms/courseware.py @@ -7,6 +7,7 @@ from bok_choy.promise import EmptyPromise import re from selenium.webdriver.common.action_chains import ActionChains +from common.test.acceptance.pages.lms.bookmarks import BookmarksPage from common.test.acceptance.pages.lms.course_page import CoursePage @@ -310,11 +311,12 @@ class CoursewarePage(CoursePage): self.q(css='.bookmark-button').first.click() EmptyPromise(lambda: self.bookmark_button_state != previous_state, "Bookmark button toggled").fulfill() - def click_bookmarks_button(self, wait_for_results=True): + # TODO: TNL-6546: Remove this helper function + def click_bookmarks_button(self): """ Click on Bookmarks button """ self.q(css='.bookmarks-list-button').first.click() - if wait_for_results: - EmptyPromise(lambda: self.q(css='#my-bookmarks').present, "Bookmarks results present").fulfill() + bookmarks_page = BookmarksPage(self.browser, self.course_id) + bookmarks_page.visit() class CoursewareSequentialTabPage(CoursePage): diff --git a/common/test/acceptance/tests/lms/test_bookmarks.py b/common/test/acceptance/tests/lms/test_bookmarks.py index be94a6505f..41bcd127ae 100644 --- a/common/test/acceptance/tests/lms/test_bookmarks.py +++ b/common/test/acceptance/tests/lms/test_bookmarks.py @@ -190,7 +190,7 @@ class BookmarksTest(BookmarksTestMixin): self.courseware_page.click_bookmark_unit_button() self.assertEqual(self.courseware_page.bookmark_icon_visible, bookmark_icon_state) self.assertEqual(self.courseware_page.bookmark_button_state, bookmark_button_state) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertEqual(self.bookmarks_page.count(), bookmarked_count) def _verify_pagination_info( @@ -212,13 +212,6 @@ class BookmarksTest(BookmarksTestMixin): self.assertEqual(self.bookmarks_page.get_current_page_number(), current_page_number) self.assertEqual(self.bookmarks_page.get_total_pages, total_pages) - def _navigate_to_bookmarks_list(self): - """ - Navigates and verifies the bookmarks list page. - """ - self.courseware_page.click_bookmarks_button() - self.assertTrue(self.bookmarks_page.results_present()) - def _verify_breadcrumbs(self, num_units, modified_name=None): """ Verifies the breadcrumb trail. @@ -277,22 +270,31 @@ class BookmarksTest(BookmarksTestMixin): self.course_home_page.outline.go_to_section('TestSection{}'.format(index), 'TestSubsection{}'.format(index)) self._toggle_bookmark_and_verify(False, '', 0) + # TODO: TNL-6546: Remove this test + def test_courseware_bookmarks_button(self): + """ + Scenario: (Temporarily) test that the courseware's "Bookmarks" button works. + """ + self.setup_test() + self.bookmark_units(2) + self.courseware_page.visit() + self.courseware_page.click_bookmarks_button() + self.assertTrue(self.bookmarks_page.is_browser_on_page()) + def test_empty_bookmarks_list(self): """ Scenario: An empty bookmarks list is shown if there are no bookmarked units. Given that I am a registered user - And I visit my courseware page - And I can see the Bookmarks button - When I click on Bookmarks button + And I visit my bookmarks page Then I should see an empty bookmarks list And empty bookmarks list content is correct """ self.setup_test() - self.courseware_page.click_bookmarks_button() - - empty_list_text = ("Use bookmarks to help you easily return to courseware pages. " - "To bookmark a page, click on \"Bookmark this page\" beneath the unit title.") + self.bookmarks_page.visit() + empty_list_text = ( + 'Use bookmarks to help you easily return to courseware pages. ' + 'To bookmark a page, click "Bookmark this page" under the page title.') self.assertEqual(self.bookmarks_page.empty_list_text(), empty_list_text) def test_bookmarks_list(self): @@ -300,9 +302,8 @@ class BookmarksTest(BookmarksTestMixin): Scenario: A bookmarks list is shown if there are bookmarked units. Given that I am a registered user - And I visit my courseware page And I have bookmarked 2 units - When I click on Bookmarks button + And I visit my bookmarks page Then I should see a bookmarked list with 2 bookmark links And breadcrumb trail is correct for a bookmark When I click on bookmarked link @@ -310,8 +311,7 @@ class BookmarksTest(BookmarksTestMixin): """ self.setup_test() self.bookmark_units(2) - - self._navigate_to_bookmarks_list() + self.bookmarks_page.visit() self._verify_breadcrumbs(num_units=2) self._verify_pagination_info( @@ -328,11 +328,10 @@ class BookmarksTest(BookmarksTestMixin): xblock_usage_ids = [xblock.locator for xblock in xblocks] # Verify link navigation for index in range(2): + self.bookmarks_page.visit() self.bookmarks_page.click_bookmarked_block(index) self.courseware_page.wait_for_page() self.assertIn(self.courseware_page.active_usage_id(), xblock_usage_ids) - self.courseware_page.visit().wait_for_page() - self.courseware_page.click_bookmarks_button() def test_bookmark_shows_updated_breadcrumb_after_publish(self): """ @@ -344,16 +343,14 @@ class BookmarksTest(BookmarksTestMixin): Then I visit unit page in studio Then I change unit display_name And I publish the changes - Then I visit my courseware page - And I visit bookmarks list page + Then I visit my bookmarks page When I see the bookmark - Then I can see the breadcrumb trail - with updated display_name. + Then I can see the breadcrumb trail has the updated display_name. """ self.setup_test(num_chapters=1) self.bookmark_units(num_units=1) - self._navigate_to_bookmarks_list() + self.bookmarks_page.visit() self._verify_breadcrumbs(num_units=1) LogoutPage(self.browser).visit() @@ -370,9 +367,8 @@ class BookmarksTest(BookmarksTestMixin): LogoutPage(self.browser).visit() LmsAutoAuthPage(self.browser, username=self.USERNAME, email=self.EMAIL, course_id=self.course_id).visit() - self.courseware_page.visit() - self._navigate_to_bookmarks_list() + self.bookmarks_page.visit() self._verify_breadcrumbs(num_units=1, modified_name=modified_name) def test_unreachable_bookmark(self): @@ -380,19 +376,18 @@ class BookmarksTest(BookmarksTestMixin): Scenario: We should get a HTTP 404 for an unreachable bookmark. Given that I am a registered user - And I visit my courseware page And I have bookmarked 2 units - Then I delete a bookmarked unit - Then I click on Bookmarks button - And I should see a bookmarked list - When I click on deleted bookmark + And I delete a bookmarked unit + And I visit my bookmarks page + Then I should see a bookmarked list + When I click on the deleted bookmark Then I should navigated to 404 page """ self.setup_test(num_chapters=1) self.bookmark_units(1) self._delete_section(0) - self._navigate_to_bookmarks_list() + self.bookmarks_page.visit() self._verify_pagination_info( bookmark_count_on_current_page=1, @@ -411,15 +406,14 @@ class BookmarksTest(BookmarksTestMixin): Scenario: We can't get bookmarks more than default page size. Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 11 units available - Then I click on Bookmarks button - And I should see a bookmarked list - And bookmark list contains 10 bookmarked items + And I visit my bookmarks page + Then I should see a bookmarked list + And the bookmark list should contain 10 bookmarked items """ self.setup_test(11) self.bookmark_units(11) - self._navigate_to_bookmarks_list() + self.bookmarks_page.visit() self._verify_pagination_info( bookmark_count_on_current_page=10, @@ -434,17 +428,15 @@ class BookmarksTest(BookmarksTestMixin): """ Scenario: Bookmarks list pagination is working as expected for single page Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 2 units available - Then I click on Bookmarks button - And I should see a bookmarked list with 2 bookmarked items + And I visit my bookmarks page + Then I should see a bookmarked list with 2 bookmarked items And I should see paging header and footer with correct data And previous and next buttons are disabled """ self.setup_test(num_chapters=2) self.bookmark_units(num_units=2) - - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertTrue(self.bookmarks_page.results_present()) self._verify_pagination_info( bookmark_count_on_current_page=2, @@ -460,11 +452,10 @@ class BookmarksTest(BookmarksTestMixin): Scenario: Next button is working as expected for bookmarks list pagination Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 12 units available + And I visit my bookmarks page - Then I click on Bookmarks button - And I should see a bookmarked list of 10 items + Then I should see a bookmarked list of 10 items And I should see paging header and footer with correct info Then I click on next page button in footer @@ -475,7 +466,7 @@ class BookmarksTest(BookmarksTestMixin): self.setup_test(num_chapters=12) self.bookmark_units(num_units=12) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertTrue(self.bookmarks_page.results_present()) self._verify_pagination_info( @@ -502,9 +493,8 @@ class BookmarksTest(BookmarksTestMixin): Scenario: Previous button is working as expected for bookmarks list pagination Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 12 units available - And I click on Bookmarks button + And I visit my bookmarks page Then I click on next page button in footer And I should be navigated to second page @@ -518,7 +508,7 @@ class BookmarksTest(BookmarksTestMixin): self.setup_test(num_chapters=12) self.bookmark_units(num_units=12) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertTrue(self.bookmarks_page.results_present()) self.bookmarks_page.press_next_page_button() @@ -546,11 +536,9 @@ class BookmarksTest(BookmarksTestMixin): Scenario: Bookmarks list pagination works as expected for valid page number Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 12 units available - - Then I click on Bookmarks button - And I should see a bookmarked list + And I visit my bookmarks page + Then I should see a bookmarked list And I should see total page value is 2 Then I enter 2 in the page number input And I should be navigated to page 2 @@ -558,7 +546,7 @@ class BookmarksTest(BookmarksTestMixin): self.setup_test(num_chapters=11) self.bookmark_units(num_units=11) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertTrue(self.bookmarks_page.results_present()) self.assertEqual(self.bookmarks_page.get_total_pages, 2) @@ -577,10 +565,9 @@ class BookmarksTest(BookmarksTestMixin): Scenario: Bookmarks list pagination works as expected for invalid page number Given that I am a registered user - And I visit my courseware page And I have bookmarked all the 11 units available - Then I click on Bookmarks button - And I should see a bookmarked list + And I visit my bookmarks page + Then I should see a bookmarked list And I should see total page value is 2 Then I enter 3 in the page number input And I should stay at page 1 @@ -588,7 +575,7 @@ class BookmarksTest(BookmarksTestMixin): self.setup_test(num_chapters=11) self.bookmark_units(num_units=11) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.assertTrue(self.bookmarks_page.results_present()) self.assertEqual(self.bookmarks_page.get_total_pages, 2) @@ -627,7 +614,7 @@ class BookmarksTest(BookmarksTestMixin): } ] self.bookmark_units(num_units=1) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self._verify_pagination_info( bookmark_count_on_current_page=1, @@ -653,5 +640,5 @@ class BookmarksA11yTests(BookmarksTestMixin): """ self.setup_test(num_chapters=11) self.bookmark_units(num_units=11) - self.courseware_page.click_bookmarks_button() + self.bookmarks_page.visit() self.bookmarks_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/lms/test_lms.py b/common/test/acceptance/tests/lms/test_lms.py index 3ead4ce3b5..97ea7fcab8 100644 --- a/common/test/acceptance/tests/lms/test_lms.py +++ b/common/test/acceptance/tests/lms/test_lms.py @@ -25,6 +25,7 @@ from common.test.acceptance.pages.common.logout import LogoutPage from common.test.acceptance.pages.lms import BASE_URL from common.test.acceptance.pages.lms.account_settings import AccountSettingsPage from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage +from common.test.acceptance.pages.lms.bookmarks import BookmarksPage from common.test.acceptance.pages.lms.create_mode import ModeCreationPage from common.test.acceptance.pages.lms.course_home import CourseHomePage from common.test.acceptance.pages.lms.course_info import CourseInfoPage @@ -853,6 +854,12 @@ class HighLevelTabTest(UniqueCourseTest): self.course_home_page.outline.go_to_section('Test Section 2', 'Test Subsection 3') self.assertTrue(self.courseware_page.nav.is_on_section('Test Section 2', 'Test Subsection 3')) + # Verify that we can navigate to the bookmarks page + self.course_home_page.visit() + self.course_home_page.click_bookmarks_button() + bookmarks_page = BookmarksPage(self.browser, self.course_id) + self.assertTrue(bookmarks_page.is_browser_on_page()) + @attr('a11y') def test_course_home_a11y(self): self.course_home_page.visit() diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index 1a683302c4..6e3b041412 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -20,7 +20,7 @@ if active_page is None and active_page_context is not UNDEFINED: def selected(is_selected): return "selected" if is_selected else "" -show_preview_menu = not disable_preview_menu and staff_access and active_page in ["course", "courseware", "info", "progress"] +show_preview_menu = not disable_preview_menu and staff_access and active_page in ["courseware", "info", "progress"] cohorted_user_partition = get_cohorted_user_partition(course) masquerade_user_name = masquerade.user_name if masquerade else None masquerade_group_id = masquerade.group_id if masquerade else None diff --git a/openedx/features/course_bookmarks/static/course_bookmarks/fixtures/bookmarks.html b/openedx/features/course_bookmarks/static/course_bookmarks/fixtures/bookmarks.html index fed78bc0d2..e5906eeb84 100644 --- a/openedx/features/course_bookmarks/static/course_bookmarks/fixtures/bookmarks.html +++ b/openedx/features/course_bookmarks/static/course_bookmarks/fixtures/bookmarks.html @@ -1,7 +1,7 @@
-