Repository: unt-libraries/django-nomination

Issue:
Use get_object_or_404
We really should be using [get_object_or_404](https://docs.djangoproject.com/en/1.11/topics/http/shortcuts/#get-object-or-404) any place that will grabbing a database record based on the URL, such as [here](https://github.com/unt-libraries/django-nomination/blob/master/nomination/views.py#L841). Would help to avoid some unnecessary server errors.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2757b5c..a8d28f3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,7 +15,9 @@ Change Log
 * Removed compatibility with Python 2
 * Added slim-select for improved select inputs
 * Prevent duplicates in url_report if a surt is duplicated in the data.
-
+* nominator_email is made unique.
+* Fixed admin links.
+* Used get_or_create and get_object_or_404 where appropriate.
 
 2.0.0
 -----
diff --git a/nomination/migrations/0005_auto_20210429_2207.py b/nomination/migrations/0005_auto_20210429_2207.py
new file mode 100644
index 0000000..50420b5
--- /dev/null
+++ b/nomination/migrations/0005_auto_20210429_2207.py
@@ -0,0 +1,18 @@
+# Generated by Django 2.2.20 on 2021-04-29 22:07
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('nomination', '0004_auto_20190927_1904'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='nominator',
+            name='nominator_email',
+            field=models.CharField(help_text='An email address for identifying your nominations in the system.', max_length=100, unique=True),
+        ),
+    ]
diff --git a/nomination/models.py b/nomination/models.py
index 6a4573e..8c55514 100644
--- a/nomination/models.py
+++ b/nomination/models.py
@@ -2,6 +2,7 @@ import datetime
 from django.db import models
 from django.contrib.sites.models import Site
 from django.conf import settings
+from django.utils.safestring import mark_safe
 
 FORM_TYPES = (
     ('checkbox', 'checkbox'),
@@ -207,6 +208,7 @@ class Nominator(models.Model):
     )
     nominator_email = models.CharField(
         max_length=100,
+        unique=True,
         help_text='An email address for identifying your nominations in the system.'
     )
 
@@ -234,7 +236,7 @@ class URL(models.Model):
         return self.entity
 
     def entity_display(self):
