Merge into trunk : released-by-default : Code : Ubuntu Apps Directory

Status:

Merged

Approved by:

Michael Nelson on 2012-08-30

Approved revision:

173

Merged at revision:

169

Proposed branch:

lp://staging/~elachuni/ubuntu-webcatalog/released-by-default

Merge into:

lp://staging/ubuntu-webcatalog

Diff against target:

251 lines (+117/-36)

6 files modified

src/webcatalog/management/commands/check_all_latest.py (+30/-12)
src/webcatalog/managers.py (+18/-8)
src/webcatalog/tests/test_commands.py (+28/-0)
src/webcatalog/tests/test_managers.py (+39/-14)
src/webcatalog/tests/test_models_oauth.py (+1/-1)
src/webcatalog/tests/test_pep8.py (+1/-1)

To merge this branch:

bzr merge lp://staging/~elachuni/ubuntu-webcatalog/released-by-default

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Nelson (community)		2012-08-29	Approve on 2012-08-30
Review via email: mp+121954@code.staging.launchpad.net

Commit message

Consider prereleased distroseries when displaying app data and calculating is_latest.

Description of the change

At some point the 'prerelease' bit was added onto a distroseries. At that point I thought we had made prerelease versions of apps not show up by default, but I seem to be wrong (or the code might have been rolled back at some point?)

Related bug #1042928 asks for this again: The data an application should display by default, when no distroseries is specified, should be the latest *released* version, if there is any released version available. If there is no released version, then it's fine to display the prerelease data.

While I was there, I made the pep8 test fail when it finds an issue, as it seems it was just displaying the error but not failing the test.

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2012-08-30:

#

Download full text (12.4 KiB)

On Wed, Aug 29, 2012 at 11:56 PM, Anthony Lenton
<email address hidden> wrote:
> For more details, see:
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/released-by-default/+merge/121954
>
> At some point the 'prerelease' bit was added onto a distroseries. At that point I thought we had made prerelease versions of apps not
> show up by default, but I seem to be wrong (or the code might have been rolled back at some point?)

That sounds bad - have you checked?

>
> Related bug #1042928 asks for this again: The data an application should display by default, when no distroseries is specified, should
> be the latest *released* version, if there is any released version available. If there is no released version, then it's fine to display the
> prerelease data.
>
> While I was there, I made the pep8 test fail when it finds an issue, as it seems it was just displaying the error but not failing the test.
> --
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/released-by-default/+merge/121954
> You are subscribed to branch lp:ubuntu-webcatalog.
>
> === modified file 'src/webcatalog/management/commands/check_all_latest.py'
> --- src/webcatalog/management/commands/check_all_latest.py 2012-06-06 16:38:11 +0000
> +++ src/webcatalog/management/commands/check_all_latest.py 2012-08-29 21:55:21 +0000
> @@ -48,17 +48,35 @@
> Application.objects.all().update(is_latest=False)
>
> # 2. Update is_latest in batches of BATCH_SIZE.
> - packages = Application.objects.values_list('id', 'package_name')
> + packages = Application.objects.values_list(
> + 'id', 'package_name', 'distroseries__prerelease')
> packages = packages.order_by('package_name', '-distroseries__version')
> current_package_name = None
> to_update = []
> - for app_id, app_package_name in packages:
> + prereleased = None # Store a prerelease app id here until we find
> + # a released version or run out of versions
> + for app_id, app_package_name, prerelease in packages:
> if current_package_name != app_package_name:
> - to_update.append(app_id)
> - if len(to_update) >= BATCH_SIZE:
> - Application.objects.filter(id__in=to_update).update(
> - is_latest=True)
> - to_update = []
> + is_latest = True
> current_package_name = app_package_name
> + if prereleased:
> + # If prereleased != None at this point, it's because the
> + # immediately previous app *only* has prerelease versions

Ah right, so a pre-release can have is_latest=True if there aren't any
other versions. Got it.
I got confused with prerelease vs prereleased... I wonder if
s/prereleased/prereleased_app_id/ might be worthwhile.

