Address code review feedback

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):
     """

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):

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()

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()

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

openedx/features/course_bookmarks/static/course_bookmarks/fixtures/bookmarks.html

 <div class="course-view container" id="course-container">
     <header class="page-header has-secondary">
         <div class="page-header-main">
-            <nav aria-label="Discussions" class="sr-is-focusable" tabindex="-1">
+            <nav aria-label="My Bookmarks" class="sr-is-focusable" tabindex="-1">
                 <div class="has-breadcrumbs"><div class="breadcrumbs">
                     <span class="nav-item">
                         <a href="/courses/course-v1:test-course/course/">Course</a>

openedx/features/course_bookmarks/static/course_bookmarks/js/spec/bookmarks_list_view_spec.js

@@ -73,7 +73,7 @@ define(['backbone',
 
                 expect(bookmarksView.$('.bookmarks-empty-detail-title').text().trim()).toBe(
                     'Use bookmarks to help you easily return to courseware pages. ' +
-                    'To bookmark a page, click on "Bookmark this page" beneath the unit title.'
+                    'To bookmark a page, click "Bookmark this page" under the page title.'
                 );
 
                 expect(bookmarksView.$('.paging-header').length).toBe(0);

openedx/features/course_bookmarks/static/course_bookmarks/templates/bookmarks-list.underscore

@@ -46,7 +46,7 @@
         </h3>
         <div  class="bookmarks-empty-detail">
             <span class="bookmarks-empty-detail-title">
-                <%- gettext('Use bookmarks to help you easily return to courseware pages. To bookmark a page, click on "Bookmark this page" beneath the unit title.') %>
+                <%- gettext('Use bookmarks to help you easily return to courseware pages. To bookmark a page, click "Bookmark this page" under the page title.') %>
             </span>
         </div>
     </div>

openedx/features/course_bookmarks/templates/course_bookmarks/course-bookmarks.html

@@ -27,11 +27,11 @@ from openedx.core.djangolib.markup import HTML
 <%include file="../courseware/course_navigation.html" args="active_page='courseware'" />
 
 <%block name="head_extra">
-${HTML(outline_fragment.head_html())}
+${HTML(bookmarks_fragment.head_html())}
 </%block>
 
 <%block name="footer_extra">
-${HTML(outline_fragment.foot_html())}
+${HTML(bookmarks_fragment.foot_html())}
 </%block>
 
 <%block name="content">
@@ -39,20 +39,21 @@ ${HTML(outline_fragment.foot_html())}
     <header class="page-header has-secondary">
         ## Breadcrumb navigation
         <div class="page-header-main">
-            <nav aria-label="Discussions" class="sr-is-focusable" tabindex="-1">
-                <div class="has-breadcrumbs"><div class="breadcrumbs">
-                    <span class="nav-item">
-                        <a href="${course_url}">Course</a>
-                    </span>
-                    <span class="icon fa fa-angle-right" aria-hidden="true"></span>
-                    <span class="nav-item">My Bookmarks</span>
-                </div>
+            <nav aria-label="${_('My Bookmarks')}" class="sr-is-focusable" tabindex="-1">
+                <div class="has-breadcrumbs">
+                    <div class="breadcrumbs">
+                        <span class="nav-item">
+                            <a href="${course_url}">Course</a>
+                        </span>
+                        <span class="icon fa fa-angle-right" aria-hidden="true"></span>
+                        <span class="nav-item">${_('My Bookmarks')}</span>
+                    </div>
                 </div>
             </nav>
         </div>
     </header>
     <div class="page-content">
-        ${HTML(outline_fragment.body_html())}
+        ${HTML(bookmarks_fragment.body_html())}
     </div>
 </div>
 </%block>

openedx/features/course_bookmarks/views/course_bookmarks.py

@@ -23,7 +23,7 @@ from xmodule.modulestore.django import modulestore
 
 class CourseBookmarksView(View):
     """
-    The home page for a course.
+    View showing the user's bookmarks for a course.
     """
     @method_decorator(login_required)
     @method_decorator(ensure_csrf_cookie)
@@ -31,7 +31,7 @@ class CourseBookmarksView(View):
     @method_decorator(ensure_valid_course_key)
     def get(self, request, course_id):
         """
-        Displays the home page for the specified course.
+        Displays the user's bookmarks for the specified course.
 
         Arguments:
             request: HTTP request
@@ -43,14 +43,14 @@ class CourseBookmarksView(View):
         course_url = reverse(course_url_name, kwargs={'course_id': unicode(course.id)})
 
         # Render the bookmarks list as a fragment
-        outline_fragment = CourseBookmarksFragmentView().render_to_fragment(request, course_id=course_id)
+        bookmarks_fragment = CourseBookmarksFragmentView().render_to_fragment(request, course_id=course_id)
 
-        # Render the entire unified course view
+        # Render the course bookmarks page
         context = {
             'csrf': csrf(request)['csrf_token'],
             'course': course,
             'course_url': course_url,
-            'outline_fragment': outline_fragment,
+            'bookmarks_fragment': bookmarks_fragment,
             'disable_courseware_js': True,
             'uses_pattern_library': True,
         }
@@ -63,7 +63,7 @@ class CourseBookmarksFragmentView(EdxFragmentView):
     """
     def render_to_fragment(self, request, course_id=None, **kwargs):
         """
-        Renders the course outline as a fragment.
+        Renders the user's course bookmarks as a fragment.
         """
         course_key = CourseKey.from_string(course_id)
         course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)

openedx/features/course_experience/templates/course_experience/course-home.html

@@ -40,10 +40,10 @@ ${HTML(outline_fragment.foot_html())}
     <header class="page-header has-secondary">
         <div class="page-header-secondary">
             <div class="form-actions">
-                <a class="btn" href="${reverse('courseware', kwargs={'course_id': unicode(course.id.to_deprecated_string())})}">
+                <a class="btn action-resume-course" href="${reverse('courseware', kwargs={'course_id': unicode(course.id.to_deprecated_string())})}">
                     ${_("Resume Course")}
                 </a>
-                <a class="btn" href="${reverse('openedx.course_bookmarks.home', args=[course.id])}">
+                <a class="btn action-show-bookmarks" href="${reverse('openedx.course_bookmarks.home', args=[course.id])}">
                     ${_("Bookmarks")}
                 </a>
             </div>