More problem operations (#523)

* Add deleting problems

* Refactor admin functions for multiple problems

* Fix splitting ids arguments into digits issue

* Add tests for deleting problems

* Add tests for passing invalid ids

* Use the built-in version of bootstrap

* re_path -> path

Author: kamilszymczak1

oioioi/contests/admin.py

@@ -382,25 +382,27 @@ class ProblemInstanceAdmin(admin.ModelAdmin):
     list_display = ('name_link', 'short_name_link', 'round', 'package', 'actions_field')
     readonly_fields = ('contest', 'problem')
     ordering = ('-round__start_date', 'short_name')
-    actions = ['attach_problems_to_another_contest', 'assign_problems_to_a_round']
+    actions = ['attach_problems_to_another_contest', 'assign_problems_to_a_round', 'delete_problems']
 
-    def attach_problems_to_another_contest(self, request, queryset):
+    def _attach_problem_ids_to_url(self, queryset, url_name):
+        """Helper function to create a URL with problem ids as query parameters."""
         ids = [problem.id for problem in queryset]
-
         # Attach problem ids as arguments to the URL
-        base_url = reverse('reattach_problem_contest_list')
+        base_url = reverse(url_name)
         query_string = urlencode({'ids': ','.join(str(i) for i in ids)}, doseq=True)
+        return '%s?%s' % (base_url, query_string)
 
-        return redirect('%s?%s' % (base_url, query_string))
+    def attach_problems_to_another_contest(self, request, queryset):
+        return redirect(
+            self._attach_problem_ids_to_url(queryset, 'reattach_problem_contest_list'))
 
     def assign_problems_to_a_round(self, request, queryset):
-        ids = [problem.id for problem in queryset]
-
-        # Attach problem ids as arguments to the URL
-        base_url = reverse('assign_problems_to_a_round')
-        query_string = urlencode({'ids': ','.join(str(i) for i in ids)}, doseq=True)
+        return redirect(
+            self._attach_problem_ids_to_url(queryset, 'assign_problems_to_a_round'))
 
-        return redirect('%s?%s' % (base_url, query_string))
+    def delete_problems(self, request, queryset):
+        return redirect(
+            self._attach_problem_ids_to_url(queryset, 'delete_problems'))
 
     def __init__(self, *args, **kwargs):
         # creating a thread local variable to store the request

oioioi/contests/fixtures/test_problem_instance_with_and_without_contests.json

@@ -0,0 +1,21 @@
+[
+  {
+    "pk": 10,
+    "model": "contests.probleminstance",
+    "fields": {
+      "problem": 1,
+      "round": 2,
+      "short_name": "zad1",
+      "contest": "c1"
+    }
+  },
+  {
+    "pk": 20,
+    "model": "contests.probleminstance",
+    "fields": {
+      "problem": 1,
+      "round": 3,
+      "short_name": "zad2"
+    }
+  }
+]

oioioi/contests/fixtures/test_problem_with_long_id.json

@@ -0,0 +1,12 @@
+[
+  {
+    "pk": 12345,
+    "model": "contests.probleminstance",
+    "fields": {
+      "problem": 1,
+      "round": 1,
+      "short_name": "zad2",
+      "contest": "c"
+    }
+  }
+]

oioioi/contests/templates/contests/delete_problems_confirm.html

@@ -0,0 +1,48 @@
+{% extends "base-with-menu.html" %}
+{% load i18n %}
+
+{% block title %}{% trans "Confirm deletion" %}{% endblock %}
+
+{% block main-content %}
+
+<h1>{% trans "Confirm deletion" %}</h1>
+<p>
+    {% blocktrans %}
+    You are going to delete the following problems from the contest
+    {% endblocktrans %}
+    <strong> {{ contest.name }} </strong>
+<ul class="list-group d-inline-block">
+    {% for problem in problem_instances %}
+    <li class="list-group-item">{{ problem }}</li>
+    {% endfor %}
+</ul>
+</p>
+
+<button class="btn btn-danger mt-3" data-toggle="modal" data-target="#confirmDeletionModal">
+    {% trans "Delete" %}
+</button>
+<button class="btn btn-outline-secondary mt-3" onclick="history.back()">
+    {% trans "No, go back" %}
+</button>
+ 
+<div class="modal fade" id="confirmDeletionModal" tabindex="-1" aria-labelledby="confirmDeletionLabel" aria-hidden="true">
+  <div class="modal-dialog">
+    <div class="modal-content">
+      <form method="post">
+        {% csrf_token %}
+        <div class="modal-header">
+          <h5 class="modal-title" id="confirmDeletionLabel">{% trans "Confirm Deletion" %}</h5>
+        </div>
+        <div class="modal-body">
+          {% blocktrans %}Are you sure you want to delete the selected problems? This action cannot be undone.{% endblocktrans %}
+        </div>
+        <div class="modal-footer">
+          <button type="button" class="btn btn-secondary" data-dismiss="modal">{% trans "Cancel" %}</button>
+          <button type="submit" class="btn btn-danger">{% trans "Confirm Deletion" %}</button>
+        </div>
+      </form>
+    </div>
+  </div>
+</div>
+{% endblock %}
+

oioioi/contests/tests/tests.py

@@ -3213,6 +3213,7 @@ class TestReattachingProblems(TestCase):
         'test_extra_problem',
         'test_permissions',
         'test_problem_site',
+        'test_problem_with_long_id'
     ]
 
     def test_reattaching_problem(self):
