Fix a missing HttpResponseForbidden in review statistics, make the

review test code use a separate reviewer and reviewsecretary user to
avoid confounding things - also let these use Unicode in their names
to check for Unicode trouble.
 - Legacy-Id: 12175

Author: OleLaursen

ietf/doc/tests_review.py

@@ -62,7 +62,7 @@ def test_request_review(self):
             "team": review_team.pk,
             "deadline": deadline.isoformat(),
             "requested_rev": "01",
-            "requested_by": Person.objects.get(user__username="plain").pk,
+            "requested_by": Person.objects.get(user__username="reviewsecretary").pk,
         })
         self.assertEqual(r.status_code, 302)
 
@@ -112,14 +112,14 @@ def test_close_request(self):
 
         # follow link
         req_url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
-        self.client.login(username="secretary", password="secretary+password")
+        self.client.login(username="reviewsecretary", password="reviewsecretary+password")
         r = self.client.get(req_url)
         self.assertEqual(r.status_code, 200)
         self.assertTrue(close_url in unicontent(r))
         self.client.logout()
 
         # get close page
-        login_testing_unauthorized(self, "secretary", close_url)
+        login_testing_unauthorized(self, "reviewsecretary", close_url)
         r = self.client.get(close_url)
         self.assertEqual(r.status_code, 200)
 
@@ -134,7 +134,7 @@ def test_close_request(self):
         self.assertEqual(e.type, "closed_review_request")
         self.assertTrue("closed" in e.desc.lower())
         self.assertEqual(len(outbox), 1)
-        self.assertTrue("closed" in unicode(outbox[0]).lower())
+        self.assertTrue("closed" in outbox[0].get_payload(decode=True).decode("utf-8").lower())
 
     def test_possibly_advance_next_reviewer_for_team(self):
         doc = make_test_data()
@@ -225,21 +225,21 @@ def get_skip_next(person):
     def test_assign_reviewer(self):
         doc = make_test_data()
 
+        # review to assign to
+        review_req = make_review_data(doc)
+        review_req.state = ReviewRequestStateName.objects.get(slug="requested")
+        review_req.reviewer = None
+        review_req.save()
+
         # set up some reviewer-suitability factors
-        plain_email = Email.objects.filter(person__user__username="plain").first()
+        reviewer_email = Email.objects.get(person__user__username="reviewer")
         DocumentAuthor.objects.create(
-            author=plain_email,
+            author=reviewer_email,
             document=doc,
         )
         doc.rev = "10"
         doc.save_with_history([DocEvent.objects.create(doc=doc, type="changed_document", by=Person.objects.get(user__username="secretary"), desc="Test")])
 
-        # review to assign to
-        review_req = make_review_data(doc)
-        review_req.state = ReviewRequestStateName.objects.get(slug="requested")
-        review_req.reviewer = None
-        review_req.save()
-
         # previous review
         ReviewRequest.objects.create(
             time=datetime.datetime.now() - datetime.timedelta(days=100),
@@ -250,55 +250,55 @@ def test_assign_reviewer(self):
             state=ReviewRequestStateName.objects.get(slug="completed"),
             reviewed_rev="01",
             deadline=datetime.date.today() - datetime.timedelta(days=80),
-            reviewer=plain_email,
+            reviewer=reviewer_email,
         )
 
-        reviewer_settings = ReviewerSettings.objects.get(person__email=plain_email, team=review_req.team)
+        reviewer_settings = ReviewerSettings.objects.get(person__email=reviewer_email, team=review_req.team)
         reviewer_settings.filter_re = doc.name
         reviewer_settings.skip_next = 1
         reviewer_settings.save()
 
         UnavailablePeriod.objects.create(
             team=review_req.team,
-            person=plain_email.person,
+            person=reviewer_email.person,
             start_date=datetime.date.today() - datetime.timedelta(days=10),
             availability="unavailable",
         )
 
-        ReviewWish.objects.create(person=plain_email.person, team=review_req.team, doc=doc)
+        ReviewWish.objects.create(person=reviewer_email.person, team=review_req.team, doc=doc)
 
         # pick a non-existing reviewer as next to see that we can
         # handle reviewers who have left
         NextReviewerInTeam.objects.filter(team=review_req.team).delete()
         NextReviewerInTeam.objects.create(
             team=review_req.team,
-            next_reviewer=Person.objects.exclude(pk=plain_email.person_id).first(),
+            next_reviewer=Person.objects.exclude(pk=reviewer_email.person_id).first(),
         )
 
         assign_url = urlreverse('ietf.doc.views_review.assign_reviewer', kwargs={ "name": doc.name, "request_id": review_req.pk })
 
 
         # follow link
         req_url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