> + to_update.append(prereleased)
> + prereleased = None
> + if is_latest:
> + if prerelease:
> + if prereleased is None:
> + prereleased = app_id
> + else:
> + ...

On Wed, Aug 29, 2012 at 11:56 PM, Anthony Lenton
<anthony.lenton@canonical.com> wrote:
> For more details, see:
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/released-by-default/+merge/121954
>
> At some point the 'prerelease' bit was added onto a distroseries.  At that point I thought we had made prerelease versions of apps not
> show up by default, but I seem to be wrong (or the code might have been rolled back at some point?)

That sounds bad - have you checked?

>
> Related bug #1042928 asks for this again:  The data an application should display by default, when no distroseries is specified, should
> be the latest *released* version, if there is any released version available.  If there is no released version, then it's fine to display the
> prerelease data.
>
> While I was there, I made the pep8 test fail when it finds an issue, as it seems it was just displaying the error but not failing the test.
> --
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/released-by-default/+merge/121954
> You are subscribed to branch lp:ubuntu-webcatalog.
>
> === modified file 'src/webcatalog/management/commands/check_all_latest.py'
> --- src/webcatalog/management/commands/check_all_latest.py      2012-06-06 16:38:11 +0000
> +++ src/webcatalog/management/commands/check_all_latest.py      2012-08-29 21:55:21 +0000
> @@ -48,17 +48,35 @@
>          Application.objects.all().update(is_latest=False)
>
>          # 2. Update is_latest in batches of BATCH_SIZE.
> -        packages = Application.objects.values_list('id', 'package_name')
> +        packages = Application.objects.values_list(
> +            'id', 'package_name', 'distroseries__prerelease')
>          packages = packages.order_by('package_name', '-distroseries__version')
>          current_package_name = None
>          to_update = []
> -        for app_id, app_package_name in packages:
> +        prereleased = None  # Store a prerelease app id here until we find
> +                            # a released version or run out of versions
> +        for app_id, app_package_name, prerelease in packages:
>              if current_package_name != app_package_name:
> -                to_update.append(app_id)
> -                if len(to_update) >= BATCH_SIZE:
> -                    Application.objects.filter(id__in=to_update).update(
> -                        is_latest=True)
> -                    to_update = []
> +                is_latest = True
>                  current_package_name = app_package_name
> +                if prereleased:
> +                    # If prereleased != None at this point, it's because the
> +                    # immediately previous app *only* has prerelease versions

Ah right, so a pre-release can have is_latest=True if there aren't any
other versions. Got it.
I got confused with prerelease vs prereleased... I wonder if
s/prereleased/prereleased_app_id/ might be worthwhile.

> +                    to_update.append(prereleased)
> +                    prereleased = None
> +            if is_latest:
> +                if prerelease:
> +                    if prereleased is None:
> +                        prereleased = app_id
> +                else:
> +                    to_update.append(app_id)

Ignore at will, but s/to_update/latest_app_ids/ .

> +                    prereleased = None
> +                    is_latest = False
> +            if len(to_update) >= BATCH_SIZE:
> +                Application.objects.filter(id__in=to_update).update(
> +                    is_latest=True)
> +                to_update = []
> +        if prereleased:
> +            to_update.append(prereleased)

This whole algorithm is getting quite confusing, basically because
we're looping through the individual apps. I wonder if it would be
much simpler if we still had the one query for all the apps, but
rather than iterating them one at a time, used a generator to deal
with one package_name at a time, like:

def get_versions_for_app(packages):
    versions_for_app = []
    current_package_name = None
    for app_id, app_package_name, prerelease in packages:
        if current_package_name != app_package_name and versions_for_app:
            yield versions_for_app
            versions_for_app = []
        versions_for_app.append((app_id, app_package_name, prerelease))

return versions_for_app

def get_latest_version_for_app(versions):
    # simple helper that returns the id of the first non-pre-release
app, or the pre-release if there are no others.

Then the algorithm could be something really simple like:
    while versions = get_versions_for_app(packages):
        latest_app_ids.append(get_latest_version_for_app(versions))
        # check batch size etc.

Anyway, just if you think it's worthwhile... I had to spend a few mins
trying to understand the current logic.

