[bug 1408981] Use more values and settings to calculate bundle.key (#303

)

* [bug 1408981] Use all Client values in bundle.key calculation.

* [bug 1408981] Update bundle key when related snippet templates change.

* Move firefox major version calculation to a function.

* [bug 1408981] Update bundle key when settings change.

* [bug 1408981] Update bundle.key when current firefox version changes.

snippets/base/models.py

@@ -27,30 +27,41 @@
 import django_mysql.models
 from jinja2 import Markup
 from jinja2.utils import LRUCache
-from product_details import product_details
-from product_details.version_compare import version_list
 
+from snippets.base import util
 from snippets.base.fields import RegexField
 from snippets.base.managers import ClientMatchRuleManager, SnippetManager
-from snippets.base.util import hashfile
 
 
 ONE_DAY = 60 * 60 * 24
 
 JINJA_ENV = engines['backend']
 
-SNIPPET_JS_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/includes/snippet.js'))
-SNIPPET_CSS_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/includes/snippet.css'))
-SNIPPET_FETCH_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/fetch_snippets.jinja'))
-SNIPPET_JS_AS_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/includes/snippet_as.js'))
-SNIPPET_CSS_AS_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/includes/snippet_as.css'))
-SNIPPET_FETCH_AS_TEMPLATE_HASH = hashfile(
-    os.path.join(settings.ROOT, 'snippets/base/templates/base/fetch_snippets_as.jinja'))
+SNIPPET_FETCH_TEMPLATE_HASH = hashlib.sha1(
+    render_to_string(
+        'base/fetch_snippets.jinja',
+        {
+            'snippet_ids': [],
+            'snippets_json': '',
+            'locale': 'xx',
+            'settings': settings,
+            'current_firefox_major_version': '00',
+            'metrics_url': settings.METRICS_URL,
+        }
+    )).hexdigest()
+
+SNIPPET_FETCH_TEMPLATE_AS_HASH = hashlib.sha1(
+    render_to_string(
+        'base/fetch_snippets_as.jinja',
+        {
+            'snippet_ids': [],
+            'snippets_json': '',
+            'locale': 'xx',
+            'settings': settings,
+            'current_firefox_major_version': '00',
+            'metrics_url': settings.METRICS_URL,
+        }
+    )).hexdigest()
 
 CHANNELS = ('release', 'beta', 'aurora', 'nightly')
 FIREFOX_STARTPAGE_VERSIONS = ('1', '2', '3', '4', '5')
@@ -145,19 +156,17 @@ def key(self):
         # Key should consist of snippets that are in the bundle plus any
         # properties of the client that may change the snippet code
         # being sent.
-        key_properties = ['{id}-{date}'.format(id=snippet.id, date=snippet.modified.isoformat())
-                          for snippet in self.snippets]
+        key_properties = [
+            '{id}-{date}-{templatedate}'.format(id=snippet.id,
+                                                date=snippet.modified.isoformat(),
+                                                templatedate=snippet.template.modified.isoformat())
+            for snippet in self.snippets]
 
+        key_properties.extend(self.client)
         key_properties.extend([
-            self.client.startpage_version,
-            self.client.locale,
-            self.client.channel,
-            SNIPPET_JS_TEMPLATE_HASH,
-            SNIPPET_CSS_TEMPLATE_HASH,
             SNIPPET_FETCH_TEMPLATE_HASH,
-            SNIPPET_JS_AS_TEMPLATE_HASH,
-            SNIPPET_CSS_AS_TEMPLATE_HASH,
-            SNIPPET_FETCH_AS_TEMPLATE_HASH,
+            SNIPPET_FETCH_TEMPLATE_AS_HASH,
+            util.current_firefox_major_version(),
         ])
 
         key_string = u'_'.join(key_properties)
@@ -215,9 +224,6 @@ def snippets(self):
 
     def generate(self):
         """Generate and save the code for this snippet bundle."""
-        current_firefox_version = (
-            version_list(product_details.firefox_history_major_releases)[0].split('.', 1)[0])
-
         template = 'base/fetch_snippets.jinja'
         if self.client.startpage_version == '5':
             template = 'base/fetch_snippets_as.jinja'
@@ -227,7 +233,7 @@ def generate(self):
             'client': self.client,
             'locale': self.client.locale,
             'settings': settings,
-            'current_firefox_version': current_firefox_version,
+            'current_firefox_major_version': util.current_firefox_major_version(),
         })
 
         if isinstance(bundle_content, unicode):