@@ -3237,10 +3238,12 @@ def test_reattaching_problem(self):
         self.assertContains(response, u'Sum\u017cyce')
         self.assertContains(response, "Attach")
 
+        prev_problem_count = ProblemInstance.objects.count()
+
         response = self.client.post(url, data={'submit': True}, follow=True)
         self.assertEqual(response.status_code, 200)
         self.assertContains(response, 'c2')
-        self.assertEqual(ProblemInstance.objects.count(), 3)
+        self.assertEqual(ProblemInstance.objects.count(), prev_problem_count + 1)
         self.assertContains(response, ' added successfully.')
         self.assertContains(response, u'Sum\u017cyce')
         self.assertTrue(ProblemInstance.objects.filter(contest__id='c2').exists())
@@ -3279,10 +3282,12 @@ def test_reattaching_problems(self):
         self.assertContains(response, u'Sum\u017cyce')
         self.assertContains(response, "Attach")
 
+        prev_problem_count = ProblemInstance.objects.count()
+
         response = self.client.post(url, data={'submit': True}, follow=True)
         self.assertEqual(response.status_code, 200)
         self.assertContains(response, 'c2')
-        self.assertEqual(ProblemInstance.objects.count(), 4)
+        self.assertEqual(ProblemInstance.objects.count(), prev_problem_count + 2)
         self.assertContains(response, ' added successfully.')
         self.assertContains(response, u'Sum\u017cyce')
         self.assertTrue(ProblemInstance.objects.filter(contest__id='c2').exists())
@@ -3312,6 +3317,37 @@ def test_permissions(self):
             response = self.client.get(url)
             self.assertEqual(response.status_code, 403)
 
+    # Makes sure that ids are correctly forwarded in links from
+    # reattach_problem_contest_list to reattach_problem_confirm.
+    def test_correctly_passing_problem_ids(self):
+        self.assertTrue(self.client.login(username='test_admin'))
+        pi_id1 = ProblemInstance.objects.get(id=1).id
+        pi_id2 = ProblemInstance.objects.get(id=12345).id
+
+        self.client.get('/c/c/')  # 'c' becomes the current contest
+
+        url = reverse('reattach_problem_contest_list') + "?ids={}".format(pi_id1)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "c/c/reattach/c/confirm/?ids=1")
+
+        url = reverse('reattach_problem_contest_list') + "?ids={}".format(pi_id2)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "c/c/reattach/c/confirm/?ids=12345")
+
+        url = reverse('reattach_problem_contest_list') + "?ids={},{}".format(pi_id1, pi_id2)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "c/c/reattach/c/confirm/?ids=1%2C12345")
+
+        url = reverse('reattach_problem_contest_list') + "?ids={},{}".format(pi_id2, pi_id1)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "c/c/reattach/c/confirm/?ids=12345%2C1")
+
+
+
 # Testing whether a user with admin permission for one contest
 # can manage problems belonging to another contest using
 # the problem manager interface.