>          if to_update:
>              Application.objects.filter(id__in=to_update).update(is_latest=True)
>
> === modified file 'src/webcatalog/managers.py'
> --- src/webcatalog/managers.py  2012-07-02 21:25:30 +0000
> +++ src/webcatalog/managers.py  2012-08-29 21:55:21 +0000
> @@ -33,7 +33,13 @@
>      def find_best(self, **kwargs):
>          options = self.filter(**kwargs).order_by('-distroseries__version')
>          if options.exists():
> -            return options[0]
> +            prereleased = []
> +            for option in options:
> +                if option.distroseries.prerelease:
> +                    prereleased.append(option)
> +                else:
> +                    return option
> +            return prereleased[0]
>
>      def find_best_or_404(self, **kwargs):
>          app = self.find_best(**kwargs)
> @@ -46,10 +52,20 @@
>          options = self.filter(package_name=package_name).order_by(
>              '-distroseries__version')
>          is_latest = True
> +        prereleased = []
> +
> +        def set_latest(app, is_latest):
> +            if app.is_latest != is_latest:
> +                app.is_latest = is_latest
> +                app.save()
>          for option in options:
> -            if option.is_latest != is_latest:
> -                option.is_latest = is_latest
> -                option.save()
> +            if option.distroseries.prerelease:
> +                prereleased.append(option)
> +            else:
> +                set_latest(option, is_latest)
> +                is_latest = False
> +        for option in prereleased:
> +            set_latest(option, is_latest)
>              is_latest = False

Had to check the context of this change... we discussed on irc whether
this second implementation for setting is_latest is necessary. It's
currently used in import_sca_apps to mark the latest for a particular
app_name. We may be able to DRY up things a bit by using a function
which takes a list (or result_set) of app_id, package_name,
pre_release in given order and updates the is_latest. But not
important for this.

>
>
>
> === modified file 'src/webcatalog/tests/test_commands.py'
> --- src/webcatalog/tests/test_commands.py       2012-08-21 13:57:11 +0000
> +++ src/webcatalog/tests/test_commands.py       2012-08-29 21:55:21 +0000
> @@ -966,6 +966,34 @@
>          self.assertTrue(Application.objects.get(package_name='bar').is_latest)
>          self.assertTrue(Application.objects.get(package_name='baz').is_latest)
>
> +    def test_updates_to_latest_released(self):
> +        for version in ['12.04', '12.10', '13.04', '13.10']:
> +            dseries = self.factory.make_distroseries(version=version)
> +            self.factory.make_application('whatnot', distroseries=dseries)
> +        dseries = self.factory.make_distroseries(version='14.04',
> +                                                 prerelease=True)
> +        self.factory.make_application('whatnot', distroseries=dseries)
> +
> +        call_command('check_all_latest')
> +
> +        retrieved = Application.objects.filter(package_name='whatnot')
> +        retrieved = retrieved.order_by('-distroseries__version')
> +        expected = [False, True, False, False, False]
> +        self.assertEqual(expected, [app.is_latest for app in retrieved])
> +
> +    def test_updates_to_latest_to_prerelease_if_none_released(self):
> +        for version in ['12.04', '12.10', '13.04']:
> +            dseries = self.factory.make_distroseries(version=version,
> +                                                     prerelease=True)
> +            self.factory.make_application('gizmo', distroseries=dseries)
> +
> +        call_command('check_all_latest')
> +
> +        retrieved = Application.objects.filter(package_name='gizmo')
> +        retrieved = retrieved.order_by('-distroseries__version')
> +        expected = [True, False, False]
> +        self.assertEqual(expected, [app.is_latest for app in retrieved])
> +

Great.