snippets/base/templates/base/fetch_snippets.jinja

@@ -7,7 +7,7 @@
  //  - {{ id }}
      {% endfor %}
      var ABOUTHOME_SNIPPETS = JSON.parse('{{ snippets_json|escapejs|safe }}');
-     var CURRENT_RELEASE = {{ current_firefox_version }};
+     var CURRENT_RELEASE = {{ current_firefox_major_version }};
      {% include 'base/includes/snippet.js' %}
 
  //]]>

snippets/base/templates/base/fetch_snippets_as.jinja

@@ -8,6 +8,6 @@
  //  - {{ id }}
  {% endfor %}
  var ABOUTHOME_SNIPPETS = JSON.parse("{{ snippets_json|escapejs|safe }}");
- var CURRENT_RELEASE = {{ current_firefox_version }};
+ var CURRENT_RELEASE = {{ current_firefox_major_version }};
  {% include 'base/includes/snippet_as.js' %}
 </script>

snippets/base/tests/test_models.py

@@ -417,6 +417,18 @@ def test_key_locale(self):
 
         self.assertNotEqual(bundle1.key, bundle2.key)
 
+    def test_key_name(self):
+        """
+        bundle.key must be different between bundles if they have
+        different names.
+        """
+        client1 = self._client(name='firefox')
+        client2 = self._client(name='firefox-test')
+        bundle1 = SnippetBundle(client1)
+        bundle2 = SnippetBundle(client2)
+
+        self.assertNotEqual(bundle1.key, bundle2.key)
+
     def test_key_equal(self):
         client1 = self._client(locale='en-US', startpage_version='4')
         client2 = self._client(locale='en-US', startpage_version='4')
@@ -427,6 +439,45 @@ def test_key_equal(self):
 
         self.assertEqual(bundle1.key, bundle2.key)
 