-        self.client.login(username="secretary", password="secretary+password")
+        self.client.login(username="reviewsecretary", password="reviewsecretary+password")
         r = self.client.get(req_url)
         self.assertEqual(r.status_code, 200)
         self.assertTrue(assign_url in unicontent(r))
         self.client.logout()
 
         # get assign page
-        login_testing_unauthorized(self, "secretary", assign_url)
+        login_testing_unauthorized(self, "reviewsecretary", assign_url)
         r = self.client.get(assign_url)
         self.assertEqual(r.status_code, 200)
         q = PyQuery(r.content)
-        plain_label = q("option[value=\"{}\"]".format(plain_email.address)).text().lower()
-        self.assertIn("reviewed document before", plain_label)
-        self.assertIn("wishes to review", plain_label)
-        self.assertIn("is author", plain_label)
-        self.assertIn("regexp matches", plain_label)
-        self.assertIn("unavailable indefinitely", plain_label)
-        self.assertIn("skip next 1", plain_label)
-        self.assertIn("#1", plain_label)
+        reviewer_label = q("option[value=\"{}\"]".format(reviewer_email.address)).text().lower()
+        self.assertIn("reviewed document before", reviewer_label)
+        self.assertIn("wishes to review", reviewer_label)
+        self.assertIn("is author", reviewer_label)
+        self.assertIn("regexp matches", reviewer_label)
+        self.assertIn("unavailable indefinitely", reviewer_label)
+        self.assertIn("skip next 1", reviewer_label)
+        self.assertIn("#1", reviewer_label)
 
         # assign
         empty_outbox()
@@ -311,7 +311,7 @@ def test_assign_reviewer(self):
         self.assertEqual(review_req.state_id, "requested")
         self.assertEqual(review_req.reviewer, reviewer)
         self.assertEqual(len(outbox), 1)
-        self.assertTrue("assigned" in unicode(outbox[0]))
+        self.assertTrue("assigned" in outbox[0].get_payload(decode=True).decode("utf-8"))
         self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
 
         # re-assign
@@ -326,8 +326,8 @@ def test_assign_reviewer(self):
         self.assertEqual(review_req.state_id, "requested") # check that state is reset
         self.assertEqual(review_req.reviewer, reviewer)
         self.assertEqual(len(outbox), 2)
-        self.assertTrue("cancelled your assignment" in unicode(outbox[0]))
-        self.assertTrue("assigned" in unicode(outbox[1]))
+        self.assertTrue("cancelled your assignment" in outbox[0].get_payload(decode=True).decode("utf-8"))
+        self.assertTrue("assigned" in outbox[1].get_payload(decode=True).decode("utf-8"))
 
     def test_accept_reviewer_assignment(self):
         doc = make_test_data()
@@ -361,14 +361,14 @@ def test_reject_reviewer_assignment(self):
 
         # follow link
         req_url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
-        self.client.login(username="secretary", password="secretary+password")
+        self.client.login(username="reviewsecretary", password="reviewsecretary+password")
         r = self.client.get(req_url)
         self.assertEqual(r.status_code, 200)
         self.assertTrue(reject_url in unicontent(r))
         self.client.logout()
 
         # get reject page
-        login_testing_unauthorized(self, "secretary", reject_url)
+        login_testing_unauthorized(self, "reviewsecretary", reject_url)
         r = self.client.get(reject_url)
         self.assertEqual(r.status_code, 200)
         self.assertTrue(unicode(review_req.reviewer.person) in unicontent(r))
@@ -385,7 +385,7 @@ def test_reject_reviewer_assignment(self):
         self.assertTrue("rejected" in e.desc)
         self.assertEqual(ReviewRequest.objects.filter(doc=review_req.doc, team=review_req.team, state="requested").count(), 1)
         self.assertEqual(len(outbox), 1)
-        self.assertTrue("Test message" in unicode(outbox[0]))
+        self.assertTrue("Test message" in outbox[0].get_payload(decode=True).decode("utf-8"))
 
     def make_test_mbox_tarball(self, review_req):
         mbox_path = os.path.join(self.review_dir, "testmbox.tar.gz")
@@ -444,7 +444,7 @@ def test_search_mail_archive(self):
             ietf.review.mailarch.construct_query_urls = lambda review_req, query=None: { "query_data_url": "file://" + os.path.abspath(mbox_path) }
 
             url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "request_id": review_req.pk })
-            login_testing_unauthorized(self, "secretary", url)
+            login_testing_unauthorized(self, "reviewsecretary", url)
 
             r = self.client.get(url)
             self.assertEqual(r.status_code, 200)
@@ -534,7 +534,7 @@ def test_complete_review_upload_content(self):
 
         self.assertEqual(len(outbox), 1)
         self.assertTrue(review_req.team.list_email in outbox[0]["To"])