@@ -3332,6 +3368,7 @@ def test_managing(self):
                 reverse('reattach_problem_contest_list') + "?ids={}".format(problem_id),
                 reverse('reattach_problem_confirm', args=('available_contest',)) + "?ids={}".format(problem_id),
                 reverse('assign_problems_to_a_round') + "?ids={}".format(problem_id),
+                reverse('delete_problems') + "?ids={}".format(problem_id),
             ]
 
             for url in get_urls:
@@ -3341,6 +3378,7 @@ def test_managing(self):
             post_urls_and_data = [
                 (reverse('reattach_problem_confirm', args=('available_contest',)) + "?ids={}".format(problem_id), {}),
                 (reverse('assign_problems_to_a_round') + "?ids={}".format(problem_id), {'round': 100}),
+                (reverse('delete_problems') + "?ids={}".format(problem_id), {}),
             ]
 
             for url, data in post_urls_and_data:
@@ -3554,6 +3592,82 @@ def test_permissions(self):
             Contest.objects.get(id='one-round').id
         )
 
+class TestDeletingProblems(TestCase):
+    fixtures = [
+        'test_users',
+        'test_contest',
+        'test_full_package',
+        'test_problem_instance',
+        'test_extra_problem',
+        'test_permissions',
+        'test_problem_site',
+        'test_extra_contests',
+        'test_problem_instance_with_and_without_contests',
+    ]
+
+    def test_deleting_problem(self):
+        pi_id = ProblemInstance.objects.get(id=1).id
+
+        self.assertTrue(self.client.login(username='test_admin'))
+        self.client.get('/c/c/')  # 'c' becomes the current contest
+
+        url = reverse('delete_problems') + "?ids={}".format(pi_id)
+        response = self.client.get(url, follow=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Are you sure you want to delete the selected problems?")
+        self.assertContains(response, ProblemInstance.objects.get(id=pi_id).problem.name)
+
+        response = self.client.post(url, data={}, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Problems deleted successfully")
+        self.assertFalse(ProblemInstance.objects.filter(id=pi_id).exists())
+
+    def test_deleting_problems(self):
+        pi_id1 = ProblemInstance.objects.get(id=1).id
+        pi_id2 = ProblemInstance.objects.get(id=2).id
+
+        self.assertTrue(self.client.login(username='test_admin'))
+        self.client.get('/c/c/')  # 'c' becomes the current contest
+
+        url = reverse('delete_problems') + "?ids={}%2C{}".format(pi_id1, pi_id2)
+        response = self.client.get(url, follow=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Are you sure you want to delete the selected problems?")
+        self.assertContains(response, ProblemInstance.objects.get(id=pi_id1).problem.name)
+        self.assertContains(response, ProblemInstance.objects.get(id=pi_id2).problem.name)
+
+        response = self.client.post(url, data={}, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Problems deleted successfully")
+        self.assertFalse(ProblemInstance.objects.filter(id=pi_id1).exists())
+        self.assertFalse(ProblemInstance.objects.filter(id=pi_id2).exists())
+
+    def test_bad_problem_ids(self):
+        self.assertTrue(self.client.login(username='test_admin'))
+
+        self.client.get('/c/c/')  # 'c' becomes the current contest
+
+        # Non-existent problem id
+        url = reverse('assign_problems_to_a_round') + "?ids={}".format(30)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 400)
+
+        # Non-numeric problem id
+        url = reverse('assign_problems_to_a_round') + "?ids=A,30"
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 400)
+
+        # ProblemInstance which belongs to another contest
+        url = reverse('assign_problems_to_a_round') + "?ids={}".format(10)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 400)
+
+        # ProblemInstance which does not belong to any contest
+        url = reverse('assign_problems_to_a_round') + "?ids={}".format(20)
+        response = self.client.get(url, follow=True)
+        self.assertEqual(response.status_code, 400)
 
 class TestModifyContest(TestCase):
     fixtures = ['test_users']

oioioi/contests/urls.py

@@ -158,10 +158,13 @@ def glob_namespaced_patterns(namespace):
     re_path(r'^admin/', admin.contest_site.urls),
     path('archive/confirm', views.confirm_archive_contest, name='confirm_archive_contest'),
     path('unarchive/', views.unarchive_contest, name='unarchive_contest'),
-    re_path(
-        r'^assign_problems_to_a_round/$',
+    path('assign_problems_to_a_round/',
         views.assign_problems_to_a_round_view,
         name='assign_problems_to_a_round',
+    ),
+    path('delete_problems/',
+        views.delete_problems_confirm_view,
+        name='delete_problems',
     )
 ]
 

oioioi/contests/views.py

@@ -866,7 +866,7 @@ def reattach_problem_contest_list_view(request, full_list=False):
             'problem_instances': problem_instances,
             'contest_list': contests,
             'full_list': full_list,
-            'problem_ids': '%2C'.join(str(i) for i in problem_ids), # Separate the problem ids with a comma (%2C)
+            'problem_ids': '%2C'.join(str(i) for i in problem_ids.split(',')), # Separate the problem ids with a comma (%2C)
         },
     )
 
