From c6c762cb577a831e40317978a9f4c1c3d4ea8ebb Mon Sep 17 00:00:00 2001 From: Mathieu Agopian Date: Fri, 6 Mar 2015 18:11:15 +0100 Subject: [PATCH] Be more lax on validating application versions for the stats (bug 1133543) --- .../commands/update_counts_from_file.py | 51 +++++++++---------- apps/stats/tests/test_commands.py | 22 +++++--- 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/apps/stats/management/commands/update_counts_from_file.py b/apps/stats/management/commands/update_counts_from_file.py index 31d2d8b1d514..b0c86109aeb1 100644 --- a/apps/stats/management/commands/update_counts_from_file.py +++ b/apps/stats/management/commands/update_counts_from_file.py @@ -12,7 +12,6 @@ import amo from addons.models import Addon -from applications.models import AppVersion from stats.models import update_inc, UpdateCount from . import get_date_from_file @@ -27,6 +26,12 @@ """, re.VERBOSE) VALID_STATUSES = ["userDisabled,incompatible", "userEnabled", "Unknown", "userDisabled", "userEnabled,incompatible"] +VALID_APP_GUIDS = amo.APP_GUIDS.keys() +APPVERSION_REGEX = re.compile( + r"""^[0-9]{1,3} # Major version: 2, 35 + \.[0-9]{1,3}([ab][0-9])? # Minor version + alpha or beta: .0a1, .0b2 + (\.[0-9]{1,3})?$ # Patch version: .1, .23 + """, re.VERBOSE) class Command(BaseCommand): @@ -96,18 +101,6 @@ def handle(self, *args, **options): .exclude(type=amo.ADDON_PERSONA) .values_list('guid', 'id'))) - # This gives a list of (application IDs, version). - appversions = AppVersion.objects.values_list('application', 'version') - # We want the application GUID, not the application ID. - appversions = [(amo.APPS_ALL[app_id].guid, version) - for app_id, version in appversions] - # This builds a dict where each key (the application guid) has a list - # of all its versions as a value. - self.valid_appversions = {} - for app_guid, app_version in appversions: - self.valid_appversions.setdefault(app_guid, []) - self.valid_appversions[app_guid].append(app_version) - index = -1 for group, filepath in group_filepaths: with codecs.open(filepath, encoding='utf8') as results_file: @@ -226,21 +219,23 @@ def update_status(self, update_count, status, count): def update_app(self, update_count, app_id, app_ver, count): """Update the applications on the update_count with the given data.""" - # Only update if app_id is a valid application guid, and if app_ver is - # a valid version for this app. - if app_ver in self.valid_appversions.get(app_id, []): - # Applications is a dict of dicts, eg: - # {"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}": - # {"10.0": 2, "21.0": 1, ....}, - # "some other application guid": ... - # } - if update_count.applications is None: - update_count.applications = {} - app = update_count.applications.get(app_id, {}) - # Now overwrite this application's dict with - # incremented counts for its versions. - update_count.applications.update( - {app_id: update_inc(app, app_ver, count)}) + # Only update if app_id is a valid application guid, and if app_ver + # "could be" a valid version. + if (app_id not in VALID_APP_GUIDS or + not re.match(APPVERSION_REGEX, app_ver)): + return + # Applications is a dict of dicts, eg: + # {"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}": + # {"10.0": 2, "21.0": 1, ....}, + # "some other application guid": ... + # } + if update_count.applications is None: + update_count.applications = {} + app = update_count.applications.get(app_id, {}) + # Now overwrite this application's dict with + # incremented counts for its versions. + update_count.applications.update( + {app_id: update_inc(app, app_ver, count)}) def update_os(self, update_count, os, count): """Update the OSes on the update_count with the given OS.""" diff --git a/apps/stats/tests/test_commands.py b/apps/stats/tests/test_commands.py index 48a1f2f7a35f..d200ae1f6eba 100644 --- a/apps/stats/tests/test_commands.py +++ b/apps/stats/tests/test_commands.py @@ -85,16 +85,26 @@ def test_update_status(self): assert uc.statuses == {'userEnabled': 123} def test_update_app(self): - # Initialize the known applications and their versions. - self.command.valid_appversions = {'{app-guid}': ['1.0', '2.0']} + firefox_guid = '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}' uc = UpdateCount(addon_id=3615) self.command.update_app(uc, 'foobar', '1.0', 123) # Non-existent app. assert not uc.applications - # Non-existent version. - self.command.update_app(uc, '{app-guid}', '3.0', 123) + # Malformed versions. + self.command.update_app(uc, firefox_guid, '3.0.1.2', 123) + self.command.update_app(uc, firefox_guid, '3.0123', 123) + self.command.update_app(uc, firefox_guid, '3.0c2', 123) + self.command.update_app(uc, firefox_guid, 'a.b.c', 123) assert not uc.applications - self.command.update_app(uc, '{app-guid}', '1.0', 123) - assert uc.applications == {'{app-guid}': {'1.0': 123}} + # Well formed versions. + self.command.update_app(uc, firefox_guid, '1.0', 123) + self.command.update_app(uc, firefox_guid, '1.0.1', 124) + self.command.update_app(uc, firefox_guid, '1.0a1', 125) + self.command.update_app(uc, firefox_guid, '1.0b2', 126) + assert uc.applications == {firefox_guid: { + '1.0': 123, + '1.0.1': 124, + '1.0a1': 125, + '1.0b2': 126}} def test_update_os(self): uc = UpdateCount(addon_id=3615)