>
>  class CleanupTestCase(TestCaseWithFactory):
>
>
> === modified file 'src/webcatalog/tests/test_managers.py'
> --- src/webcatalog/tests/test_managers.py       2012-07-02 21:25:30 +0000
> +++ src/webcatalog/tests/test_managers.py       2012-08-29 21:55:21 +0000
> @@ -19,6 +19,8 @@
>
>  from __future__ import absolute_import
>
> +from django.http import Http404
> +
>  from webcatalog.tests.factory import TestCaseWithFactory
>  from webcatalog.models import Application
>
> @@ -30,31 +32,54 @@
>
>  class ApplicationManagerTestCase(TestCaseWithFactory):
>
> +    def make_apps(self, package_name, versions, prerelease=None):
> +        results = []
> +        for version in versions:
> +            is_prerelease = version == prerelease
> +            distroseries = self.factory.make_distroseries(
> +                version=version, prerelease=is_prerelease)
> +            app = self.factory.make_application(
> +                package_name, distroseries=distroseries)
> +            results.append(app)
> +        return results
> +
>      def test_find_best_returns_none(self):
>          self.assertIsNone(Application.objects.find_best(package_name='foo'))
>
> +    def test_find_best_or_404_raises(self):
> +        self.assertRaises(Http404, Application.objects.find_best_or_404,
> +                          package_name='doodle')
> +
>      def test_find_best_returns_latest(self):
> -        latest = self.factory.make_distroseries(version='14.10')
> -        older = self.factory.make_distroseries(version='14.04')
> -
> -        expected = self.factory.make_application(distroseries=latest)
> -        self.factory.make_application(
> -            package_name=expected.package_name, distroseries=older)
> -
> -        retrieved = Application.objects.find_best(
> -            package_name=expected.package_name)
> +        expected, _ = self.make_apps('gparted', ['14.10', '14.04'])
> +
> +        retrieved = Application.objects.find_best(package_name='gparted')
> +
> +        self.assertEqual(expected.id, retrieved.id)
> +
> +    def test_find_best_returns_latest_released(self):
> +        _, expected = self.make_apps('vlc', ['14.10', '14.04'],
> +                                     prerelease='14.10')
> +
> +        retrieved = Application.objects.find_best(package_name='vlc')
> +
> +        self.assertEqual(expected.id, retrieved.id)
> +
> +    def test_find_best_returns_prerelease_if_no_release_available(self):
> +        [expected] = self.make_apps('amnesia', ['14.10'],
> +                                    prerelease='14.10')
> +
> +        retrieved = Application.objects.find_best(package_name='amnesia')
>
>          self.assertEqual(expected.id, retrieved.id)
>
>      def test_check_latest(self):
> -        for version in ['10.04', '10.10', '11.04', '11.10']:
> -            dseries = self.factory.make_distroseries(version=version)
> -            self.factory.make_application(
> -                package_name='foobar', distroseries=dseries)
> +        self.make_apps('foobar', ['10.04', '10.10', '11.04', '11.10'],
> +                       prerelease='11.10')
>
>          Application.objects.check_latest('foobar')
>
>          retrieved = Application.objects.filter(package_name='foobar').order_by(
>              '-distroseries__version')
> -        self.assertEqual([True, False, False, False],
> +        self.assertEqual([False, True, False, False],
>                           [app.is_latest for app in retrieved])

Sweet.

>
> === modified file 'src/webcatalog/tests/test_models_oauth.py'
> --- src/webcatalog/tests/test_models_oauth.py   2012-08-23 10:07:28 +0000
> +++ src/webcatalog/tests/test_models_oauth.py   2012-08-29 21:55:21 +0000
> === modified file 'src/webcatalog/tests/test_pep8.py'
> --- src/webcatalog/tests/test_pep8.py   2012-06-26 23:45:37 +0000
> +++ src/webcatalog/tests/test_pep8.py   2012-08-29 21:55:21 +0000

Revision history for this message

Michael Nelson (michael.nelson) on 2012-08-30:

#

review: Approve

lp://staging/~elachuni/ubuntu-webcatalog/released-by-default updated on 2012-08-30

172. By Anthony Lenton on 2012-08-30: Refactored check_all_latest and dried up code a bit, per code review.
173. By Anthony Lenton on 2012-08-30: Minor refactor to find_best.

Ubuntu Apps Directory

Merge lp://staging/~elachuni/ubuntu-webcatalog/released-by-default into lp://staging/ubuntu-webcatalog

Commit message

Description of the change

Preview Diff

Subscribers