-        self.assertTrue("This is a review" in unicode(outbox[0]))
+        self.assertTrue("This is a review" in outbox[0].get_payload(decode=True).decode("utf-8"))
 
         self.assertTrue(settings.MAILING_LIST_ARCHIVE_URL in review_req.review.external_url)
 
@@ -574,7 +574,7 @@ def test_complete_review_enter_content(self):
 
         self.assertEqual(len(outbox), 1)
         self.assertTrue(review_req.team.list_email in outbox[0]["To"])
-        self.assertTrue("This is a review" in unicode(outbox[0]))
+        self.assertTrue("This is a review" in outbox[0].get_payload(decode=True).decode("utf-8"))
 
         self.assertTrue(settings.MAILING_LIST_ARCHIVE_URL in review_req.review.external_url)
 
@@ -628,13 +628,13 @@ def test_partially_complete_review(self):
         self.assertTrue(review_req.doc.rev in review_req.review.name)
 
         self.assertEqual(len(outbox), 2)
-        self.assertTrue("secretary" in outbox[0]["To"])
+        self.assertTrue("reviewsecretary@example.com" in outbox[0]["To"])
         self.assertTrue("partially" in outbox[0]["Subject"].lower())
-        self.assertTrue("new review request" in unicode(outbox[0]))
+        self.assertTrue("new review request" in outbox[0].get_payload(decode=True).decode("utf-8"))
 
         self.assertTrue(review_req.team.list_email in outbox[1]["To"])
         self.assertTrue("partial review" in outbox[1]["Subject"].lower())
-        self.assertTrue("This is a review" in unicode(outbox[1]))
+        self.assertTrue("This is a review" in outbox[1].get_payload(decode=True).decode("utf-8"))
 
         first_review = review_req.review
         first_reviewer = review_req.reviewer

ietf/group/tests_review.py

@@ -114,7 +114,7 @@ def test_reviewer_overview(self):
             deadline=datetime.date.today() + datetime.timedelta(days=30),
             state_id="accepted",
             reviewer=review_req1.reviewer,
-            requested_by=Person.objects.get(user__username="plain"),
+            requested_by=Person.objects.get(user__username="reviewer"),
         )
 
         UnavailablePeriod.objects.create(
@@ -161,7 +161,7 @@ def test_manage_review_requests(self):
             deadline=datetime.date.today() + datetime.timedelta(days=30),
             state_id="accepted",
             reviewer=review_req1.reviewer,
-            requested_by=Person.objects.get(user__username="plain"),
+            requested_by=Person.objects.get(user__username="reviewer"),
         )
 
         review_req3 = ReviewRequest.objects.create(
@@ -170,7 +170,7 @@ def test_manage_review_requests(self):
             type_id="early",
             deadline=datetime.date.today() + datetime.timedelta(days=30),
             state_id="requested",
-            requested_by=Person.objects.get(user__username="plain"),
+            requested_by=Person.objects.get(user__username="reviewer"),
         )
 
         # previous reviews
@@ -290,17 +290,17 @@ def test_email_open_review_assignments(self):
         self.assertEqual(len(outbox), 1)
         self.assertTrue(group.list_email in outbox[0]["To"])
         self.assertEqual(outbox[0]["subject"], "Test subject")
-        self.assertTrue("Test body" in unicode(outbox[0]))
+        self.assertTrue("Test body" in outbox[0].get_payload(decode=True).decode("utf-8"))
 
     def test_change_reviewer_settings(self):
         doc = make_test_data()
 
-        reviewer = Person.objects.get(name="Plain Man")
-
         review_req = make_review_data(doc)
-        review_req.reviewer = reviewer.email_set.first()
+        review_req.reviewer = Email.objects.get(person__user__username="reviewer")
         review_req.save()
