Skip to content

Be more lax on validating application versions for the stats (bug 1133543) #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
magopian merged 1 commit into from
Mar 6, 2015
Merged

Be more lax on validating application versions for the stats (bug 1133543) #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 23 additions & 28 deletions apps/stats/management/commands/update_counts_from_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Copy link
Copy Markdown
Contributor

@kmaglione kmaglione Mar 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could technically be a 4th component, but we haven't done that since 2.0, so this is probably better.

""", re.VERBOSE)


class Command(BaseCommand):
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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."""
Expand Down
22 changes: 16 additions & 6 deletions apps/stats/tests/test_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down