From 4b987436c05b9565384a2cb8052d33bb336342ec Mon Sep 17 00:00:00 2001 From: Ole Laursen Date: Wed, 19 Oct 2016 11:58:49 +0000 Subject: [PATCH] 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 --- ietf/doc/tests_review.py | 80 +++++++++++++++++++------------------- ietf/group/tests_review.py | 42 +++++++++++--------- ietf/ietfauth/tests.py | 8 ++-- ietf/stats/tests.py | 10 ++++- ietf/stats/views.py | 5 ++- ietf/utils/test_data.py | 22 +++++++---- 6 files changed, 95 insertions(+), 72 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 9099d2fba..97a2c5a01 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -62,7 +62,7 @@ class ReviewTests(TestCase): "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 @@ class ReviewTests(TestCase): # 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): def test_assign_reviewer(self): doc = make_test_data() - # set up some reviewer-suitability factors - plain_email = Email.objects.filter(person__user__username="plain").first() - DocumentAuthor.objects.create( - author=plain_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() + # set up some reviewer-suitability factors + reviewer_email = Email.objects.get(person__user__username="reviewer") + DocumentAuthor.objects.create( + 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")]) + # previous review ReviewRequest.objects.create( time=datetime.datetime.now() - datetime.timedelta(days=100), @@ -250,29 +250,29 @@ class ReviewTests(TestCase): 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 }) @@ -280,25 +280,25 @@ class ReviewTests(TestCase): # 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): # 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 8ec938d83..0d5e90a92 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -114,7 +114,7 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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")) diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index b3ede910c..7bab9f340 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -342,12 +342,12 @@ class IetfAuthTests(TestCase): 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, diff --git a/ietf/stats/tests.py b/ietf/stats/tests.py index bd66946dd..5852f3577 100644 --- a/ietf/stats/tests.py +++ b/ietf/stats/tests.py @@ -21,11 +21,19 @@ class StatisticsTests(TestCase): 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) diff --git a/ietf/stats/views.py b/ietf/stats/views.py index 348ffc496..e2c3956b8 100644 --- a/ietf/stats/views.py +++ b/ietf/stats/views.py @@ -3,7 +3,7 @@ import datetime, itertools, json, calendar 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 review_stats(request, stats_type=None, acronym=None): 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: diff --git a/ietf/utils/test_data.py b/ietf/utils/test_data.py index 12189d8cb..ce9895d6f 100644 --- a/ietf/utils/test_data.py +++ b/ietf/utils/test_data.py @@ -401,10 +401,14 @@ def make_review_data(doc): for t in ReviewTypeName.objects.filter(slug__in=["early", "lc", "telechat"]): TypeUsedInReviewTeam.objects.create(team=team, type=t) - p = Person.objects.get(user__username="plain") - email = p.email_set.first() - Role.objects.create(name_id="reviewer", person=p, email=email, group=team) - ReviewerSettings.objects.create(team=team, person=p, min_interval=14, skip_next=0) + u = User.objects.create(username="reviewer") + u.set_password("reviewer+password") + u.save() + reviewer = Person.objects.create(name=u"Some Réviewer", ascii="Some Reviewer", user=u) + email = Email.objects.create(address="reviewer@example.com", person=reviewer) + + Role.objects.create(name_id="reviewer", person=reviewer, email=email, group=team) + ReviewerSettings.objects.create(team=team, person=reviewer, min_interval=14, skip_next=0) review_req = ReviewRequest.objects.create( doc=doc, @@ -412,15 +416,19 @@ def make_review_data(doc): type_id="early", deadline=datetime.datetime.now() + datetime.timedelta(days=20), state_id="accepted", - requested_by=p, + requested_by=reviewer, reviewer=email, ) p = Person.objects.get(user__username="marschairman") Role.objects.create(name_id="reviewer", person=p, email=p.email_set.first(), group=team) - p = Person.objects.get(user__username="secretary") - Role.objects.create(name_id="secr", person=p, email=p.email_set.first(), group=team) + u = User.objects.create(username="reviewsecretary") + u.set_password("reviewsecretary+password") + u.save() + reviewsecretary = Person.objects.create(name=u"Réview Secretary", ascii="Review Secretary", user=u) + reviewsecretary_email = Email.objects.create(address="reviewsecretary@example.com", person=reviewsecretary) + Role.objects.create(name_id="secr", person=reviewsecretary, email=reviewsecretary_email, group=team) return review_req