Bug#806740: tracker.debian.org: Please add AppStream hints panel

[Raphael Hertzog <hertzog@debian.org>]

Hi Matthias,

On Wed, 11 May 2016, Matthias Klumpp wrote:
> I created a preliminary patch to solve this issue.

Thanks, here are some early comments, I did not review everything in
detail. The patch seems rather long for this feature alone...

> +++ b/distro_tracker/vendor/debian/migrations/0002_appstreamstats.py

> --- a/distro_tracker/vendor/debian/models.py
> +++ b/distro_tracker/vendor/debian/models.py
> @@ -89,6 +89,51 @@ def get_lintian_url(self, full=False):
>  
>  
>  @python_2_unicode_compatible
> +class AppStreamStats(models.Model):
> +    """
> +    Model for AppStream hint stats of packages.
> +    """
> +    package = models.OneToOneField(PackageName, related_name='appstream_stats')
> +    stats = JSONField()

We don't want this model. It's basically a specialized version
of PackageExtractedInfo. Please use PackageExtractedInfo to store
your data related to each package (with a key="appstream" or similar).

> +    def get_appstream_url(self):
> +        """
> +        Returns the AppStream URL for the package matching the
> +        :class:`AppStreamStats
> +        <distro_tracker.vendor.debian.models.AppStreamHints>`.
> +        """
> +        package = get_or_none(SourcePackageName, pk=self.package.pk)
> +        if not package:
> +            return ''

This test is not needed as self.package is not allowed to be NULL/None.

> +        # TODO: What is the proper way to get (guess?) the archive-component vai the source-pkg here?
> +        section = "main"

I see two ways to achieve this:

- either by parsing the section in SourcePackageRepositoryEntry
  component = "main"
  if "/" in section:
     component = section.split("/")[0]

- or by parsing the directory in which the source package is stored:
  source.main_version.directory.split("/")[1]

> +++ b/distro_tracker/vendor/debian/templates/debian/appstream-action-item.html
> @@ -0,0 +1,14 @@
> +{% with warnings=item.extra_data.warnings %}
> +{% with errors=item.extra_data.errors %}
> +<a href="https://wiki.debian.org/AppStream";>AppStream</a> found
> +<a href="{{ item.extra_data.appstream_url }}">
> +{% if errors %}
> +<span>{{ errors }} error{% if errors > 1 %}s{% endif %}</span>
> +{% if warnings %}and{% endif %}
> +{% endif %}
> +{% if warnings %}
> +<span>{{ warnings }} warning{% if warnings > 1 %}s{% endif %}</span>
> +{% endif %}

Django has a pluralize filter for this kind of problems:
https://docs.djangoproject.com/en/1.9/ref/templates/builtins/#pluralize

> --- /dev/null
> +++ b/distro_tracker/vendor/debian/templates/debian/appstream-link.html
> @@ -0,0 +1,10 @@

I don't think that you should use a link and an action item for the same
purpose. I would keep only the action item.

About the tests, they might be a bit too extensive or they need some
refactoring to avoid duplication, I haven't checked in details.

I think that gzip compression should be abstracted away and
be tested separately. In other words, the mocked function that you use
should provide uncompressed data directly.

Also you might want to use static data stored in "tests-data" instead
of duplicating the same data in each test. We have a helper method
to locate data files from those directories (get_test_data_path).


I think this is enough feedback for a start. Thanks for working on this!

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/