PMT #115881: Standardize Topics

econplayground/main/admin.py

@@ -1,7 +1,10 @@
 from django import forms
-from django.contrib import admin
+from django.contrib import admin, messages
 from django.db import models
 from ordered_model.admin import OrderedModelAdmin
+from django.db.models import ProtectedError
+from django.urls import reverse
+from django.http import HttpResponseRedirect
 from econplayground.main.models import (
     Graph, Assessment, AssessmentRule, Topic
 )
@@ -30,6 +33,43 @@ class AssessmentAdmin(admin.ModelAdmin):
 class TopicAdmin(OrderedModelAdmin):
     list_display = ('name', 'move_up_down_links')
 
+    # Below prevents the default topic from being deleted
+    # Its largely taken from:
+    # https://stackoverflow.com/questions/49326282
+    #       /django-admin-not-handling-protectederror-exception
+    def delete_view(self, request, object_id, extra_context=None):
+        try:
+            return super().delete_view(request, object_id, extra_context=None)
+        except ProtectedError:
+            msg = "{} can not be deleted." \
+                .format(self.model.objects.get(id=object_id).name)
+            self.message_user(request, msg, messages.ERROR)
+            opts = self.model._meta
+            return_url = reverse(
+                'admin:{}_{}_change'.format(opts.app_label, opts.model_name),
+                args=(object_id,),
+                current_app=self.admin_site.name,
+            )
+            return HttpResponseRedirect(return_url)
+
+    def response_action(self, request, queryset):
+        try:
+            return super().response_action(request, queryset)
+        except ProtectedError:
+            msg = "This object can not be deleted."
+            self.message_user(request, msg, messages.ERROR)
+            opts = self.model._meta
+            return_url = reverse(
+                'admin:{}_{}_change'.format(opts.app_label, opts.model_name),
+                current_app=self.admin_site.name,
+            )
+            return HttpResponseRedirect(return_url)
+
+    def has_delete_permission(self, request, obj=None):
+        return super().has_delete_permission(request, obj) and (
+            not obj or obj.id is not 1
+        )
+
 
 admin.site.register(Graph, GraphAdmin)
 admin.site.register(Assessment, AssessmentAdmin)

econplayground/main/migrations/0052_create_general_topic.py

@@ -0,0 +1,21 @@
+from django.db import migrations
+
+
+def create_general_topic(apps, schema_editor):
+    Graph = apps.get_model('main', 'Graph')
+    Topic = apps.get_model('main', 'Topic')
+
+    Graph.objects.all().update(topic=None)
+    Topic.objects.all().delete()
+    t = Topic.objects.create(name='General', pk=1, order=1)
+    Graph.objects.all().update(topic=t)
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('main', '0051_set_graph_order')
+    ]
+
+    operations = [
+        migrations.RunPython(create_general_topic),
+    ]

econplayground/main/migrations/0053_auto_20180710_1703.py

@@ -0,0 +1,19 @@
+# Generated by Django 2.0.7 on 2018-07-10 21:03
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('main', '0052_create_general_topic'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='graph',
+            name='topic',
+            field=models.ForeignKey(default=1, on_delete=django.db.models.deletion.PROTECT, to='main.Topic'),
+        ),
+    ]

econplayground/main/models.py

@@ -5,6 +5,9 @@
 from django.contrib.auth.models import User
 from django.db import models
 from ordered_model.models import OrderedModel
+from django.dispatch import receiver
+from django.db.models.signals import pre_delete
+from django.db.models import ProtectedError
 
 
 GRAPH_TYPES = (
@@ -46,6 +49,12 @@ def __str__(self):
         return self.name
 
 
+@receiver(pre_delete, sender=Topic)
+def default_topic_handler(sender, instance, **kwargs):
+    if instance.id is 1:
+        raise ProtectedError('The General topic can not be deleted', instance)
+
+
 class Graph(OrderedModel):
     class Meta(OrderedModel.Meta):
         pass
@@ -58,7 +67,7 @@ class Meta(OrderedModel.Meta):
     topic = models.ForeignKey(
         Topic,
         on_delete=models.PROTECT,
-        null=True, blank=True)
+        null=False, blank=False, default=1)
     featured = models.BooleanField(default=False)
     order_with_respect_to = ('featured')
     created_at = models.DateTimeField(auto_now_add=True)

