From a34081c24bb2f2299afb8ec30d4ad5296acccf6d Mon Sep 17 00:00:00 2001
From: Harry Rein <hrein@edx.org>
Date: Fri, 23 Jun 2017 11:07:41 -0400
Subject: [PATCH] Assuring that courses without updates cannot see the updates
 link from the home page and that the update message does not throw an error
 upon loading from a direct URL without any updates. Added test to assure this
 behavior and fixed up welcome message tests.

---
 openedx/features/course_bookmarks/plugins.py  |  2 +-
 .../course_experience/course_tools.py         |  2 +-
 openedx/features/course_experience/plugins.py | 16 +++++----
 .../course-home-fragment.html                 |  2 +-
 .../course-updates-fragment.html              | 26 ++++++++------
 .../tests/views/test_course_home.py           | 34 +++++++++++++-----
 .../tests/views/test_course_updates.py        |  5 +--
 .../tests/views/test_welcome_message.py       |  2 +-
 .../course_experience/views/course_home.py    |  8 ++---
 .../course_experience/views/course_updates.py | 35 +++++++++++++------
 .../views/welcome_message.py                  |  7 ++--
 11 files changed, 87 insertions(+), 52 deletions(-)

diff --git a/openedx/features/course_bookmarks/plugins.py b/openedx/features/course_bookmarks/plugins.py
index 3f24942ae8..17880af8b9 100644
--- a/openedx/features/course_bookmarks/plugins.py
+++ b/openedx/features/course_bookmarks/plugins.py
@@ -12,7 +12,7 @@ class CourseBookmarksTool(CourseTool):
     The course bookmarks tool.
     """
     @classmethod
-    def is_enabled(cls, course_key):
+    def is_enabled(cls, request, course_key):
         """
         Always show the bookmarks tool.
         """
diff --git a/openedx/features/course_experience/course_tools.py b/openedx/features/course_experience/course_tools.py
index 1057f355bc..76ecb4e857 100644
--- a/openedx/features/course_experience/course_tools.py
+++ b/openedx/features/course_experience/course_tools.py
@@ -18,7 +18,7 @@ class CourseTool(object):
     """
 
     @classmethod
-    def is_enabled(cls, course_key):
+    def is_enabled(cls, request, course_key):
         """
         Returns true if this tool is enabled for the specified course key.
         """
