From 6761a87bfb419533046377262bca39bafac849b8 Mon Sep 17 00:00:00 2001 From: Harry Rein Date: Tue, 30 May 2017 15:10:55 -0400 Subject: [PATCH] Fix for LEARNER-859: Updating styling on the course updates page to more clearly differentiate between multiple updates. Specifically: - Updated styling on date in the top left corner - Added horizontal line between updates - Removed ability to toggle updates on and off - Cleaned up code to always show all updates: --- common/lib/xmodule/xmodule/html_module.py | 15 ++++--- .../xmodule/xmodule/tests/test_html_module.py | 12 +++--- .../mobile_api/course_info/tests.py | 6 +-- .../sass/features/_course-experience.scss | 21 ++++++++++ .../course-updates-fragment.html | 17 +++++++- .../tests/views/test_course_updates.py | 8 ++-- .../course_experience/views/course_updates.py | 40 +++++++++++++++++-- .../views/welcome_message.py | 5 ++- 8 files changed, 97 insertions(+), 27 deletions(-) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index fce51db3fc..02063444c0 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -447,23 +447,26 @@ class CourseInfoModule(CourseInfoFields, HtmlModuleMixin): return self.data.replace("%%USER_ID%%", self.system.anonymous_student_id) return self.data else: - course_updates = self.ordered_updates() + # This should no longer be called on production now that we are using a separate updates page + # and using a fragment HTML file - it will be called in tests until those are removed. + course_updates = self.order_updates(self.items) context = { 'visible_updates': course_updates[:3], 'hidden_updates': course_updates[3:], } return self.system.render_template("{0}/course_updates.html".format(self.TEMPLATE_DIR), context) - def ordered_updates(self): + @classmethod + def order_updates(self, updates): """ Returns any course updates in reverse chronological order. """ - course_updates = [item for item in self.items if item.get('status') == self.STATUS_VISIBLE] - course_updates.sort( - key=lambda item: (CourseInfoModule.safe_parse_date(item['date']), item['id']), + sorted_updates = [update for update in updates if update.get('status') == self.STATUS_VISIBLE] + sorted_updates.sort( + key=lambda item: (self.safe_parse_date(item['date']), item['id']), reverse=True ) - return course_updates + return sorted_updates @staticmethod def safe_parse_date(date): diff --git a/common/lib/xmodule/xmodule/tests/test_html_module.py b/common/lib/xmodule/xmodule/tests/test_html_module.py index 0f3992af3f..2b272487ff 100644 --- a/common/lib/xmodule/xmodule/tests/test_html_module.py +++ b/common/lib/xmodule/xmodule/tests/test_html_module.py @@ -1,14 +1,14 @@ import unittest from mock import Mock - -from xblock.field_data import DictFieldData -from xmodule.html_module import HtmlModule, HtmlDescriptor, CourseInfoModule - -from . import get_test_system, get_test_descriptor_system from opaque_keys.edx.locations import SlashSeparatedCourseKey +from xblock.field_data import DictFieldData from xblock.fields import ScopeIds +from xmodule.html_module import CourseInfoModule, HtmlDescriptor, HtmlModule + +from . import get_test_descriptor_system, get_test_system + def instantiate_descriptor(**field_data): """ @@ -148,7 +148,7 @@ class HtmlDescriptorIndexingTestCase(unittest.TestCase): class CourseInfoModuleTestCase(unittest.TestCase): """ - Make sure that CourseInfoModule renders updates properly + Make sure that CourseInfoModule renders updates properly. """ def test_updates_render(self): """ diff --git a/lms/djangoapps/mobile_api/course_info/tests.py b/lms/djangoapps/mobile_api/course_info/tests.py index 39b44e91c9..0ae5aa52cd 100644 --- a/lms/djangoapps/mobile_api/course_info/tests.py +++ b/lms/djangoapps/mobile_api/course_info/tests.py @@ -4,17 +4,15 @@ Tests for course_info import ddt from django.conf import settings +from milestones.tests.utils import MilestonesTestCaseMixin from nose.plugins.attrib import attr from xmodule.html_module import CourseInfoModule from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml_importer import import_course_from_xml -from milestones.tests.utils import MilestonesTestCaseMixin -from ..testutils import ( - MobileAPITestCase, MobileCourseAccessTestMixin, MobileAuthTestMixin -) +from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin @attr(shard=3) diff --git a/lms/static/sass/features/_course-experience.scss b/lms/static/sass/features/_course-experience.scss index 0cd18b1341..6920862c58 100644 --- a/lms/static/sass/features/_course-experience.scss +++ b/lms/static/sass/features/_course-experience.scss @@ -171,3 +171,24 @@ } } } + +// Course Updates Page +.course-updates { + .all-updates { + .updates-article { + margin: ($baseline*6/5) 0; + padding-bottom: ($baseline*6/5); + border-bottom: 1px solid $lms-border-color; + .date { + font-size: font-size(small); + font-weight: 300; + float: none; + padding-bottom: ($baseline/4); + } + &:last-child { + border-bottom: 0; + } + } + } +} + 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 17b0c284b2..c4344d5c57 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 @@ -28,7 +28,22 @@ from openedx.core.djangolib.markup import HTML
- ${HTML(updates_html)} + % if len(plain_html_updates) > 0: + ${HTML(plain_html_updates)} + % else: +
+ % for index, update in enumerate(updates): +
+ % if not update.get("is_error"): +
${update.get("date")}
+ % endif +
+ ${HTML(update.get("content"))} +
+
+ % endfor +
+ % endif
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 b6471b274c..01db7926fb 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -3,10 +3,10 @@ Tests for the course updates page. """ from django.core.urlresolvers import reverse -from courseware.courses import get_course_info_section_module, get_course_info_usage_key +from courseware.courses import get_course_info_usage_key from student.models import CourseEnrollment from student.tests.factories import UserFactory -from xmodule.html_module import CourseInfoModule +from openedx.features.course_experience.views.course_updates import CourseUpdatesFragmentView from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -41,7 +41,7 @@ def create_course_update(course, user, content, date='December 31, 1999'): "id": len(course_updates.items) + 1, "date": date, "content": content, - "status": CourseInfoModule.STATUS_VISIBLE + "status": CourseUpdatesFragmentView.STATUS_VISIBLE }) modulestore().update_item(course_updates, user.id) @@ -124,7 +124,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): course_updates_url(self.course) # Fetch the view and verify that the query counts haven't changed - with self.assertNumQueries(33): + with self.assertNumQueries(32): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/views/course_updates.py b/openedx/features/course_experience/views/course_updates.py index a321ceb0a3..5379eaf284 100644 --- a/openedx/features/course_experience/views/course_updates.py +++ b/openedx/features/course_experience/views/course_updates.py @@ -1,6 +1,7 @@ """ Views that handle course updates. """ +from datetime import datetime from django.contrib.auth.decorators import login_required from django.core.context_processors import csrf @@ -11,7 +12,7 @@ from django.views.decorators.cache import cache_control from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment -from courseware.courses import get_course_info_section, get_course_with_access +from courseware.courses import get_course_info_section_module, get_course_with_access from lms.djangoapps.courseware.views.views import CourseTabView from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.features.course_experience import default_course_url_name @@ -21,6 +22,7 @@ class CourseUpdatesView(CourseTabView): """ The course updates page. """ + @method_decorator(login_required) @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True)) def get(self, request, course_id, **kwargs): @@ -39,6 +41,9 @@ class CourseUpdatesFragmentView(EdxFragmentView): """ A fragment to render the home page for a course. """ + STATUS_VISIBLE = 'visible' + STATUS_DELETED = 'deleted' + def render_to_fragment(self, request, course_id=None, **kwargs): """ Renders the course's home page as a fragment. @@ -48,17 +53,44 @@ class CourseUpdatesFragmentView(EdxFragmentView): course_url_name = default_course_url_name(request) course_url = reverse(course_url_name, kwargs={'course_id': unicode(course.id)}) - # Fetch the updates as HTML - updates_html = get_course_info_section(request, request.user, course, 'updates') + # 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 # Render the course home fragment context = { 'csrf': csrf(request)['csrf_token'], 'course': course, 'course_url': course_url, - 'updates_html': updates_html, + 'updates': ordered_updates, + 'plain_html_updates': plain_html_updates, 'disable_courseware_js': True, 'uses_pattern_library': True, } html = render_to_string('course_experience/course-updates-fragment.html', context) return Fragment(html) + + @classmethod + def order_updates(self, updates): + """ + 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( + key=lambda item: (self.safe_parse_date(item['date']), item['id']), + reverse=True + ) + return sorted_updates + + @staticmethod + def safe_parse_date(date): + """ + Since this is used solely for ordering purposes, use today's date as a default + """ + try: + return datetime.strptime(date, '%B %d, %Y') + except ValueError: # occurs for ill-formatted date values + return datetime.today() diff --git a/openedx/features/course_experience/views/welcome_message.py b/openedx/features/course_experience/views/welcome_message.py index 71215f91a3..aa571a04ca 100644 --- a/openedx/features/course_experience/views/welcome_message.py +++ b/openedx/features/course_experience/views/welcome_message.py @@ -6,6 +6,7 @@ from django.template.loader import render_to_string from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment +from course_updates import CourseUpdatesFragmentView from courseware.courses import get_course_info_section_module, get_course_with_access from openedx.core.djangoapps.plugin_api.views import EdxFragmentView @@ -43,10 +44,10 @@ class WelcomeMessageFragmentView(EdxFragmentView): return None # Return the course update with the most recent publish date - info_block = getattr(info_module, '_xmodule', info_module) - ordered_updates = info_block.ordered_updates() + ordered_updates = CourseUpdatesFragmentView.order_updates(info_module.items) content = None if ordered_updates: + info_block = getattr(info_module, '_xmodule', info_module) content = info_block.system.replace_urls(ordered_updates[0]['content']) return content