econplayground/main/tests/test_models.py

@@ -1,6 +1,7 @@
 from django.test import TestCase
 from django.db.utils import IntegrityError
-from econplayground.main.models import Assessment, Graph
+from econplayground.main.models import Assessment, Graph, Topic
+from django.db.models import ProtectedError
 from econplayground.main.tests.factories import (
     GraphFactory, JXGLineFactory, JXGLineTransformationFactory,
     SubmissionFactory, TopicFactory,
@@ -84,6 +85,32 @@ def setUp(self):
     def test_is_valid_from_factory(self):
         self.x.full_clean()
 
+    def test_cant_delete_general(self):
+        # Verify that Topic.delete() won't work
+        t = Topic.objects.get(id=1)
+        self.assertRaises(ProtectedError, t.delete)
+
+    def test_cant_delete_general_qs(self):
+        # Verify that the QuerySet delete won't work
+        self.assertRaises(ProtectedError, Topic.objects.all().delete)
+
+    def test_can_delete_other_topics(self):
+        # Verify that Topic.delete() still works
+        t = Topic.objects.create(name='Topic', order=2)
+        try:
+            t.delete()
+        except ProtectedError:
+            self.fail('Topics with pk > 1 should be able to be deleted')
+
+    def test_can_delete_other_topics_qs(self):
+        # Verify that the QuerySet delete method still works
+        Topic.objects.create(name='topic 1', order=2)
+        Topic.objects.create(name='Topic 2', order=3)
+        try:
+            Topic.objects.exclude(pk=1).delete()
+        except ProtectedError:
+            self.fail('Topics with pk > 1 should be able to be deleted')
+
 
 class GraphOrderTest(TestCase):
     def test_bottom(self):

econplayground/main/tests/test_views.py

@@ -70,14 +70,13 @@ def test_get(self):
         # Context Data
         self.assertEqual(r.context['featured'], True)
         self.assertEqual(r.context['active_topic'], '')
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 5)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].graph_count(), 2)
         self.assertEqual(r.context['topic_list'][1].graph_count(), 2)
+        self.assertEqual(r.context['topic_list'][2].graph_count(), 2)
 
         r = self.client.get('/?all=true')
         self.assertEqual(r.status_code, 200)
@@ -90,16 +89,15 @@ def test_get(self):
         # Context Data
         self.assertEqual(r.context['featured'], False)
         self.assertEqual(r.context['active_topic'], '')
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 5)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].graph_count(), 2)
         self.assertEqual(r.context['topic_list'][1].graph_count(), 2)
+        self.assertEqual(r.context['topic_list'][2].graph_count(), 2)
 
-        r = self.client.get('/?topic=1')
+        r = self.client.get('/?topic=2')
         self.assertEqual(r.status_code, 200)
         # Graphs
         self.assertNotContains(r, 'Graph 1')
@@ -109,17 +107,16 @@ def test_get(self):
         self.assertNotContains(r, 'Draft graph')
         # Context Data
         self.assertEqual(r.context['featured'], False)
-        self.assertEqual(r.context['active_topic'], 1)
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['active_topic'], 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 5)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].graph_count(), 2)
         self.assertEqual(r.context['topic_list'][1].graph_count(), 2)
+        self.assertEqual(r.context['topic_list'][2].graph_count(), 2)
 
-        r = self.client.get('/?topic=2')
+        r = self.client.get('/?topic=3')
         self.assertEqual(r.status_code, 200)
         # Graphs
         self.assertNotContains(r, 'Graph 1')