@@ -946,6 +946,7 @@ def assign_problems_to_a_round_view(request):
     if not request.contest:
         raise SuspiciousOperation("Invalid contest")
 
+    # Check if the problem instances belong to the contest in the request
     if not _check_if_problem_instances_belong_to_contest(problem_instances, request.contest.id):
         raise SuspiciousOperation("Invalid problem instances")
 
@@ -954,10 +955,6 @@ def assign_problems_to_a_round_view(request):
         messages.error(request, _("The contest has no rounds."))
         return redirect('oioioiadmin:contests_probleminstance_changelist')
 
-    # Check if the problem instances belong to the contest in the request
-    if not all(pi.contest and pi.contest.id == request.contest.id for pi in problem_instances):
-        raise SuspiciousOperation("Invalid problem instances")
-
     if request.method == 'POST':
         form = RoundSelectionForm(request.POST, contest=request.contest)
         # Round is optional in the form, so we need to check if it is selected
@@ -993,6 +990,53 @@ def assign_problems_to_a_round_view(request):
         },
     )
 
+@enforce_condition(contest_exists & is_contest_basicadmin)
+def delete_problems_confirm_view(request):
+    """
+    Handles the assignment of problem instances to a specific round within a contest.
+    This view retrieves problem instances based on the provided IDs in the request,
+    validates their association with the current contest, and allows the user to assign
+    them to a specific round within the contest. If the assignment is successful, the
+    user is redirected to the problem instance changelist page.
+    Args:
+        request (HttpRequest): The HTTP request object containing GET or POST data.
+    Raises:
+        SuspiciousOperation: If the contest in the request is invalid, or if the problem
+                             instances or selected round do not belong to the contest.
+    """
+
+    problem_ids = request.GET.get('ids')
+
+    # Get the problems instances from the request
+    problem_instances = _get_problem_instances_from_problem_ids(problem_ids)
+
+    if not request.contest:
+        raise SuspiciousOperation("Invalid contest")
+
+    if not _check_if_problem_instances_belong_to_contest(problem_instances, request.contest.id):
+        raise SuspiciousOperation("Invalid problem instances")
+
+    if request.method == 'POST':
+        for problem_instance in problem_instances:
+            problem_instance.delete()
+        messages.success(request, _("Problems deleted successfully."))
+        return safe_redirect(
+            request,
+            reverse(
+                'oioioiadmin:contests_probleminstance_changelist',
+                kwargs={'contest_id': request.contest.id},
+            ),
+        )
+
+    return TemplateResponse(
+        request,
+        'contests/delete_problems_confirm.html',
+        {
+            'problem_instances': problem_instances,
+        },
+    )
+
+
 @enforce_condition(contest_exists & is_contest_basicadmin)
 def confirm_archive_contest(request):
     if request.method == 'POST':