diff --git a/openedx/features/course_experience/plugins.py b/openedx/features/course_experience/plugins.py
index 611c348382..4cb091544f 100644
--- a/openedx/features/course_experience/plugins.py
+++ b/openedx/features/course_experience/plugins.py
@@ -3,13 +3,15 @@ Platform plugins to support the course experience.
 
 This includes any locally defined CourseTools.
 """
-
 from django.core.urlresolvers import reverse
 from django.utils.translation import ugettext as _
 
-from . import UNIFIED_COURSE_TAB_FLAG, SHOW_REVIEWS_TOOL_FLAG
-from views.course_reviews import CourseReviewsModuleFragmentView
 from course_tools import CourseTool
+from courseware.courses import get_course_by_id
+from views.course_reviews import CourseReviewsModuleFragmentView
+from views.course_updates import CourseUpdatesFragmentView
+
+from . import SHOW_REVIEWS_TOOL_FLAG, UNIFIED_COURSE_TAB_FLAG
 
 
 class CourseUpdatesTool(CourseTool):
@@ -31,11 +33,13 @@ class CourseUpdatesTool(CourseTool):
         return 'fa fa-newspaper-o'
 
     @classmethod
-    def is_enabled(cls, course_key):
+    def is_enabled(cls, request, course_key):
         """
         Returns True if this tool is enabled for the specified course key.
         """
-        return UNIFIED_COURSE_TAB_FLAG.is_enabled(course_key)
+        course = get_course_by_id(course_key)
+        has_updates = CourseUpdatesFragmentView.has_updates(request, course)
+        return UNIFIED_COURSE_TAB_FLAG.is_enabled(course_key) and has_updates
 
     @classmethod
     def url(cls, course_key):
@@ -64,7 +68,7 @@ class CourseReviewsTool(CourseTool):
         return 'fa fa-star'
 
     @classmethod
-    def is_enabled(cls, course_key):
+    def is_enabled(cls, request, course_key):
         """
         Returns True if this tool is enabled for the specified course key.
         """
diff --git a/openedx/features/course_experience/templates/course_experience/course-home-fragment.html b/openedx/features/course_experience/templates/course_experience/course-home-fragment.html
index 21abafd7fe..8f858d31db 100644
--- a/openedx/features/course_experience/templates/course_experience/course-home-fragment.html
+++ b/openedx/features/course_experience/templates/course_experience/course-home-fragment.html
@@ -75,7 +75,7 @@ from openedx.features.course_experience.course_tools import CourseToolsPluginMan
                         <h3 class="hd-6">${_("Course Tools")}</h3>
                         <ul class="list-unstyled">
                             % for course_tool in course_tools:
-                                % if course_tool.is_enabled(course_key):
+                                % if course_tool.is_enabled(request, course_key):
                                     <li>
                                         <a href="${course_tool.url(course_key)}">
                                             <span class="icon ${course_tool.icon_classes()}" aria-hidden="true"></span>
diff --git a/openedx/features/course_experience/templates/course_experience/course-updates-fragment.html b/openedx/features/course_experience/templates/course_experience/course-updates-fragment.html
index d17c483087..721853718f 100644
--- a/openedx/features/course_experience/templates/course_experience/course-updates-fragment.html
+++ b/openedx/features/course_experience/templates/course_experience/course-updates-fragment.html
@@ -29,20 +29,26 @@ from openedx.features.course_experience import course_home_page_title
         </div>
     </header>
     <div class="page-content">
-        % if len(plain_html_updates) > 0:
+        % if plain_html_updates:
             ${HTML(plain_html_updates)}
         % else:
             <div class="all-updates">
-                % for index, update in enumerate(updates):
-                <article class="updates-article">
-                    % if not update.get("is_error"):
-                    <div class="date">${update.get("date")}</div>
-                    % endif
-                    <div class="article-content">
-                        ${HTML(update.get("content"))}
+                % if updates:
+                    % for index, update in enumerate(updates):
+                        <article class="updates-article">
+                            % if not update.get("is_error"):
+                                <div class="date">${update.get("date")}</div>
+                            % endif
+                            <div class="article-content">
+                                ${HTML(update.get("content"))}
+                            </div>
+                        </article>
+                    % endfor
+                % else:
+                    <div class="well depth-0 message-area">
+                        ${_("This course does not have any updates.")}
                     </div>
-                </article>
-              % endfor
+                % endif
             </div>
         % endif
     </div>
diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py
index aeaaea9bd4..6ec1e32450 100644
--- a/openedx/features/course_experience/tests/views/test_course_home.py
+++ b/openedx/features/course_experience/tests/views/test_course_home.py
@@ -16,6 +16,8 @@ from .test_course_updates import create_course_update, remove_course_updates
 
 TEST_PASSWORD = 'test'
 TEST_WELCOME_MESSAGE = '<h2>Welcome!</h2>'
+TEST_UPDATE_MESSAGE = '<h2>Test Update!</h2>'
+TEST_COURSE_UPDATES_TOOL = '/course/updates">'
 
 
 def course_home_url(course):
@@ -55,9 +57,6 @@ class TestCourseHomePage(SharedModuleStoreTestCase):
         cls.user = UserFactory(password=TEST_PASSWORD)
         CourseEnrollment.enroll(cls.user, cls.course.id)
 
-        # Create a welcome message
-        create_course_update(cls.course, cls.user, TEST_WELCOME_MESSAGE)
-
     def setUp(self):
         """
         Set up for the tests.
@@ -65,22 +64,39 @@ class TestCourseHomePage(SharedModuleStoreTestCase):
         super(TestCourseHomePage, self).setUp()
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
 
-    def tearDown(self):
-        remove_course_updates(self.course)
-        super(TestCourseHomePage, self).tearDown()
-
     @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=True)
     def test_welcome_message_when_unified(self):