+    def test_key_snippet_modified(self):
+        client1 = self._client(locale='en-US', startpage_version='4')
+        bundle = SnippetBundle(client1)
+        bundle._snippets = [self.snippet1]
+        key_1 = bundle.key
+
+        # save snippet, touch modified
+        self.snippet1.save()
+        bundle = SnippetBundle(client1)
+        bundle._snippets = [self.snippet1]
+        key_2 = bundle.key
+        self.assertNotEqual(key_1, key_2)
+
+    def test_key_template_modified(self):
+        client1 = self._client(locale='en-US', startpage_version='4')
+        bundle = SnippetBundle(client1)
+        bundle._snippets = [self.snippet1]
+        key_1 = bundle.key
+
+        # save template, touch modified
+        self.snippet1.template.save()
+        bundle = SnippetBundle(client1)
+        bundle._snippets = [self.snippet1]
+        key_2 = bundle.key
+        self.assertNotEqual(key_1, key_2)
+
+    def test_key_current_firefox_version(self):
+        client1 = self._client(locale='en-US', startpage_version='4')
+        bundle = SnippetBundle(client1)
+        bundle._snippets = [self.snippet1]
+        key_1 = bundle.key
+
+        with patch('snippets.base.util.current_firefox_major_version') as cfmv:
+            cfmv.return_value = 'xx'
+            bundle = SnippetBundle(client1)
+            bundle._snippets = [self.snippet1]
+            key_2 = bundle.key
+        self.assertNotEqual(key_1, key_2)
+
     def test_generate(self):
         """
         bundle.generate should render the snippets, save them to the
@@ -440,8 +491,8 @@ def test_generate(self):
             with patch('snippets.base.models.render_to_string') as render_to_string:
                 with patch('snippets.base.models.default_storage') as default_storage:
                     with self.settings(SNIPPET_BUNDLE_TIMEOUT=10):
-                        with patch('snippets.base.models.version_list') as version_list:
-                            version_list.return_value = ['45.0']
+                        with patch('snippets.base.util.current_firefox_major_version') as cfmv:
+                            cfmv.return_value = '45'
                             render_to_string.return_value = 'rendered snippet'
                             bundle.generate()
 
@@ -451,7 +502,7 @@ def test_generate(self):
             'client': bundle.client,
             'locale': 'fr',
             'settings': settings,
-            'current_firefox_version': '45',
+            'current_firefox_major_version': '45',
         })
         default_storage.save.assert_called_with(bundle.filename, ANY)
         cache.set.assert_called_with(bundle.cache_key, True, ONE_DAY)
@@ -474,8 +525,8 @@ def test_generate_activity_stream(self):
             with patch('snippets.base.models.render_to_string') as render_to_string:
                 with patch('snippets.base.models.default_storage') as default_storage:
                     with self.settings(SNIPPET_BUNDLE_TIMEOUT=10):
-                        with patch('snippets.base.models.version_list') as version_list:
-                            version_list.return_value = ['45.0']
+                        with patch('snippets.base.util.current_firefox_major_version') as cfmv:
+                            cfmv.return_value = '45'
                             render_to_string.return_value = 'rendered snippet'
                             bundle.generate()
 
@@ -485,7 +536,7 @@ def test_generate_activity_stream(self):
             'client': bundle.client,
             'locale': 'fr',
             'settings': settings,
-            'current_firefox_version': '45',
+            'current_firefox_major_version': '45',
         })
         default_storage.save.assert_called_with(bundle.filename, ANY)
         cache.set.assert_called_with(bundle.cache_key, True, ONE_DAY)

snippets/base/util.py

@@ -1,6 +1,5 @@
-import hashlib
-
 from product_details import product_details
+from product_details.version_compare import version_list
 
 
 def get_object_or_none(model_class, **filters):
@@ -22,13 +21,6 @@ def first(collection, callback):
     return next((item for item in collection if callback(item)), None)
 
 
-def hashfile(filepath):
-    sha1 = hashlib.sha1()
-    with open(filepath, 'rb') as fp:
-        sha1.update(fp.read())
-    return sha1.hexdigest()
-
-
 def create_locales():
     from snippets.base.models import TargetedLocale
 
@@ -48,3 +40,10 @@ def create_countries():
         if country.name != name:
             country.name = name
             country.save()
+
+
+def current_firefox_major_version():
+    full_version = version_list(
+        product_details.firefox_history_major_releases)[0]
+
+    return full_version.split('.', 1)[0]

snippets/base/views.py

@@ -18,10 +18,9 @@
 
 import django_filters
 from django_statsd.clients import statsd
-from product_details import product_details
-from product_details.version_compare import version_list
 from raven.contrib.django.models import client as sentry_client
 
+from snippets.base import util
 from snippets.base.decorators import access_control
 from snippets.base.encoders import JSONSnippetEncoder
 from snippets.base.models import Client, JSONSnippet, Snippet, SnippetBundle, SnippetTemplate
@@ -122,9 +121,6 @@ def fetch_render_snippets(request, **kwargs):
                          .select_related('template')
                          .filter_by_available())
 
-    current_firefox_version = (
-        version_list(product_details.firefox_history_major_releases)[0].split('.', 1)[0])
-
     template = 'base/fetch_snippets.jinja'
 
     if client.startpage_version == '5':
@@ -134,7 +130,7 @@ def fetch_render_snippets(request, **kwargs):
         'snippets_json': json.dumps([s.to_dict() for s in matching_snippets]),
         'client': client,
         'locale': client.locale,
-        'current_firefox_version': current_firefox_version,
+        'current_firefox_major_version': util.current_firefox_major_version(),
     })
 
     # ETag will be a hash of the response content.
@@ -205,14 +201,11 @@ def preview_snippet(request):
         preview_client = Client('5', 'Firefox', '57.0', 'default', 'default', 'en-US',
                                 'release', 'default', 'default', 'default')
 
-    current_firefox_version = (
-        version_list(product_details.firefox_history_major_releases)[0].split('.', 1)[0])
-
     return render(request, template_name, {
         'snippets_json': json.dumps([snippet.to_dict()]),
         'client': preview_client,
         'preview': True,
-        'current_firefox_version': current_firefox_version,
+        'current_firefox_major_version': util.current_firefox_major_version(),
     })
 
 
@@ -224,13 +217,11 @@ def show_snippet(request, snippet_id):
     if snippet.disabled and not request.user.is_authenticated():
         raise Http404()
 
-    current_firefox_version = (
-        version_list(product_details.firefox_history_major_releases)[0].split('.', 1)[0])
     return render(request, 'base/preview.jinja', {
         'snippets_json': json.dumps([snippet.to_dict()]),
         'client': preview_client,
         'preview': True,
-        'current_firefox_version': current_firefox_version,
+        'current_firefox_major_version': util.current_firefox_major_version(),
     })