Merge branch 'branch-coverage' of salsa.debian.org:lyknode/debexpo into live

MR: !187


Signed-off-by: Mattia Rizzolo <mattia@debian.org>

.gitlab-ci.yml

@@ -47,7 +47,7 @@ before_script:
   script:
     - python3 setup.py develop | tee /tmp/deps.txt
     - (! grep Downloading /tmp/deps.txt)
-    - python3 -m coverage run manage.py test -v 2 --exclude-tag nntp
+    - python3 -m coverage run --branch manage.py test -v 2 --exclude-tag nntp
     - python3 -m coverage report --include='debexpo*' --omit '*/nntp.py'
     - python3 -m coverage html --include='debexpo*' --omit '*/nntp.py'
   artifacts:

debexpo/accounts/models.py

@@ -26,6 +26,7 @@
 #   FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 #   OTHER DEALINGS IN THE SOFTWARE.
 
+from email.utils import getaddresses
 from enum import Enum
 
 from django.contrib.auth.models import AbstractUser, BaseUserManager
@@ -92,6 +93,28 @@ class UserManager(BaseUserManager):
 
         return self._create_user(email, name, password, **extra_fields)
 
+    def lookup_user_from_address(self, address):
+        """
+        Lookup a user using an arbitrary formatted email.
+
+        This method is used by Changes to lookup the user of an unsigned upload,
+        when allowed. The address is extracted from the .chagnes Changed-By
+        field or, if missing, from the Maintainer field.
+
+        The address format (Vincent Time <vtime@example.org>) is decoded using
+        email.utils.getaddresses.
+        """
+        if not address:
+            return
+
+        decoded_address = getaddresses([address])
+        email = decoded_address[0][1]
+
+        try:
+            return self.get(email=email)
+        except User.DoesNotExist:
+            return
+
 
 class User(AbstractUser):
     """User model."""

debexpo/accounts/views.py

@@ -278,6 +278,7 @@ class EmailChangeConfirmView(PasswordResetConfirmView):
         assert 'uidb64' in kwargs and 'token' in kwargs and 'email' in kwargs
 
         self.validlink = False
+        self.email = None
         self.user = self.get_user(kwargs['uidb64'])
 
         if self.user is not None:
@@ -307,7 +308,6 @@ class EmailChangeConfirmView(PasswordResetConfirmView):
         return self.render_to_response(self.get_context_data())
 
     def form_valid(self, form):
-        if self.validlink:
         self.user.email = self.email
         self.user.full_clean()
         self.user.save()

debexpo/comments/views.py

@@ -79,7 +79,12 @@ def subscribe(request, name):
         form = SubscriptionForm(request.POST, instance=instance)
         package = request.POST.get('next')
 
-        if form.is_valid():
+        # The condition here is just for show. The form is composed of two
+        # checkbox which are either absent for False or present with any value
+        # for True. It's not possible for it to be not valid.
+        #
+        # The test is kept here for future needs.
+        if form.is_valid():  # pragma: no branch
             subscription = form.save(commit=False)
             subscription.user = request.user
             subscription.package = name
@@ -129,3 +134,9 @@ def comment(request, name):
         comment.notify(request)
 
         return HttpResponseRedirect(f"{redirect_url}#upload-{index}")
+
+    return render(request, 'package.html', {
+        'settings': settings,
+        'package': package,
+        'comment_form': form,
+    })

debexpo/importer/models.py

@@ -237,9 +237,7 @@ class Importer():
             finally:
                 changes.remove()
 
-        if self.repository:
         self.repository.update()
-
         self.spool.cleanup()
 
         return success
@@ -411,7 +409,6 @@ class Importer():
             git_ref = git_storage.install(source)
 
         # Install to repository
-        if self.repository:
         self.repository.install(changes)
 
         # Create DB entries
@@ -424,8 +421,7 @@ class Importer():
         # match their checksum
         try:
             changes.validate()
-            if not self.skip_gpg:
-                changes.authenticate()
+            changes.authenticate(not self.skip_gpg)
             if getattr(settings, 'CHECK_NEWER_UPLOAD', True):
                 changes.assert_newer()
             changes.files.validate()

debexpo/packages/migrations/0001_initial.py

@@ -59,13 +59,11 @@ def create_distributions(apps, schema_editor):
     Project = apps.get_model('packages', 'Project')
 
     for name, project in DISTRIBUTIONS:
-        distribution = Distribution()
-
-        if project:
         project, _ = Project.objects.get_or_create(name=project)