-        
+
+        reviewer = review_req.reviewer.person
+
         url = urlreverse(ietf.group.views_review.change_reviewer_settings, kwargs={
             "acronym": review_req.team.acronym,
             "reviewer_email": review_req.reviewer_id,
@@ -335,8 +335,9 @@ def test_change_reviewer_settings(self):
         self.assertEqual(settings.remind_days_before_deadline, 6)
         self.assertEqual(len(outbox), 1)
         self.assertTrue("reviewer availability" in outbox[0]["subject"].lower())
-        self.assertTrue("frequency changed", unicode(outbox[0]).lower())
-        self.assertTrue("skip next", unicode(outbox[0]).lower())
+        msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
+        self.assertTrue("frequency changed", msg_content)
+        self.assertTrue("skip next", msg_content)
 
         # add unavailable period
         start_date = datetime.date.today() + datetime.timedelta(days=10)
@@ -352,8 +353,9 @@ def test_change_reviewer_settings(self):
         self.assertEqual(period.end_date, None)
         self.assertEqual(period.availability, "unavailable")
         self.assertEqual(len(outbox), 1)
-        self.assertTrue(start_date.isoformat(), unicode(outbox[0]).lower())
-        self.assertTrue("indefinite", unicode(outbox[0]).lower())
+        msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
+        self.assertTrue(start_date.isoformat(), msg_content)
+        self.assertTrue("indefinite", msg_content)
 
         # end unavailable period
         empty_outbox()
@@ -367,8 +369,9 @@ def test_change_reviewer_settings(self):
         period = reload_db_objects(period)
         self.assertEqual(period.end_date, end_date)
         self.assertEqual(len(outbox), 1)
-        self.assertTrue(start_date.isoformat(), unicode(outbox[0]).lower())
-        self.assertTrue("indefinite", unicode(outbox[0]).lower())
+        msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
+        self.assertTrue(start_date.isoformat(), msg_content)
+        self.assertTrue("indefinite", msg_content)
 
         # delete unavailable period
         empty_outbox()
@@ -379,16 +382,17 @@ def test_change_reviewer_settings(self):
         self.assertEqual(r.status_code, 302)
         self.assertEqual(UnavailablePeriod.objects.filter(person=reviewer, team=review_req.team, start_date=start_date).count(), 0)
         self.assertEqual(len(outbox), 1)
-        self.assertTrue(start_date.isoformat(), unicode(outbox[0]).lower())
-        self.assertTrue(end_date.isoformat(), unicode(outbox[0]).lower())
+        msg_content = outbox[0].get_payload(decode=True).decode("utf-8").lower()
+        self.assertTrue(start_date.isoformat(), msg_content)
+        self.assertTrue(end_date.isoformat(), msg_content)
 
     def test_reviewer_reminders(self):
         doc = make_test_data()
 
-        reviewer = Person.objects.get(name="Plain Man")
-
         review_req = make_review_data(doc)
 
+        reviewer = Person.objects.get(user__username="reviewer")
+
         settings = ReviewerSettings.objects.get(team=review_req.team, person=reviewer)
         settings.remind_days_before_deadline = 6
         settings.save()
@@ -411,4 +415,4 @@ def test_reviewer_reminders(self):
         empty_outbox()
         email_reviewer_reminder(review_req)
         self.assertEqual(len(outbox), 1)
-        self.assertTrue(review_req.doc_id in unicode(outbox[0]))
+        self.assertTrue(review_req.doc_id in outbox[0].get_payload(decode=True).decode("utf-8"))

ietf/ietfauth/tests.py

@@ -342,12 +342,12 @@ def test_reset_password(self):
     def test_review_overview(self):
         doc = make_test_data()
 
-        reviewer = Person.objects.get(name="Plain Man")
-
         review_req = make_review_data(doc)
-        review_req.reviewer = reviewer.email_set.first()
+        review_req.reviewer = Email.objects.get(person__user__username="reviewer")
         review_req.save()
-        
+
+        reviewer = review_req.reviewer.person
+
         UnavailablePeriod.objects.create(
             team=review_req.team,
             person=reviewer,

ietf/stats/tests.py

@@ -21,11 +21,19 @@ def test_review_stats(self):
 
         login_testing_unauthorized(self, "secretary", url)
 
+        completion_url = urlreverse(ietf.stats.views.review_stats, kwargs={ "stats_type": "completion" })
+
         r = self.client.get(url)
         self.assertEqual(r.status_code, 302)
-        self.assertTrue(urlreverse(ietf.stats.views.review_stats, kwargs={ "stats_type": "completion" }) in r["Location"])
+        self.assertTrue(completion_url in r["Location"])
+
+        self.client.logout()
+        self.client.login(username="plain", password="plain+password")
+        r = self.client.get(completion_url)
+        self.assertEqual(r.status_code, 403)
 
         # check tabular
+        self.client.login(username="secretary", password="secretary+password")
         for stats_type in ["completion", "results", "states"]:
             url = urlreverse(ietf.stats.views.review_stats, kwargs={ "stats_type": stats_type })
             r = self.client.get(url)

ietf/stats/views.py

@@ -3,7 +3,7 @@
 from django.shortcuts import render
 from django.contrib.auth.decorators import login_required
 from django.core.urlresolvers import reverse as urlreverse
-from django.http import HttpResponseRedirect
+from django.http import HttpResponseRedirect, HttpResponseForbidden
 
 import dateutil.relativedelta
 
@@ -134,6 +134,9 @@ def parse_date(s):
                 if not r.group_id in secr_access:
                     reviewer_only_access.add(r.group_id)
 
+        if not secr_access and not reviewer_only_access:
+            return HttpResponseForbidden("You do not have the necessary permissions to view this page")
+
         teams = [t for t in teams if t.pk in secr_access or t.pk in reviewer_only_access]
 
         for t in reviewer_only_access: