Merge pull request #9745 from edx/andya/fix-duplicate-search-request

Fix duplicate calls to search API

common/static/common/js/components/collections/paging_collection.js

@@ -90,16 +90,20 @@
              */
             setPage: function (page) {
                 var oldPage = this.currentPage,
-                    self = this;
-                return this.goTo(page - (this.isZeroIndexed ? 1 : 0), {reset: true}).then(
+                    self = this,
+                    deferred = $.Deferred();
+                this.goTo(page - (this.isZeroIndexed ? 1 : 0), {reset: true}).then(
                     function () {
                         self.isStale = false;
                         self.trigger('page_changed');
+                        deferred.resolve();
                     },
                     function () {
                         self.currentPage = oldPage;
+                        deferred.fail();
                     }
                 );
+                return deferred.promise();
             },
 
 

common/test/acceptance/tests/helpers.py

@@ -334,7 +334,7 @@ class EventsTestMixin(TestCase):
                 captured_events.append(event)
 
     @contextmanager
-    def assert_events_match_during(self, event_filter=None, expected_events=None):
+    def assert_events_match_during(self, event_filter=None, expected_events=None, in_order=True):
         """
         Context manager that ensures that events matching the `event_filter` and `expected_events` are emitted.
 
@@ -351,7 +351,7 @@ class EventsTestMixin(TestCase):
         with self.capture_events(event_filter, len(expected_events), captured_events):
             yield
 
-        self.assert_events_match(expected_events, captured_events)
+        self.assert_events_match(expected_events, captured_events, in_order=in_order)
 
     def wait_for_events(self, start_time=None, event_filter=None, number_of_matches=1, timeout=None):
         """
@@ -477,17 +477,29 @@ class EventsTestMixin(TestCase):
 
         self.assertEquals(len(matching_events), 0, description)
 
-    def assert_events_match(self, expected_events, actual_events):
-        """
-        Assert that each item in the expected events sequence matches its counterpart at the same index in the actual
-        events sequence.
+    def assert_events_match(self, expected_events, actual_events, in_order=True):
+        """Assert that each actual event matches one of the expected events.
+
+        Args:
+            expected_events (List): a list of dicts representing the expected events.
+            actual_events (List): a list of dicts that were actually recorded.
+            in_order (bool): if True then the events must be in the same order (defaults to True).
         """
-        for expected_event, actual_event in zip(expected_events, actual_events):
-            assert_event_matches(
-                expected_event,
-                actual_event,
-                tolerate=EventMatchTolerates.lenient()
-            )
+        if in_order:
+            for expected_event, actual_event in zip(expected_events, actual_events):
+                assert_event_matches(
+                    expected_event,
+                    actual_event,
+                    tolerate=EventMatchTolerates.lenient()
+                )
+        else:
+            for expected_event in expected_events:
+                actual_event = next(event for event in actual_events if is_matching_event(expected_event, event))
+                assert_event_matches(
+                    expected_event,
+                    actual_event or {},
+                    tolerate=EventMatchTolerates.lenient()
+                )
 
     def relative_path_to_absolute_uri(self, relative_path):
         """Return an aboslute URI given a relative path taking into account the test context."""

common/test/acceptance/tests/lms/test_teams.py

@@ -9,7 +9,6 @@ from dateutil.parser import parse
 import ddt
 from nose.plugins.attrib import attr
 from uuid import uuid4
-from unittest import skip
 
 from ..helpers import EventsTestMixin, UniqueCourseTest
 from ...fixtures import LMS_BASE_URL
@@ -783,7 +782,6 @@ class BrowseTeamsWithinTopicTest(TeamsTabBase):
         self.browse_teams_page.click_browse_all_teams_link()
         self.assertTrue(self.topics_page.is_browser_on_page())
 
-    @skip("Skip until TNL-3198 (searching teams makes two AJAX requests) is resolved")
     def test_search(self):
         """
         Scenario: User should be able to search for a team
@@ -794,6 +792,7 @@ class BrowseTeamsWithinTopicTest(TeamsTabBase):
         And the search header should be shown
         And 0 results should be shown
         And my browser should fire a page viewed event for the search page
+        And a searched event should have been fired
         """
         # Note: all searches will return 0 results with the mock search server
         # used by Bok Choy.
@@ -801,21 +800,21 @@ class BrowseTeamsWithinTopicTest(TeamsTabBase):
         self.create_teams(self.topic, 5)
         self.browse_teams_page.visit()
         events = [{
-            'event_type': 'edx.team.searched',
+            'event_type': 'edx.team.page_viewed',
             'event': {
-                'search_text': search_text,
+                'page_name': 'search-teams',
                 'topic_id': self.topic['id'],
-                'number_of_results': 0
+                'team_id': None
             }
         }, {
-            'event_type': 'edx.team.page_viewed',
+            'event_type': 'edx.team.searched',
             'event': {
-                'page_name': 'search-teams',
+                'search_text': search_text,
                 'topic_id': self.topic['id'],
-                'team_id': None
+                'number_of_results': 0
             }
         }]
-        with self.assert_events_match_during(self.only_team_events, expected_events=events):
+        with self.assert_events_match_during(self.only_team_events, expected_events=events, in_order=False):
             search_results_page = self.browse_teams_page.search(search_text)
         self.verify_search_header(search_results_page, search_text)
         self.assertTrue(search_results_page.get_pagination_header_text().startswith('Showing 0 out of 0 total'))

lms/djangoapps/teams/static/teams/js/spec/views/teams_spec.js

@@ -24,7 +24,9 @@ define([
         it('can render itself', function () {
             var testTeamData = TeamSpecHelpers.createMockTeamData(1, 5),
                 teamsView = createTeamsView({
-                    teams: TeamSpecHelpers.createMockTeams(testTeamData)
+                    teams: TeamSpecHelpers.createMockTeams({
+                        results: testTeamData
+                    })
                 });
 
             expect(teamsView.$('.teams-paging-header').text()).toMatch('Showing 1-5 out of 6 total');

lms/djangoapps/teams/static/teams/js/spec/views/teams_tab_spec.js

@@ -29,19 +29,6 @@ define([
             return teamsTabView;
         };
 
-        /**
-         * Filters out all team events from a list of requests.
-         */
-        var removeTeamEvents = function (requests) {
-            return requests.filter(function (request) {
-                if (request.requestBody && request.requestBody.startsWith('event_type=edx.team')) {
-                    return false;
-                } else {
-                    return true;
-                }
-            });
-        };
-
         beforeEach(function () {
             setFixtures('<div class="teams-content"></div>');
             spyOn($.fn, 'focus');
@@ -233,25 +220,33 @@ define([
                         options
                     ));
             };
+            var performSearch = function(requests, teamsTabView) {
+                teamsTabView.$('.search-field').val('foo');
+                teamsTabView.$('.action-search').click();
+                verifyTeamsRequest(requests, {
+                    order_by: '',
+                    text_search: 'foo'
+                });
+                AjaxHelpers.respondWithJson(requests, TeamSpecHelpers.createMockTeamsResponse({results: []}));
+            };
 
             it('can search teams', function () {
                 var requests = AjaxHelpers.requests(this),
-                    teamsTabView = createTeamsTabView();
+                    teamsTabView = createTeamsTabView(),
+                    requestCountBeforeSearch;
                 teamsTabView.browseTopic(TeamSpecHelpers.testTopicID);
                 verifyTeamsRequest(requests, {
                     order_by: 'last_activity_at',
                     text_search: ''
                 });
                 AjaxHelpers.respondWithJson(requests, {});
-                teamsTabView.$('.search-field').val('foo');
-                teamsTabView.$('.action-search').click();
-                verifyTeamsRequest(requests, {
-                    order_by: '',
-                    text_search: 'foo'
-                });
-                AjaxHelpers.respondWithJson(requests, {});
+                requestCountBeforeSearch = requests.length;
+                performSearch(requests, teamsTabView);
                 expect(teamsTabView.$('.page-title').text()).toBe('Team Search');
                 expect(teamsTabView.$('.page-description').text()).toBe('Showing results for "foo"');
+
+                // Expect exactly one search request to be fired
+                expect(requests.length).toBe(requestCountBeforeSearch + 1);
             });
 
             it('can clear a search', function () {
@@ -259,17 +254,10 @@ define([
                     teamsTabView = createTeamsTabView();
                 teamsTabView.browseTopic(TeamSpecHelpers.testTopicID);
                 AjaxHelpers.respondWithJson(requests, {});
+                performSearch(requests, teamsTabView);
 
                 // Perform a search
-                teamsTabView.$('.search-field').val('foo');
-                teamsTabView.$('.action-search').click();
-                // Note: this is a bit of a hack -- without it the URL
-                // fragment won't be what it would be in the real
-                // app. This line sets the fragment without triggering
-                // callbacks, allowing teams_tab.js to detect the
-                // fragment correctly.
-                Backbone.history.navigate('topics/' + TeamSpecHelpers.testTopicID + '/search', {trigger: false});
-                AjaxHelpers.respondWithJson(requests, {});
+                performSearch(requests, teamsTabView);
 
                 // Clear the search and submit it again
                 teamsTabView.$('.search-field').val('');
@@ -283,16 +271,39 @@ define([
                 expect(teamsTabView.$('.page-description').text()).toBe('Test description 1');
             });
 
-            it('does not switch to showing results when the search returns an error', function () {
+            it('can navigate back to all teams from a search', function () {
                 var requests = AjaxHelpers.requests(this),
                     teamsTabView = createTeamsTabView();
                 teamsTabView.browseTopic(TeamSpecHelpers.testTopicID);
                 AjaxHelpers.respondWithJson(requests, {});
 
                 // Perform a search
+                performSearch(requests, teamsTabView);
+
+                // Verify the breadcrumbs have a link back to the teams list, and click on it
+                expect(teamsTabView.$('.breadcrumbs a').length).toBe(2);
+                teamsTabView.$('.breadcrumbs a').last().click();
+                verifyTeamsRequest(requests, {
+                    order_by: 'last_activity_at',
+                    text_search: ''
+                });
+                AjaxHelpers.respondWithJson(requests, {});
+                expect(teamsTabView.$('.page-title').text()).toBe('Test Topic 1');
+                expect(teamsTabView.$('.page-description').text()).toBe('Test description 1');
+            });
+
+            it('does not switch to showing results when the search returns an error', function () {
+                var requests = AjaxHelpers.requests(this),
+                    teamsTabView = createTeamsTabView();
+                teamsTabView.browseTopic(TeamSpecHelpers.testTopicID);
+                AjaxHelpers.respondWithJson(requests, {});
+
+                // Perform a search but respond with a 500
                 teamsTabView.$('.search-field').val('foo');
                 teamsTabView.$('.action-search').click();
                 AjaxHelpers.respondWithError(requests);
+
+                // Verify that the team list is still shown
                 expect(teamsTabView.$('.page-title').text()).toBe('Test Topic 1');
                 expect(teamsTabView.$('.page-description').text()).toBe('Test description 1');
                 expect(teamsTabView.$('.search-field').val(), 'foo');

lms/djangoapps/teams/static/teams/js/spec/views/topic_teams_spec.js

@@ -44,7 +44,9 @@ define([
         it('can render itself', function () {
             var testTeamData = TeamSpecHelpers.createMockTeamData(1, 5),
                 teamsView = createTopicTeamsView({
-                    teams: TeamSpecHelpers.createMockTeams(testTeamData),
+                    teams: TeamSpecHelpers.createMockTeams({
+                        results: testTeamData
+                    }),
                     teamMemberships: TeamSpecHelpers.createMockTeamMemberships([])
                 });
 

lms/djangoapps/teams/static/teams/js/spec_helpers/team_spec_helpers.js

@@ -43,18 +43,22 @@ define([
         });
     };
 
-    var createMockTeams = function(teamData) {
-        if (!teamData) {
-            teamData = createMockTeamData(1, 5);
-        }
-        return new TeamCollection(
+    var createMockTeamsResponse = function(options) {
+        return _.extend(
             {
                 count: 6,
                 num_pages: 2,
                 current_page: 1,
                 start: 0,
-                results: teamData
+                results: createMockTeamData(1, 5)
             },
+            options
+        );
+    };
+
+    var createMockTeams = function(options) {
+        return new TeamCollection(
+            createMockTeamsResponse(options),
             {
                 teamEvents: teamEvents,
                 course_id: testCourseID,
@@ -325,6 +329,7 @@ define([
         testTeamDiscussionID: testTeamDiscussionID,
         testContext: testContext,
         createMockTeamData: createMockTeamData,
+        createMockTeamsResponse: createMockTeamsResponse,
         createMockTeams: createMockTeams,
         createMockTeamMembershipsData: createMockTeamMembershipsData,
         createMockTeamMemberships: createMockTeamMemberships,

lms/djangoapps/teams/static/teams/js/views/teams_tab.js

@@ -214,6 +214,7 @@
                             view.mainView = view.createTeamsListView({
                                 topic: topic,
                                 collection: view.teamsCollection,
+                                breadcrumbs: view.createBreadcrumbs(topic),
                                 title: gettext('Team Search'),
                                 description: interpolate(
                                     gettext('Showing results for "%(searchString)s"'),
@@ -337,6 +338,7 @@
                                         var teamsView = view.createTeamsListView({
                                             topic: topic,
                                             collection: collection,
+                                            breadcrumbs: view.createBreadcrumbs(),
                                             showSortControls: true
                                         });
                                         deferred.resolve(teamsView);
@@ -368,7 +370,7 @@
                             headerActionsView: searchFieldView,
                             title: options.title,
                             description: options.description,
-                            breadcrumbs: this.createBreadcrumbs()
+                            breadcrumbs: options.breadcrumbs
                         }),
                         searchUrl = 'topics/' + topic.get('id') + '/search';
                     // Listen to requests to sync the collection and redirect it as follows:
@@ -378,6 +380,11 @@
                     // 3. Otherwise, do nothing and remain on the current page.
                     // Note: Backbone makes this a no-op if redirecting to the current page.
                     this.listenTo(collection, 'sync', function() {
+                        // Clear the stale flag here as by definition the collection is up-to-date,
+                        // and the flag itself isn't guaranteed to be cleared yet. This is to ensure
+                        // that the collection doesn't unnecessarily get refreshed again.
+                        collection.isStale = false;
+
                         if (collection.searchString) {
                             Backbone.history.navigate(searchUrl, {trigger: true});
                         } else if (Backbone.history.getFragment() === searchUrl) {