@@ -129,15 +126,14 @@ def test_get(self):
         self.assertContains(r, 'Draft graph')
         # Context Data
         self.assertEqual(r.context['featured'], False)
-        self.assertEqual(r.context['active_topic'], 2)
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['active_topic'], 3)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 5)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].graph_count(), 2)
         self.assertEqual(r.context['topic_list'][1].graph_count(), 2)
+        self.assertEqual(r.context['topic_list'][2].graph_count(), 2)
 
 
 class GraphListStudentViewTest(LoggedInTestStudentMixin, TestCase):
@@ -165,14 +161,13 @@ def test_get(self):
         # Context Data
         self.assertEqual(r.context['featured'], True)
         self.assertEqual(r.context['active_topic'], '')
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 3)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].published_graph_count(), 1)
         self.assertEqual(r.context['topic_list'][1].published_graph_count(), 1)
+        self.assertEqual(r.context['topic_list'][2].published_graph_count(), 1)
 
         r = self.client.get('/?all=true')
         self.assertEqual(r.status_code, 200)
@@ -185,16 +180,15 @@ def test_get(self):
         # Context Data
         self.assertEqual(r.context['featured'], False)
         self.assertEqual(r.context['active_topic'], '')
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 3)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].published_graph_count(), 1)
         self.assertEqual(r.context['topic_list'][1].published_graph_count(), 1)
+        self.assertEqual(r.context['topic_list'][2].published_graph_count(), 1)
 
-        r = self.client.get('/?topic=1')
+        r = self.client.get('/?topic=2')
         self.assertEqual(r.status_code, 200)
         # Graphs
         self.assertNotContains(r, 'Graph 1')
@@ -204,17 +198,16 @@ def test_get(self):
         self.assertNotContains(r, 'Draft graph')
         # Context Data
         self.assertEqual(r.context['featured'], False)
-        self.assertEqual(r.context['active_topic'], 1)
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['active_topic'], 2)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 3)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].published_graph_count(), 1)
         self.assertEqual(r.context['topic_list'][1].published_graph_count(), 1)
+        self.assertEqual(r.context['topic_list'][2].published_graph_count(), 1)
 
-        r = self.client.get('/?topic=2')
+        r = self.client.get('/?topic=3')
         self.assertEqual(r.status_code, 200)
         # Graphs
         self.assertNotContains(r, 'Graph 1')
@@ -224,15 +217,14 @@ def test_get(self):
         self.assertNotContains(r, 'Draft graph')
         # Context Data
         self.assertEqual(r.context['featured'], False)
-        self.assertEqual(r.context['active_topic'], 2)
-        self.assertEqual(r.context['topic_list'].count(), 2)
+        self.assertEqual(r.context['active_topic'], 3)
+        self.assertEqual(r.context['topic_list'].count(), 3)
         self.assertContains(r, 'Topic A')
         self.assertContains(r, 'Topic B')
         self.assertEqual(r.context['all_count'], 3)
         self.assertEqual(r.context['featured_count'], 2)
-        self.assertEqual(r.context['graphs_without_topics'].count(), 1)
-        self.assertEqual(r.context['topic_list'][0].published_graph_count(), 1)
         self.assertEqual(r.context['topic_list'][1].published_graph_count(), 1)
+        self.assertEqual(r.context['topic_list'][2].published_graph_count(), 1)
 
 
 class MockLTI(object):

econplayground/main/views.py

@@ -139,7 +139,6 @@ def get_context_data(self, **kwargs):
         context['topic_list'] = Topic.objects.all()
         context['all_count'] = graphs.count()
         context['featured_count'] = graphs.filter(featured=True).count()
-        context['graphs_without_topics'] = graphs.filter(topic=None)
 
         # If there are no query string params, then set featured to true.
         # Set active_topic guard condition, and assign to an id if present in