+        # Create a welcome message
+        create_course_update(self.course, self.user, TEST_WELCOME_MESSAGE)
+
         url = course_home_url(self.course)
         response = self.client.get(url)
         self.assertContains(response, TEST_WELCOME_MESSAGE, status_code=200)
 
     @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=False)
     def test_welcome_message_when_not_unified(self):
+        # Create a welcome message
+        create_course_update(self.course, self.user, TEST_WELCOME_MESSAGE)
+
         url = course_home_url(self.course)
         response = self.client.get(url)
         self.assertNotContains(response, TEST_WELCOME_MESSAGE, status_code=200)
 
+    @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=True)
+    def test_updates_tool_visibility(self):
+        """
+        Verify that the updates course tool is visible only when the course
+        has one or more updates.
+        """
+        url = course_home_url(self.course)
+        response = self.client.get(url)
+        self.assertNotContains(response, TEST_COURSE_UPDATES_TOOL, status_code=200)
+
+        create_course_update(self.course, self.user, TEST_UPDATE_MESSAGE)
+        url = course_home_url(self.course)
+        response = self.client.get(url)
+        self.assertContains(response, TEST_COURSE_UPDATES_TOOL, status_code=200)
+
     def test_queries(self):
         """
         Verify that the view's query count doesn't regress.
@@ -89,7 +105,7 @@ class TestCourseHomePage(SharedModuleStoreTestCase):
         course_home_url(self.course)
 
         # Fetch the view and verify the query counts
-        with self.assertNumQueries(51):
-            with check_mongo_calls(5):
+        with self.assertNumQueries(46):
+            with check_mongo_calls(4):
                 url = course_home_url(self.course)
                 self.client.get(url)
diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py
index f675830de7..f0967a9ae0 100644
--- a/openedx/features/course_experience/tests/views/test_course_updates.py
+++ b/openedx/features/course_experience/tests/views/test_course_updates.py
@@ -61,7 +61,7 @@ def create_course_updates_block(course, user):
     return course_updates
 
 
-def remove_course_updates(course):
+def remove_course_updates(user, course):
     """
     Remove any course updates in the specified course.
     """
@@ -69,6 +69,7 @@ def remove_course_updates(course):
     try:
         course_updates = modulestore().get_item(updates_usage_key)
         course_updates.items = []
+        modulestore().update_item(course_updates, user.id)
     except ItemNotFoundError:
         pass
 
@@ -105,7 +106,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase):
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
 
     def tearDown(self):
-        remove_course_updates(self.course)
+        remove_course_updates(self.user, self.course)
         super(TestCourseUpdatesPage, self).tearDown()
 
     def test_view(self):
diff --git a/openedx/features/course_experience/tests/views/test_welcome_message.py b/openedx/features/course_experience/tests/views/test_welcome_message.py
index 4bbb436700..db1e1f6270 100644
--- a/openedx/features/course_experience/tests/views/test_welcome_message.py
+++ b/openedx/features/course_experience/tests/views/test_welcome_message.py
@@ -58,7 +58,7 @@ class TestWelcomeMessageView(ModuleStoreTestCase):
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
 
     def tearDown(self):
-        remove_course_updates(self.course)
+        remove_course_updates(self.user, self.course)
         super(TestWelcomeMessageView, self).tearDown()
 
     def test_welcome_message(self):
diff --git a/openedx/features/course_experience/views/course_home.py b/openedx/features/course_experience/views/course_home.py
index 9cd74cd398..e9c801d9c8 100644
--- a/openedx/features/course_experience/views/course_home.py
+++ b/openedx/features/course_experience/views/course_home.py
@@ -19,9 +19,8 @@ from util.views import ensure_valid_course_key
 from ..utils import get_course_outline_block_tree
 from .course_dates import CourseDatesFragmentView
 from .course_outline import CourseOutlineFragmentView
-from .course_reviews import CourseReviewsModuleFragmentView
-from .welcome_message import WelcomeMessageFragmentView
 from .course_sock import CourseSockFragmentView
+from .welcome_message import WelcomeMessageFragmentView
 
 
 class CourseHomeView(CourseTabView):
@@ -113,11 +112,9 @@ class CourseHomeFragmentView(EdxFragmentView):
         # Get the handouts
         handouts_html = get_course_info_section(request, request.user, course, 'handouts')
 
-        # Only show the reviews button if configured
-        show_reviews_link = CourseReviewsModuleFragmentView.is_configured()
-
         # Render the course home fragment
         context = {
+            'request': request,
             'csrf': csrf(request)['csrf_token'],
             'course': course,
             'course_key': course_key,
@@ -130,7 +127,6 @@ class CourseHomeFragmentView(EdxFragmentView):
             'course_sock_fragment': course_sock_fragment,
             'disable_courseware_js': True,
             'uses_pattern_library': True,
-            'show_reviews_link': show_reviews_link,
         }
         html = render_to_string('course_experience/course-home-fragment.html', context)
         return Fragment(html)
diff --git a/openedx/features/course_experience/views/course_updates.py b/openedx/features/course_experience/views/course_updates.py
index aca6719934..923584598a 100644
--- a/openedx/features/course_experience/views/course_updates.py
+++ b/openedx/features/course_experience/views/course_updates.py
@@ -53,12 +53,10 @@ class CourseUpdatesFragmentView(EdxFragmentView):
         course_url_name = default_course_url_name(request)
         course_url = reverse(course_url_name, kwargs={'course_id': unicode(course.id)})
 
-        # Fetch all of the updates individually
-        info_module = get_course_info_section_module(request, request.user, course, 'updates')
-        ordered_updates = self.order_updates(info_module.items)
-
-        # Older implementations and a few tests store a single html object representing all the updates
-        plain_html_updates = info_module.data
+        ordered_updates = self.get_ordered_updates(request, course)
+        plain_html_updates = ''
+        if ordered_updates:
+            plain_html_updates = self.get_plain_html_updates(request, course)
 
         # Render the course home fragment
         context = {
@@ -74,16 +72,33 @@ class CourseUpdatesFragmentView(EdxFragmentView):
         return Fragment(html)
 
     @classmethod
-    def order_updates(self, updates):
+    def get_ordered_updates(self, request, course):
         """
         Returns any course updates in reverse chronological order.
         """
-        sorted_updates = [update for update in updates if update.get('status') == self.STATUS_VISIBLE]
-        sorted_updates.sort(
+        info_module = get_course_info_section_module(request, request.user, course, 'updates')
+
+        updates = info_module.items if info_module else []
+        ordered_updates = [update for update in updates if update.get('status') == self.STATUS_VISIBLE]
+        ordered_updates.sort(
             key=lambda item: (self.safe_parse_date(item['date']), item['id']),
             reverse=True
         )
-        return sorted_updates
+        return ordered_updates
+
+    @classmethod
+    def has_updates(self, request, course):
+        return len(self.get_ordered_updates(request, course)) > 0
+
+    @classmethod
+    def get_plain_html_updates(self, request, course):
+        """
+        Returns any course updates in an html chunk. Used
+        for older implementations and a few tests that store
+        a single html object representing all the updates.
+        """
+        info_module = get_course_info_section_module(request, request.user, course, 'updates')
+        return info_module.data if info_module else ''
 
     @staticmethod
     def safe_parse_date(date):
diff --git a/openedx/features/course_experience/views/welcome_message.py b/openedx/features/course_experience/views/welcome_message.py
index 3f8340627d..bf4e6452dc 100644
--- a/openedx/features/course_experience/views/welcome_message.py
+++ b/openedx/features/course_experience/views/welcome_message.py
@@ -53,14 +53,11 @@ class WelcomeMessageFragmentView(EdxFragmentView):
         """
         Returns the course's welcome message or None if it doesn't have one.
         """
-        info_module = get_course_info_section_module(request, request.user, course, 'updates')
-        if not info_module:
-            return None
-
         # Return the course update with the most recent publish date
-        ordered_updates = CourseUpdatesFragmentView.order_updates(info_module.items)
+        ordered_updates = CourseUpdatesFragmentView.get_ordered_updates(request, course)
         content = None
         if ordered_updates:
+            info_module = get_course_info_section_module(request, request.user, course, 'updates')
             info_block = getattr(info_module, '_xmodule', info_module)
             content = info_block.system.replace_urls(ordered_updates[0]['content'])