-
+        distribution = Distribution()
         distribution.name = name
         distribution.project = project
+
         distribution.full_clean()
         distribution.save()
 

debexpo/packages/models.py

@@ -141,7 +141,6 @@ class Package(models.Model):
         return self.name
 
     def get_description(self):
-        if self.packageupload_set.count():
         upload = self.packageupload_set.latest('uploaded')
 
         if upload.binarypackage_set.count():
@@ -157,8 +156,6 @@ class Package(models.Model):
 
     def get_full_description(self):
         description = []
-
-        if self.packageupload_set.count():
         upload = self.packageupload_set.latest('uploaded')
 
         for binary in upload.binarypackage_set.all():

debexpo/packages/views.py

@@ -81,7 +81,8 @@ class PackageGroups(object):
                 x for x in packages if
                 x.packageupload_set.latest('uploaded').uploaded > deltamax
             ]
-        elif (deltamin is not None and deltamax is None):
+        # Last actual possibility
+        elif (deltamin is not None and deltamax is None):  # pragma: no branch
             self.packages = [
                 x for x in packages if
                 x.packageupload_set.latest('uploaded').uploaded <= deltamin

debexpo/plugins/lintian.py

@@ -38,20 +38,20 @@ from debexpo.tools.proc import debexpo_exec
 
 
 class PluginLintian(BasePlugin):
-    levels = [
-        {'level': 'X', 'name': 'Experimental', 'severity': PluginSeverity.info,
+    levels = {
+        'X': {'name': 'Experimental', 'severity': PluginSeverity.info,
               'outcome': 'Package has lintian experimental tags'},
-        {'level': 'P', 'name': 'Pedantic', 'severity': PluginSeverity.info,
+        'P': {'name': 'Pedantic', 'severity': PluginSeverity.info,
               'outcome': 'Package has lintian pedantic tags'},
-        {'level': 'O', 'name': 'Override', 'severity': PluginSeverity.info,
+        'O': {'name': 'Override', 'severity': PluginSeverity.info,
               'outcome': 'Package has overridden lintian tags'},
-        {'level': 'I', 'name': 'Info', 'severity': PluginSeverity.info,
+        'I': {'name': 'Info', 'severity': PluginSeverity.info,
               'outcome': 'Package has lintian informational tags'},
-        {'level': 'W', 'name': 'Warning', 'severity': PluginSeverity.warning,
+        'W': {'name': 'Warning', 'severity': PluginSeverity.warning,
               'outcome': 'Package has lintian warnings'},
-        {'level': 'E', 'name': 'Error', 'severity': PluginSeverity.error,
+        'E': {'name': 'Error', 'severity': PluginSeverity.error,
               'outcome': 'Package has lintian errors'},
-    ]
+    }
 
     @property
     def name(self):
@@ -102,15 +102,11 @@ class PluginLintian(BasePlugin):
                 continue
 
             severity, package, rest = tag.split(': ', 2)
-            name = [level['name'] for level in self.levels
-                    if level['level'] == severity]
             lintian_severities.add(severity)
             lintian_tag_data = rest.split()
             lintian_tag = lintian_tag_data[0]
             lintian_data = ' '.join(lintian_tag_data[1:])
-
-            if name:
-                severity = f'{severity}-{name[0]}'
+            severity = f'{severity}-{self.levels[severity]["name"]}'
 
             if override_comments:
                 comments = ' '.join(override_comments)
@@ -124,8 +120,8 @@ class PluginLintian(BasePlugin):
         severity = PluginSeverity.info
         outcome = 'Package is lintian clean'
 
-        for level in self.levels:
-            if level['level'] in lintian_severities:
+        for name, level in self.levels.items():
+            if name in lintian_severities:
                 severity = level['severity']
                 outcome = level['outcome']
 

debexpo/plugins/maintaineremail.py

@@ -43,10 +43,11 @@ class PluginMaintainerEmail(BasePlugin):
         """
         Tests whether the maintainer email is the same as the uploader email.
         """
+        if not changes.maintainer:
+            self.failed('No maintainer address found')
+
         uploader_emails = []
         maintainer_emails = email.utils.getaddresses([changes.maintainer])
-
-        if maintainer_emails:
         maintainer_email = maintainer_emails[0][1]
 
         if changes.dsc.uploaders:

debexpo/tools/debian/changes.py

@@ -86,9 +86,17 @@ class Changes(GPGSignedFile):
 
         return False
 
-    def authenticate(self):
+    def authenticate(self, gpg=True):
+        if gpg:
             super().authenticate()
             self.uploader = User.objects.get(key=self.key)
+        else:
+            user = User.objects.lookup_user_from_address(self.uploader)
+
+            if not user:
+                raise ExceptionChanges(f'No user found for {self.uploader}')
+
+            self.uploader = user
 
     def _build_changes(self):
         self.dsc = None

debexpo/tools/debian/copyright.py

@@ -78,7 +78,6 @@ class Copyright():
             return licenses
 
         for paragraph in self.copyright.all_files_paragraphs():
-            if paragraph.license and paragraph.license.synopsis:
             if not paragraph.files == ('debian/*',):
                 licenses.update([paragraph.license.synopsis])
 

debexpo/tools/debian/origin.py

@@ -60,7 +60,6 @@ class Origin():
 
         # For the origin tarball and its signature
         for origin in origin_files:
-            if origin:
             if isfile(join(self.dest_dir, str(origin))):
                 # Origin file is present in the upload
                 continue

debexpo/tools/gitstorage.py

@@ -56,7 +56,9 @@ class GitStorage():
         return ref
 
     def remove(self):
-        if isdir(self.repository):
+        # Exclude the False branch from coverage since having the repository
+        # removed is unlikely to happen and would be an external action
+        if isdir(self.repository):  # pragma: no branch
             rmtree(self.repository)
 
     # def diff(self, upload_from, upload_to):
@@ -107,7 +109,9 @@ class GitBackendDulwich():
             files.update(set(get_unstaged_changes(self.repo.open_index(),
                                                   'sources')))
 
-        if len(files) != 0:
+        # Exclude the False branch from coverage. In practice, there is always
+        # files present in debian/ with accepted packages
+        if len(files) != 0:  # pragma: no branch
             self.repo.stage(list(files))
 
     def commit(self):

debexpo/tools/gnupg.py

@@ -100,7 +100,7 @@ class GnuPG():
         """Returns true if the gpg binary is not installed or not executable."""
         return self.gpg_path is None
 
-    def get_keys_data(self, fingerprints=None):
+    def get_keys_data(self):
         """
         Returns the key object of the given GPG public key.
 
@@ -109,11 +109,8 @@ class GnuPG():
 
         """
 
-        if fingerprints is None:
-            fingerprints = []
-
         try:
-            (output, status) = self._run(args=['--list-keys'] + fingerprints)
+            (output, status) = self._run(['--list-keys'])
             keys = KeyData.read_from_gpg(output.splitlines())
 
             return list(keys.values())
@@ -129,8 +126,8 @@ class GnuPG():
         ``signed_file``
              path to signed file
         """
-        args = ('--verify', signed_file)
-        (output, status) = self._run(args=args)
+        args = ['--verify', signed_file]
+        (output, status) = self._run(args)
         output = output.splitlines()
 
         err_sig_re = re.compile(r'\[GNUPG:\] ERRSIG (?P<long_id>\w+)'
@@ -175,7 +172,7 @@ class GnuPG():
         """
         args = ['--import-options', 'import-minimal', '--import']
 
-        (output, status) = self._run(args=args, stdin=data)
+        (output, status) = self._run(args, stdin=data)
 
         if status and output and len(output.splitlines()) > 0:
             raise ExceptionGnuPG(_('Cannot add key:'
@@ -183,7 +180,7 @@ class GnuPG():
 
         return (output, status)
 
-    def _run(self, stdin=None, args=None):
+    def _run(self, args, stdin=None):
         """
         Run gpg with the given stdin and arguments and return the output and
         exit status.
@@ -217,10 +214,7 @@ class GnuPG():
             '--trust-model', 'always',
             '--with-colons',
             '--with-fingerprint'
-            ]
-
-        if args is not None:
-            cmd.extend(args)
+            ] + args
 
         try:
             output = debexpo_exec(self.gpg_path, cmd, env=env,
@@ -247,8 +241,6 @@ class KeyData():
 
     def get_uid(self, uid):
         uidfpr = uid[7]
-        res = self.uids.get(uidfpr, None)
-        if res is None:
         self.uids[uidfpr] = res = Uid(self, uid)
         return res
 
@@ -296,7 +288,6 @@ class KeyData():
                 # Correlate fpr with the previous pub record, and start
                 # gathering information for a new key
                 if pub:
-                    if cur_key is None:
                     keys[fpr] = cur_key = cls(fpr, pub)
                     pub_fpr = fpr
                     pub = None

tests/functional/accounts/test_profile.py

@@ -446,6 +446,16 @@ psCWDYcNhNTEmXgsDSqUlLrqqde/3hDynhWeAP9jk7QAnDToELBdyCe6HpGXLC3q
         # Login
         self.client.post(reverse('login'), self._AUTHDATA)
 
+        # Test password change, failed
+        response = self.client.post(reverse('profile'), {
+            'old_password': 'not-my-password',
+            'new_password1': 'newpassword',
+            'new_password2': 'newpassword',
+            'commit_password': 'submit'
+        })
+        self.assertEquals(response.status_code, 200)
+        self.assertIn('errorlist', str(response.content))
+
         # Test password change
         response = self.client.post(reverse('profile'), {
             'old_password': 'password',

tests/functional/accounts/test_update_email.py

@@ -86,13 +86,16 @@ class TestUpdateEmail(TransactionTestController):
         return token
 
     def _submit_token(self, token, email, invalid=False, error=None,
-                      redirect=None):
+                      redirect=None, submit_data=None):
+
+        if not submit_data:
             user = User.objects.get(email=self._AUTHDATA['username'])
             submit_data = {
                 'uidb64': urlsafe_base64_encode(force_bytes(user.pk)),
                 'token': token,
                 'email': email,
             }
+
         submit_url = reverse('email_change_confirm', kwargs=submit_data)
         response = self.client.get(submit_url)
 
@@ -173,6 +176,28 @@ class TestUpdateEmail(TransactionTestController):
         self._submit_token(token, 'new@example.org',
                            error='address already exists')
 
+    def test_update_email_invalid_token(self):
+        # On first call
+        invalid_data = {
+            'uidb64': 'a',
+            'token': 'x-x',
+            'email': 'a',
+        }
+
+        self._submit_token(None, 'new@example.org',
+                           invalid=True, submit_data=invalid_data)
+
+        # On redirect call
+        user = User.objects.get(email=self._AUTHDATA['username'])
+        invalid_data = {
+            'uidb64': urlsafe_base64_encode(force_bytes(user.pk)),
+            'token': 'change-email',
+            'email': 'another@example.org',
+        }
+
+        self._submit_token(None, 'new@example.org',
+                           invalid=True, submit_data=invalid_data)
+
     def test_update_email_mismatch_email(self):
         token = self._get_update_token('new@example.org')
 

tests/functional/base/test_page.py

@@ -26,6 +26,8 @@
 #   FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 #   OTHER DEALINGS IN THE SOFTWARE.
 
+from os import environ
+
 from django.test import TestCase
 from django.test import Client
 
@@ -43,6 +45,19 @@ PAGES = (
 
 
 class TestBrowsingPage(TestCase):
+    def test_debexpo_no_version(self):
+        client = Client()
+        path = environ['PATH']
+        environ['PATH'] = ''
+
+        try:
+            response = client.get('/')
+        finally:
+            environ['PATH'] = path
+
+        self.assertEquals(response.status_code, 200)
+        self.assertNotIn('Version', str(response.content))
+
     def test_page_content(self):
         client = Client()
 

tests/functional/importer/__init__.py

@@ -155,7 +155,7 @@ class TestImporterController(TestController):
                               skip_email=False, base_dir=None, sub_dir=None):
         source_package = TestSourcePackage(package_dir, base_dir)
 
-        source_package.build()
+        source_package.build(not skip_gpg)
         self._run_importer(source_package.get_package_dir(), skip_gpg=skip_gpg,
                            skip_email=skip_email, sub_dir=sub_dir)
 

tests/functional/importer/data/sources/hello_1.0-1_amd64.changes-not-signed

@@ -10,10 +10,7 @@ Maintainer: Baptiste BEAUPLAT <lyknode@cilg.org>
 Changed-By: Another Uploader <another@example.org>
 Description:
  hello      - <insert up to 60 chars description>
-Changes:
- hello (1.0-1) unstable; urgency=medium
- .
-   * Initial release (Closes: #nnnn)  <nnnn is the bug number of your ITP>
+Changes: Single line changes
 Checksums-Sha1:
  12b9785a45d1400abc684af9964fed348b2fcf9d 784 hello_1.0-1.dsc
  be505a70074ab85fed3f68026242651fcde85bf2 168 hello_1.0.orig.tar.xz