From e6871af2d3959f649d7285f19f92e407be7f76e1 Mon Sep 17 00:00:00 2001 From: jsa Date: Mon, 16 Jun 2014 15:56:40 -0400 Subject: [PATCH] forums: add UX for username search results. JIRA: FOR-627 --- .../discussion_thread_list_view_spec.coffee | 183 ++++++++++++------ .../static/coffee/src/discussion/utils.coffee | 1 + .../views/discussion_thread_list_view.coffee | 27 +++ .../test/acceptance/pages/lms/discussion.py | 7 +- .../test/acceptance/tests/test_discussion.py | 34 +++- .../discussion/_discussion-developer.scss | 5 +- 6 files changed, 193 insertions(+), 64 deletions(-) diff --git a/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee b/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee index f2dd11b8cd..7c7e82abc4 100644 --- a/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee +++ b/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee @@ -84,63 +84,6 @@ describe "DiscussionThreadListView", -> @view = new DiscussionThreadListView({collection: @discussion, el: $(".sidebar")}) @view.render() - testAlertMessages = (expectedMessages) -> - expect($(".search-alert .message").map( -> - $(@).html() - ).get()).toEqual(expectedMessages) - - it "renders and removes search alerts", -> - testAlertMessages [] - foo = @view.addSearchAlert("foo") - testAlertMessages ["foo"] - bar = @view.addSearchAlert("bar") - testAlertMessages ["foo", "bar"] - @view.removeSearchAlert(foo.cid) - testAlertMessages ["bar"] - @view.removeSearchAlert(bar.cid) - testAlertMessages [] - - it "clears all search alerts", -> - @view.addSearchAlert("foo") - @view.addSearchAlert("bar") - @view.addSearchAlert("baz") - testAlertMessages ["foo", "bar", "baz"] - @view.clearSearchAlerts() - testAlertMessages [] - - testCorrection = (view, correctedText) -> - spyOn(view, "addSearchAlert") - $.ajax.andCallFake( - (params) => - params.success( - {discussion_data: [], page: 42, num_pages: 99, corrected_text: correctedText}, 'success' - ) - {always: ->} - ) - view.searchFor("dummy") - expect($.ajax).toHaveBeenCalled() - - it "adds a search alert when an alternate term was searched", -> - testCorrection(@view, "foo") - expect(@view.addSearchAlert).toHaveBeenCalled() - expect(@view.addSearchAlert.mostRecentCall.args[0]).toMatch(/foo/) - - it "does not add a search alert when no alternate term was searched", -> - testCorrection(@view, null) - expect(@view.addSearchAlert).not.toHaveBeenCalled() - - it "clears search alerts when a new search is performed", -> - spyOn(@view, "clearSearchAlerts") - spyOn(DiscussionUtil, "safeAjax") - @view.searchFor("dummy") - expect(@view.clearSearchAlerts).toHaveBeenCalled() - - it "clears search alerts when the underlying collection changes", -> - spyOn(@view, "clearSearchAlerts") - spyOn(@view, "renderThread") - @view.collection.trigger("change", new Thread({id: 1})) - expect(@view.clearSearchAlerts).toHaveBeenCalled() - makeView = (discussion) -> return new DiscussionThreadListView( el: $(".sidebar"), @@ -202,3 +145,129 @@ describe "DiscussionThreadListView", -> it "with sort preference comments", -> changeSorting(@threads, "votes", "comments", ["Thread3", "Thread2", "Thread1"]) + + describe "search alerts", -> + + testAlertMessages = (expectedMessages) -> + expect($(".search-alert .message").map( -> + $(@).html() + ).get()).toEqual(expectedMessages) + + it "renders and removes search alerts", -> + testAlertMessages [] + foo = @view.addSearchAlert("foo") + testAlertMessages ["foo"] + bar = @view.addSearchAlert("bar") + testAlertMessages ["foo", "bar"] + @view.removeSearchAlert(foo.cid) + testAlertMessages ["bar"] + @view.removeSearchAlert(bar.cid) + testAlertMessages [] + + it "clears all search alerts", -> + @view.addSearchAlert("foo") + @view.addSearchAlert("bar") + @view.addSearchAlert("baz") + testAlertMessages ["foo", "bar", "baz"] + @view.clearSearchAlerts() + testAlertMessages [] + + describe "search spell correction", -> + + beforeEach -> + spyOn(@view, "searchForUser") + + testCorrection = (view, correctedText) -> + spyOn(view, "addSearchAlert") + $.ajax.andCallFake( + (params) => + params.success( + {discussion_data: [], page: 42, num_pages: 99, corrected_text: correctedText}, 'success' + ) + {always: ->} + ) + view.searchFor("dummy") + expect($.ajax).toHaveBeenCalled() + + it "adds a search alert when an alternate term was searched", -> + testCorrection(@view, "foo") + expect(@view.addSearchAlert.callCount).toEqual(1) + expect(@view.addSearchAlert.mostRecentCall.args[0]).toMatch(/foo/) + + it "does not add a search alert when no alternate term was searched", -> + testCorrection(@view, null) + expect(@view.addSearchAlert.callCount).toEqual(1) + expect(@view.addSearchAlert.mostRecentCall.args[0]).toMatch(/no threads matched/i) + + it "clears search alerts when a new search is performed", -> + spyOn(@view, "clearSearchAlerts") + spyOn(DiscussionUtil, "safeAjax") + @view.searchFor("dummy") + expect(@view.clearSearchAlerts).toHaveBeenCalled() + + it "clears search alerts when the underlying collection changes", -> + spyOn(@view, "clearSearchAlerts") + spyOn(@view, "renderThread") + @view.collection.trigger("change", new Thread({id: 1})) + expect(@view.clearSearchAlerts).toHaveBeenCalled() + + describe "username search", -> + + it "makes correct ajax calls", -> + $.ajax.andCallFake( + (params) => + expect(params.data.username).toEqual("testing-username") + expect(params.url.path()).toEqual(DiscussionUtil.urlFor("users")) + params.success( + {users: []}, 'success' + ) + {always: ->} + ) + @view.searchForUser("testing-username") + expect($.ajax).toHaveBeenCalled() + + setAjaxResults = (threadSuccess, userResult) -> + # threadSuccess is a boolean indicating whether the thread search ajax call should succeed + # userResult is the value that should be returned as data from the username search ajax call + $.ajax.andCallFake( + (params) => + if params.data.text and threadSuccess + params.success( + {discussion_data: [], page: 42, num_pages: 99, corrected_text: "dummy"}, + "success" + ) + else if params.data.username + params.success( + {users: userResult}, + "success" + ) + {always: ->} + ) + + it "gets called after a thread search succeeds", -> + spyOn(@view, "searchForUser").andCallThrough() + setAjaxResults(true, []) + @view.searchFor("gizmo") + expect(@view.searchForUser).toHaveBeenCalled() + expect($.ajax.mostRecentCall.args[0].data.username).toEqual("gizmo") + + it "does not get called after a thread search fails", -> + spyOn(@view, "searchForUser").andCallThrough() + setAjaxResults(false, []) + @view.searchFor("gizmo") + expect(@view.searchForUser).not.toHaveBeenCalled() + + it "adds a search alert when an username was matched", -> + spyOn(@view, "addSearchAlert") + setAjaxResults(true, [{username: "gizmo", id: "1"}]) + @view.searchForUser("dummy") + expect($.ajax).toHaveBeenCalled() + expect(@view.addSearchAlert).toHaveBeenCalled() + expect(@view.addSearchAlert.mostRecentCall.args[0]).toMatch(/gizmo/) + + it "does not add a search alert when no username was matched", -> + spyOn(@view, "addSearchAlert") + setAjaxResults(true, []) + @view.searchForUser("dummy") + expect($.ajax).toHaveBeenCalled() + expect(@view.addSearchAlert).not.toHaveBeenCalled() diff --git a/common/static/coffee/src/discussion/utils.coffee b/common/static/coffee/src/discussion/utils.coffee index 6346d1230a..e634c79975 100644 --- a/common/static/coffee/src/discussion/utils.coffee +++ b/common/static/coffee/src/discussion/utils.coffee @@ -73,6 +73,7 @@ class @DiscussionUtil downvote_comment : "/courses/#{$$course_id}/discussion/comments/#{param}/downvote" undo_vote_for_comment : "/courses/#{$$course_id}/discussion/comments/#{param}/unvote" upload : "/courses/#{$$course_id}/discussion/upload" + users : "/courses/#{$$course_id}/discussion/users" search : "/courses/#{$$course_id}/discussion/forum/search" retrieve_discussion : "/courses/#{$$course_id}/discussion/forum/#{param}/inline" retrieve_single_thread : "/courses/#{$$course_id}/discussion/forum/#{param}/threads/#{param1}" diff --git a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index 96a6f2a419..4a0a2ccc06 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee @@ -445,6 +445,7 @@ if Backbone? data: { text: text } url: url type: "GET" + dataType: 'json' $loading: $ loadingCallback: => @$(".post-list").html('
  • ' + gettext('Loading thread list') + '
    • ') @@ -465,10 +466,36 @@ if Backbone? true ) @addSearchAlert(message) + else if response.discussion_data.length == 0 + @addSearchAlert(gettext('No threads matched your query.')) # TODO: Perhaps reload user info so that votes can be updated. # In the future we might not load all of a user's votes at once # so this would probably be necessary anyway @displayedCollection.reset(@collection.models) # Don't think this is necessary + @searchForUser(text) if text + + + searchForUser: (text) -> + DiscussionUtil.safeAjax + data: { username: text } + url: DiscussionUtil.urlFor("users") + type: "GET" + dataType: 'json' + error: => + return + success: (response) => + if response.users.length > 0 + message = interpolate( + _.escape(gettext('Show posts by %(username)s.')), + {"username": + _.template('<%- username %>', { + url: DiscussionUtil.urlFor("user_profile", response.users[0].id), + username: response.users[0].username + }) + }, + true + ) + @addSearchAlert(message) clearSearch: (callback, value) -> @$(".post-search-field").val("") diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index 91886c0c29..431784716c 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/common/test/acceptance/pages/lms/discussion.py @@ -351,13 +351,13 @@ class DiscussionTabHomePage(CoursePage, DiscussionPageMixin): def is_browser_on_page(self): return self.q(css=".discussion-body section.home-header").present - def perform_search(self): + def perform_search(self, text="dummy"): self.q(css=".discussion-body .sidebar .search").first.click() EmptyPromise( lambda: self.q(css=".discussion-body .sidebar .search.is-open").present, "waiting for search input to be available" ).fulfill() - self.q(css="#search-discussions").fill("dummy" + chr(10)) + self.q(css="#search-discussions").fill(text + chr(10)) EmptyPromise( self.is_ajax_finished, "waiting for server to return result" @@ -366,6 +366,9 @@ class DiscussionTabHomePage(CoursePage, DiscussionPageMixin): def get_search_alert_messages(self): return self.q(css=self.ALERT_SELECTOR + " .message").text + def get_search_alert_links(self): + return self.q(css=self.ALERT_SELECTOR + " .link-jump") + def dismiss_alert_message(self, text): """ dismiss any search alert message containing the specified text. diff --git a/common/test/acceptance/tests/test_discussion.py b/common/test/acceptance/tests/test_discussion.py index d518a63e4b..d7e7be8c29 100644 --- a/common/test/acceptance/tests/test_discussion.py +++ b/common/test/acceptance/tests/test_discussion.py @@ -421,9 +421,18 @@ class DiscussionSearchAlertTest(UniqueCourseTest): Tests for spawning and dismissing alerts related to user search actions and their results. """ + SEARCHED_USERNAME = "gizmo" + def setUp(self): super(DiscussionSearchAlertTest, self).setUp() CourseFixture(**self.course_info).install() + # first auto auth call sets up a user that we will search for in some tests + self.searched_user_id = AutoAuthPage( + self.browser, + username=self.SEARCHED_USERNAME, + course_id=self.course_id + ).visit().get_user_id() + # this auto auth call creates the actual session user AutoAuthPage(self.browser, course_id=self.course_id).visit() self.page = DiscussionTabHomePage(self.browser, self.course_id) self.page.visit() @@ -433,12 +442,12 @@ class DiscussionSearchAlertTest(UniqueCourseTest): def check_search_alert_messages(self, expected): actual = self.page.get_search_alert_messages() - self.assertTrue(all(map(lambda msg, sub: msg.find(sub) >= 0, actual, expected))) + self.assertTrue(all(map(lambda msg, sub: msg.lower().find(sub.lower()) >= 0, actual, expected))) def test_no_rewrite(self): self.setup_corrected_text(None) self.page.perform_search() - self.check_search_alert_messages([]) + self.check_search_alert_messages(["no threads"]) def test_rewrite_dismiss(self): self.setup_corrected_text("foo") @@ -458,7 +467,26 @@ class DiscussionSearchAlertTest(UniqueCourseTest): self.setup_corrected_text(None) self.page.perform_search() - self.check_search_alert_messages([]) + self.check_search_alert_messages(["no threads"]) + + def test_rewrite_and_user(self): + self.setup_corrected_text("foo") + self.page.perform_search(self.SEARCHED_USERNAME) + self.check_search_alert_messages(["foo", self.SEARCHED_USERNAME]) + + def test_user_only(self): + self.setup_corrected_text(None) + self.page.perform_search(self.SEARCHED_USERNAME) + self.check_search_alert_messages(["no threads", self.SEARCHED_USERNAME]) + # make sure clicking the link leads to the user profile page + UserProfileViewFixture([]).push() + self.page.get_search_alert_links().first.click() + DiscussionUserProfilePage( + self.browser, + self.course_id, + self.searched_user_id, + self.SEARCHED_USERNAME + ).wait_for_page() class DiscussionSortPreferenceTest(UniqueCourseTest): diff --git a/lms/static/sass/discussion/_discussion-developer.scss b/lms/static/sass/discussion/_discussion-developer.scss index ac2e16dd3d..8340a3a08f 100644 --- a/lms/static/sass/discussion/_discussion-developer.scss +++ b/lms/static/sass/discussion/_discussion-developer.scss @@ -70,11 +70,12 @@ body.discussion { @include transition(none); @extend %t-weight5; padding: ($baseline/4) ($baseline/2); + color: $white; // reseting poorly globally scoped hover/focus state for this control &:hover, &:focus { - color: $link-color; - text-decoration: underline; + color: $white; + text-decoration: none; } } }