-        return(
+        return mark_safe(
             "<a href=\'http://%s/admin/nomination/url/%s\'>%s</a>&nbsp;"
             "<a href=\'%s\'><img src=\'%snomination/images/external-link.gif\'/></a>"
             % (Site.objects.get_current().domain, self.id, self.entity, self.entity,
@@ -244,13 +246,15 @@ class URL(models.Model):
     entity_display.allow_tags = True
 
     def get_project(self):
-        return "<a href=\'http://%s/admin/nomination/project/%s\'>%s</a>" % \
-                    (Site.objects.get_current().domain, self.url_project.id, self.url_project)
+        return mark_safe(
+            "<a href=\'http://%s/admin/nomination/project/%s\'>%s</a>" %
+            (Site.objects.get_current().domain, self.url_project.id, self.url_project))
     get_project.short_description = 'Project'
     get_project.allow_tags = True
 
     def get_nominator(self):
-        return "<a href=\'http://%s/admin/nomination/nominator/%s\'>%s</a>" % \
-                    (Site.objects.get_current().domain, self.url_nominator.id, self.url_nominator)
+        return mark_safe(
+            "<a href=\'http://%s/admin/nomination/nominator/%s\'>%s</a>" %
+            (Site.objects.get_current().domain, self.url_nominator.id, self.url_nominator))
     get_nominator.short_description = 'Nominator'
     get_nominator.allow_tags = True
diff --git a/nomination/url_handler.py b/nomination/url_handler.py
index 810205c..320fc3e 100644
--- a/nomination/url_handler.py
+++ b/nomination/url_handler.py
@@ -8,6 +8,8 @@ from urllib.parse import urlparse
 
 from django import http
 from django.conf import settings
+from django.shortcuts import get_object_or_404
+from django.db import IntegrityError
 
 from nomination.models import Project, Nominator, URL, Value
 
@@ -121,10 +123,7 @@ def add_url(project, form_data):
     summary_list = []
     form_data['url_value'] = check_url(form_data['url_value'])
     # Get the system nominator
-    try:
-        system_nominator = Nominator.objects.get(id=settings.SYSTEM_NOMINATOR_ID)
-    except Nominator.DoesNotExist:
-        raise http.Http404
+    system_nominator = get_object_or_404(Nominator, id=settings.SYSTEM_NOMINATOR_ID)
 
     # Check for or add surt
     surt_successful = surt_exists(project, system_nominator, form_data['url_value'])
@@ -134,7 +133,7 @@ def add_url(project, form_data):
     # Get/Add a nominator
     nominator = get_nominator(form_data)
     if not nominator:
-        raise http.Http404
+        return False
 
     # Nominate a URL
     summary_list = nominate_url(project, nominator, form_data, '1')
@@ -172,17 +171,27 @@ def check_url(url):
 def get_nominator(form_data):
     try:
         # Try to retrieve the nominator
-        nominator = Nominator.objects.get(nominator_email=form_data['nominator_email'])
-    except (Nominator.DoesNotExist, Nominator.MultipleObjectsReturned):
+        nominator, created = Nominator.objects.get_or_create(
+                             nominator_email=form_data['nominator_email'],
+                             defaults={
+                                       'nominator_name': form_data['nominator_name'],
+                                       'nominator_institution': form_data['nominator_institution']
+                                      })
+
+    except Nominator.MultipleObjectsReturned:
+        # Retrieve unique nominator
         try:
-            # Create a new nominator object
-            nominator = Nominator(nominator_email=form_data['nominator_email'],
-                                  nominator_name=form_data['nominator_name'],
-                                  nominator_institution=form_data['nominator_institution'])
-            nominator.save()
-        except Exception:
+            nominator = Nominator.objects.get(
+                            nominator_email=form_data['nominator_email'],
+                            nominator_name=form_data['nominator_name'],
+                            nominator_institution=form_data['nominator_institution'])
+
+        except (Nominator.MultipleObjectsReturned, Nominator.DoesNotExist):
             return False
 
+    except (IntegrityError, KeyError):
+        raise http.Http404
+
     return nominator
 
 
@@ -191,23 +200,21 @@ def nominate_url(project, nominator, form_data, scope_value):
     # Nominate URL
     try:
         # Check if user has already nominated the URL
-        nomination_url = URL.objects.get(url_nominator__id__iexact=nominator.id,
-                                         url_project=project,
-                                         entity__iexact=form_data['url_value'],
-                                         attribute__iexact='nomination')
+        nomination_url, created = URL.objects.get_or_create(url_nominator__id__iexact=nominator.id,
+                                                            url_project=project,
+                                                            entity__iexact=form_data['url_value'],
+                                                            attribute__iexact='nomination',
+                                                            defaults={
+                                                                  'entity': form_data['url_value'],
+                                                                  'attribute': 'nomination',
+                                                                  'value': scope_value,
+                                                                  'url_nominator': nominator,
+                                                                 })
     except Exception:
-        try:
-            # Nominate URL
-            nomination_url = URL(entity=form_data['url_value'],
-                                 value=scope_value,
-                                 attribute='nomination',
-                                 url_project=project,
-                                 url_nominator=nominator)
-            nomination_url.save()
-        except Exception:
-            raise http.Http404
-        else:
-            summary_list.append('You have successfully nominated ' + form_data['url_value'])
+        raise http.Http404
+
+    if created:
+        summary_list.append('You have successfully nominated ' + form_data['url_value'])
     else:
         if nomination_url.value == scope_value:
             if scope_value == '1':
@@ -267,25 +274,23 @@ def save_attribute(project, nominator, form_data, summary_list, attribute_name,
     """
     try:
         # Check if URL attribute and value already exist
-        added_url = URL.objects.get(url_nominator=nominator,
-                                    url_project=project,
-                                    entity__iexact=form_data['url_value'],
-                                    value__iexact=valvar,
-                                    attribute__iexact=attribute_name)
-    except (URL.DoesNotExist, URL.MultipleObjectsReturned):
-        try:
-            added_url = URL(entity=form_data['url_value'],
-                            value=valvar,
-                            attribute=attribute_name,
-                            url_project=project,
-                            url_nominator=nominator)
-            added_url.save()
-        except Exception:
-            raise http.Http404
-        else:
-            summary_list.append('You have successfully added the '
-                                + attribute_name + ' \"' + valvar + '\" for '
-                                + form_data['url_value'])
+        added_url, created = URL.objects.get_or_create(url_nominator=nominator,
+                                                       url_project=project,
+                                                       entity__iexact=form_data['url_value'],
+                                                       value__iexact=valvar,
+                                                       attribute__iexact=attribute_name,
+                                                       defaults={
+                                                                 'entity': form_data['url_value'],
+                                                                 'value': valvar,
+                                                                 'attribute': attribute_name,
+                                                                })
+    except Exception:
+        raise http.Http404
+
+    if created:
+        summary_list.append('You have successfully added the '
+                            + attribute_name + ' \"' + valvar + '\" for '
+                            + form_data['url_value'])
     else:
         summary_list.append('You have already added the '
                             + attribute_name + ' \"' + valvar + '\" for '
@@ -297,17 +302,17 @@ def save_attribute(project, nominator, form_data, summary_list, attribute_name,
 def surt_exists(project, system_nominator, url_entity):
     # Create a SURT if the url doesn't already have one
     try:
-        URL.objects.get(url_project=project, entity__iexact=url_entity, attribute__iexact='surt')
-    except URL.DoesNotExist:
-        try:
-            new_surt = URL(entity=url_entity,
-                           value=surtize(url_entity),
-                           attribute='surt',
-                           url_project=project,
-                           url_nominator=system_nominator)
-            new_surt.save()
-        except Exception:
-            raise http.Http404
+        URL.objects.get_or_create(url_project=project,
+                                  entity__iexact=url_entity,
+                                  attribute__iexact='surt',
+                                  defaults={
+                                            'entity': url_entity,
+                                            'attribute': 'surt',
+                                            'value': surtize(url_entity),
+                                            'url_nominator': system_nominator
+                                           })
+    except Exception:
+        raise http.Http404
 
     return True
 
@@ -406,10 +411,7 @@ def create_json_browse(slug, url_attribute, root=''):
     json_list = []
 
     # Make sure the project exist in the database
-    try:
-        project = Project.objects.get(project_slug=slug)
-    except Project.DoesNotExist:
-        raise http.Http404
+    project = get_object_or_404(Project, project_slug=slug)
 
     if root != '':
         # Find all URLs with the project and domain specified
@@ -489,10 +491,7 @@ def create_json_browse(slug, url_attribute, root=''):
 
 def create_json_search(slug):
     """Create JSON list of all URLs added to the specified project."""
-    try:
-        project = Project.objects.get(project_slug=slug)
-    except Project.DoesNotExist:
-        raise http.Http404
+    project = get_object_or_404(Project, project_slug=slug)
 
     json_list = []
     query_list = (URL.objects.filter(url_project=project)
diff --git a/nomination/views.py b/nomination/views.py
index 85de2b7..3f9ed7b 100644
--- a/nomination/views.py
+++ b/nomination/views.py
@@ -481,6 +481,13 @@ def url_add(request, slug):
                     posted_data = handle_metadata(request, posted_data)
 
                     summary_list = add_url(project, posted_data)
+                    if not summary_list:
+                        return HttpResponse('There was a problem processing your nominator '
+                                            'details. Please contact {admin_email} for '
+                                            'assistance and provide the name, email address,'
+                                            ' and institution you are using for nominations.'
+                                            .format(admin_email=project.admin_email),
+                                            content_type='text/plain')
                     # send url value to provide metadata link
                     url_entity = posted_data['url_value']
                     # clear out posted data, so it is not sent back to form


Test patch:
diff --git a/tests/test_url_handler.py b/tests/test_url_handler.py
index 9b3877e..1da2af8 100644
--- a/tests/test_url_handler.py
+++ b/tests/test_url_handler.py
@@ -195,6 +195,8 @@ class TestAddMetadata():
         value = 'some_value'
         form_data = {
             'nominator_email': nominator.nominator_email,
+            'nominator_institution': nominator.nominator_institution,
+            'nominator_name': nominator.nominator_name,
             'scope': '1',
             'url_value': 'http://www.example.com',
             attribute_name: value
@@ -229,7 +231,11 @@ class TestGetNominator():
 
     def test_returns_nominator(self):
         nominator = factories.NominatorFactory()
-        form_data = {'nominator_email': nominator.nominator_email}
+        form_data = {
+            'nominator_email': nominator.nominator_email,
+            'nominator_name': nominator.nominator_name,
+            'nominator_institution': nominator.nominator_institution
+        }
         assert url_handler.get_nominator(form_data) == nominator
 
     def test_creates_and_returns_nominator(self):
@@ -251,9 +257,8 @@ class TestGetNominator():
             'nominator_name': None,
             'nominator_institution': None
         }
-        new_nominator = url_handler.get_nominator(form_data)
-
-        assert new_nominator is False
+        with pytest.raises(http.Http404):
+            url_handler.get_nominator(form_data)
 
 
 class TestNominateURL():