From 44abf7a9a2f38f6260c6bd28f2598f3f9d9b5206 Mon Sep 17 00:00:00 2001
From: Nimisha Asthagiri <nasthagiri@edx.org>
Date: Wed, 23 Dec 2015 12:17:34 -0500
Subject: [PATCH] Performance enhancement in see_exists: reverse order of
 checks

---
 lms/djangoapps/courseware/access.py           |  2 +-
 .../courseware/tests/test_access.py           | 48 +++++++++++++++----
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py
index 66cf45e928..8af5c1d208 100644
--- a/lms/djangoapps/courseware/access.py
+++ b/lms/djangoapps/courseware/access.py
@@ -318,7 +318,7 @@ def _has_access_course(user, action, courselike):
         Can see if can enroll, but also if can load it: if user enrolled in a course and now
         it's past the enrollment period, they should still see it.
         """
-        return ACCESS_GRANTED if (can_enroll() or can_load()) else ACCESS_DENIED
+        return ACCESS_GRANTED if (can_load() or can_enroll()) else ACCESS_DENIED
 
     def can_see_in_catalog():
         """
diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py
index d9b222d69b..3667369f7a 100644
--- a/lms/djangoapps/courseware/tests/test_access.py
+++ b/lms/djangoapps/courseware/tests/test_access.py
@@ -7,8 +7,9 @@ import ddt
 import itertools
 import pytz
 
-from django.test import TestCase
+from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
+from django.test import TestCase
 from mock import Mock, patch
 from nose.plugins.attrib import attr
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -504,15 +505,9 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase):
         self.user_staff = UserFactory.create(is_staff=True)
         self.user_anonymous = AnonymousUserFactory.create()
 
-    ENROLL_TEST_DATA = list(itertools.product(
+    COURSE_TEST_DATA = list(itertools.product(
         ['user_normal', 'user_staff', 'user_anonymous'],
-        ['enroll'],
-        ['course_default', 'course_started', 'course_not_started', 'course_staff_only'],
-    ))
-
-    LOAD_TEST_DATA = list(itertools.product(
-        ['user_normal', 'user_beta_tester', 'user_staff'],
-        ['load'],
+        ['enroll', 'load', 'staff', 'instructor', 'see_exists', 'see_in_catalog', 'see_about_page'],
         ['course_default', 'course_started', 'course_not_started', 'course_staff_only'],
     ))
 
@@ -528,8 +523,9 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase):
         ['course_default', 'course_with_pre_requisite', 'course_with_pre_requisites'],
     ))
 
-    @ddt.data(*(ENROLL_TEST_DATA + LOAD_TEST_DATA + LOAD_MOBILE_TEST_DATA + PREREQUISITES_TEST_DATA))
+    @ddt.data(*(COURSE_TEST_DATA + LOAD_MOBILE_TEST_DATA + PREREQUISITES_TEST_DATA))
     @ddt.unpack
+    @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False})
     def test_course_overview_access(self, user_attr_name, action, course_attr_name):
         """
         Check that a user's access to a course is equal to the user's access to
@@ -562,3 +558,35 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase):
         overview = CourseOverview.get_from_id(self.course_default.id)
         with self.assertRaises(ValueError):
             access.has_access(self.user, '_non_existent_action', overview)
+
+    @ddt.data(
+        *itertools.product(
+            ['user_normal', 'user_staff', 'user_anonymous'],
+            ['see_exists', 'see_in_catalog', 'see_about_page'],
+            ['course_default', 'course_started', 'course_not_started'],
+        )
+    )
+    @ddt.unpack
+    @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False})
+    def test_course_catalog_access_num_queries(self, user_attr_name, action, course_attr_name):
+        course = getattr(self, course_attr_name)
+
+        # get a fresh user object that won't have any cached role information
+        if user_attr_name == 'user_anonymous':
+            user = AnonymousUserFactory()
+        else:
+            user = getattr(self, user_attr_name)
+            user = User.objects.get(id=user.id)
+
+        if user_attr_name == 'user_staff' and action == 'see_exists' and course_attr_name == 'course_not_started':
+            # checks staff role
+            num_queries = 1
+        elif user_attr_name == 'user_normal' and action == 'see_exists' and course_attr_name != 'course_started':
+            # checks staff role and enrollment data
+            num_queries = 2
+        else:
+            num_queries = 0
+
+        course_overview = CourseOverview.get_from_id(course.id)
+        with self.assertNumQueries(num_queries):
+            bool(access.has_access(user, action, course_overview, course_key=course.id))