launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01470
[Merge] lp:~sinzui/launchpad/poll-oops into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/poll-oops into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#46581 Change a poll type URL manually crashes
https://bugs.launchpad.net/bugs/46581
This is my branch to simplify the poll vote behaviour to prevent an oops.
lp:~sinzui/launchpad/poll-oops
Diff size: 324
Launchpad bug: https://bugs.launchpad.net/bugs/46581
Test command: ./bin/test -vv -t test_poll -t poll-view -t team-polls
Pre-implementation: no one
Target release: 10.11
I.
II. Simplify the poll vote behaviour to prevent an oops
---------------------------------------------------
Users who read the Launchpad code can discover that condorcet polls are
partially implemented but disabled. The user can create an oops by hacking
the URL to attempt to change the way voting works.
The crux of the problem is that there are separate names for the vote pages
which tempts users to try switching the vote mechanism. One name for voting
could be registered and the view can select the template for the poll type.
The original suggestion I made in the bug supposed that the poll_types were
implemented as interfaces--they are enums. The view knows the PollAlgorithm
of the poll and makes decisions based on it. The view can do the same for
the template.
Rules
-----
* Update PollVoteView to select the template based on the PollAlgorithm.
* Change the view name to +vote in configure.zcml and remove the templates
attrs.
* Find and replace +vote-simple and +vote-condorcet with +vote.
QA
--
* Visit an open poll you can vote on.
* Verify the URL to vote ends with +vote
Lint
----
Linting changed files:
lib/lp/registry/browser/configure.zcml
lib/lp/registry/browser/poll.py
lib/lp/registry/browser/tests/poll-views_0.txt
lib/lp/registry/browser/tests/test_poll.py
lib/lp/registry/stories/team-polls/edit-poll.txt
lib/lp/registry/stories/team-polls/vote-poll.txt
lib/lp/registry/stories/team-polls/xx-poll-condorcet-voting.txt
lib/lp/testing/factory.py
^ Lint reports indentation and heading issues in the doctests. I will fix
these after this branch is approved to land.
Test
----
Added tests to verify PollVoteView selects the correct template for +vote.
* lib/lp/registry/browser/tests/test_poll.py
Used find and replace to update existing tests to use _+vote.
* lib/lp/registry/browser/tests/poll-views_0.txt
* lib/lp/registry/stories/team-polls/edit-poll.txt
* lib/lp/registry/stories/team-polls/vote-poll.txt
* lib/lp/registry/stories/team-polls/xx-poll-condorcet-voting.txt
Implementation
--------------
Move the template selection rules into the view.
* lib/lp/registry/browser/configure.zcml
* lib/lp/registry/browser/poll.py
* Also simplified the redirect rule to redirect eligible users to vote.
Updated the factor to make testing easier.
* lib/lp/testing/factory.py
--
https://code.launchpad.net/~sinzui/launchpad/poll-oops/+merge/38142
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/poll-oops into lp:launchpad/devel.
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2010-10-04 20:46:55 +0000
+++ lib/lp/registry/browser/configure.zcml 2010-10-11 17:56:01 +0000
@@ -625,17 +625,11 @@
name="+portlet-options"
template="../templates/poll-portlet-options.pt"/>
</browser:pages>
- <browser:pages
+ <browser:page
+ name="+vote"
for="lp.registry.interfaces.poll.IPoll"
permission="launchpad.AnyPerson"
- class="lp.registry.browser.poll.PollVoteView">
- <browser:page
- name="+vote-simple"
- template="../templates/poll-vote-simple.pt"/>
- <browser:page
- name="+vote-condorcet"
- template="../templates/poll-vote-condorcet.pt"/>
- </browser:pages>
+ class="lp.registry.browser.poll.PollVoteView"/>
<browser:page
name="+edit"
for="lp.registry.interfaces.poll.IPoll"
=== modified file 'lib/lp/registry/browser/poll.py'
--- lib/lp/registry/browser/poll.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/poll.py 2010-10-11 17:56:01 +0000
@@ -17,6 +17,7 @@
'PollBreadcrumb',
]
+from z3c.ptcompat import ViewPageTemplateFile
from zope.app.form.browser import TextWidget
from zope.component import getUtility
from zope.event import notify
@@ -219,11 +220,8 @@
request = self.request
if (self.userCanVote() and self.context.isOpen() and
self.context.getActiveOptions()):
- context_url = canonical_url(self.context)
- if self.isSimple():
- request.response.redirect("%s/+vote-simple" % context_url)
- elif self.isCondorcet():
- request.response.redirect("%s/+vote-condorcet" % context_url)
+ vote_url = canonical_url(self.context, view_name='+vote')
+ request.response.redirect(vote_url)
def getVotesByOption(self, option):
"""Return the number of votes the given option received."""
@@ -256,6 +254,18 @@
change it. Otherwise he can register his vote.
"""
+ default_template = ViewPageTemplateFile(
+ '../templates/poll-vote-simple.pt')
+ condorcet_template = ViewPageTemplateFile(
+ '../templates/poll-vote-condorcet.pt')
+
+ @property
+ def template(self):
+ if self.isCondorcet():
+ return self.condorcet_template
+ else:
+ return self.default_template
+
def initialize(self):
"""Process the form, if it was submitted."""
super(PollVoteView, self).initialize()
=== modified file 'lib/lp/registry/browser/tests/poll-views_0.txt'
--- lib/lp/registry/browser/tests/poll-views_0.txt 2010-10-03 15:30:06 +0000
+++ lib/lp/registry/browser/tests/poll-views_0.txt 2010-10-11 17:56:01 +0000
@@ -78,7 +78,7 @@
>>> request = TestRequest(form={'changevote': 'Change Vote'})
>>> request.method = 'POST'
- >>> voting_page = getMultiAdapter((poll, request), name="+vote-condorcet")
+ >>> voting_page = getMultiAdapter((poll, request), name="+vote")
>>> form_processed = False
>>> def form_processing():
... form_processed = True
=== added file 'lib/lp/registry/browser/tests/test_poll.py'
--- lib/lp/registry/browser/tests/test_poll.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_poll.py 2010-10-11 17:56:01 +0000
@@ -0,0 +1,38 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for IPoll views."""
+
+__metaclass__ = type
+
+import os
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.poll import PollAlgorithm
+from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_view
+
+
+class TestPollVoteView(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestPollVoteView, self).setUp()
+ self.team = self.factory.makeTeam()
+
+ def test_simple_poll_template(self):
+ poll = self.factory.makePoll(
+ self.team, 'name', 'title', 'proposition',
+ poll_type=PollAlgorithm.SIMPLE)
+ view = create_view(poll, name='+vote')
+ self.assertEqual(
+ 'poll-vote-simple.pt', os.path.basename(view.template.filename))
+
+ def test_condorcet_poll_template(self):
+ poll = self.factory.makePoll(
+ self.team, 'name', 'title', 'proposition',
+ poll_type=PollAlgorithm.CONDORCET)
+ view = create_view(poll, name='+vote')
+ self.assertEqual(
+ 'poll-vote-condorcet.pt',
+ os.path.basename(view.template.filename))
=== modified file 'lib/lp/registry/stories/team-polls/edit-poll.txt'
--- lib/lp/registry/stories/team-polls/edit-poll.txt 2009-08-20 17:56:48 +0000
+++ lib/lp/registry/stories/team-polls/edit-poll.txt 2010-10-11 17:56:01 +0000
@@ -69,7 +69,7 @@
>>> team_admin_browser.getLink('A random poll that never closes').click()
>>> team_admin_browser.url
- 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote-simple'
+ 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote'
>>> team_admin_browser.open(
... 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+edit')
=== modified file 'lib/lp/registry/stories/team-polls/vote-poll.txt'
--- lib/lp/registry/stories/team-polls/vote-poll.txt 2008-10-08 14:56:56 +0000
+++ lib/lp/registry/stories/team-polls/vote-poll.txt 2010-10-11 17:56:01 +0000
@@ -8,7 +8,7 @@
>>> browser.open('http://launchpad.dev/~ubuntu-team/+polls')
>>> browser.getLink('A random poll that never closes').click()
>>> browser.url
- 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote-simple'
+ 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote'
>>> print find_tag_by_id(browser.contents, 'your-vote').renderContents()
<BLANKLINE>
@@ -22,7 +22,7 @@
>>> browser.getControl('Continue').click()
>>> browser.url
- 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote-simple'
+ 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote'
>>> tags = find_tags_by_class(browser.contents, "informational message")
>>> for tag in tags:
@@ -42,7 +42,7 @@
>>> browser.open('http://launchpad.dev/~ubuntu-team/+polls')
>>> browser.getLink('A public poll that never closes').click()
>>> browser.url
- 'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+vote-simple'
+ 'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+vote'
>>> print find_tag_by_id(browser.contents, 'your-vote').renderContents()
<BLANKLINE>
@@ -56,7 +56,7 @@
>>> browser.getControl('Continue').click()
>>> browser.url
- 'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+vote-simple'
+ 'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+vote'
>>> tags = find_tags_by_class(browser.contents, "informational message")
>>> for tag in tags:
@@ -78,7 +78,7 @@
>>> team_admin_browser = setupBrowser(
... auth='Basic jeff.waugh@xxxxxxxxxxxxxxx:jdub')
>>> team_admin_browser.open(
- ... 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote-simple')
+ ... 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote')
>>> not_yet_voted_message = 'You have not yet voted in this poll.'
>>> not_yet_voted_message in team_admin_browser.contents
True
@@ -107,7 +107,7 @@
>>> non_member_browser = setupBrowser(
... auth='Basic test@xxxxxxxxxxxxx:test')
>>> non_member_browser.open(
- ... 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote-simple')
+ ... 'http://launchpad.dev/~ubuntu-team/+poll/never-closes/+vote')
>>> for tag in find_tags_by_class(
... non_member_browser.contents, "informational message"):
... print tag.renderContents()
@@ -129,7 +129,7 @@
...
>>> team_admin_browser.open(
- ... 'http://launchpad.dev/~ubuntu-team/+poll/leader-2004/+vote-simple')
+ ... 'http://launchpad.dev/~ubuntu-team/+poll/leader-2004/+vote')
>>> print find_tag_by_id(
... team_admin_browser.contents, 'maincontent').renderContents()
<BLANKLINE>
@@ -161,7 +161,7 @@
LookupError: name 'continue'
>>> team_admin_browser.open(
- ... 'http://launchpad.dev/~ubuntu-team/+poll/director-2004/+vote-condorcet')
+ ... 'http://launchpad.dev/~ubuntu-team/+poll/director-2004/+vote')
>>> for message in get_feedback_messages(team_admin_browser.contents):
... print message
This poll is already closed.
=== modified file 'lib/lp/registry/stories/team-polls/xx-poll-condorcet-voting.txt'
--- lib/lp/registry/stories/team-polls/xx-poll-condorcet-voting.txt 2009-08-21 18:46:34 +0000
+++ lib/lp/registry/stories/team-polls/xx-poll-condorcet-voting.txt 2010-10-11 17:56:01 +0000
@@ -11,11 +11,11 @@
... """)
HTTP/1.1 303 See Other
...
- Location: http://localhost/~ubuntu-team/+poll/never-closes2/+vote-condorcet
+ Location: http://localhost/~ubuntu-team/+poll/never-closes2/+vote
...
>>> print http(r"""
- ... GET /~ubuntu-team/+poll/never-closes2/+vote-condorcet HTTP/1.1
+ ... GET /~ubuntu-team/+poll/never-closes2/+vote HTTP/1.1
... Authorization: Basic foo.bar@xxxxxxxxxxxxx:test
... """)
HTTP/1.1 200 Ok
@@ -32,7 +32,7 @@
he won't be allowed.
>>> print http(r"""
- ... GET /~ubuntu-team/+poll/never-closes2/+vote-condorcet HTTP/1.1
+ ... GET /~ubuntu-team/+poll/never-closes2/+vote HTTP/1.1
... Authorization: Basic dGVzdEBjYW5vbmljYWwuY29tOnRlc3Q=
... """)
HTTP/1.1 200 Ok
@@ -43,7 +43,7 @@
By providing the token we will be able to see our current vote.
>>> print http(r"""
- ... POST /~ubuntu-team/+poll/never-closes2/+vote-condorcet HTTP/1.1
+ ... POST /~ubuntu-team/+poll/never-closes2/+vote HTTP/1.1
... Authorization: Basic foo.bar@xxxxxxxxxxxxx:test
... Content-Type: application/x-www-form-urlencoded
...
@@ -78,7 +78,7 @@
It's also possible to change the vote, if wanted.
>>> print http(r"""
- ... POST /~ubuntu-team/+poll/never-closes2/+vote-condorcet HTTP/1.1
+ ... POST /~ubuntu-team/+poll/never-closes2/+vote HTTP/1.1
... Authorization: Basic foo.bar@xxxxxxxxxxxxx:test
... Content-Type: application/x-www-form-urlencoded
...
@@ -124,11 +124,11 @@
... """)
HTTP/1.1 303 See Other
...
- Location: http://localhost/~ubuntu-team/+poll/never-closes3/+vote-condorcet
+ Location: http://localhost/~ubuntu-team/+poll/never-closes3/+vote
...
>>> print http(r"""
- ... GET /~ubuntu-team/+poll/never-closes3/+vote-condorcet HTTP/1.1
+ ... GET /~ubuntu-team/+poll/never-closes3/+vote HTTP/1.1
... Authorization: Basic foo.bar@xxxxxxxxxxxxx:test
... """)
HTTP/1.1 200 Ok
@@ -165,7 +165,7 @@
vote.
>>> print http(r"""
- ... POST /~ubuntu-team/+poll/never-closes3/+vote-condorcet HTTP/1.1
+ ... POST /~ubuntu-team/+poll/never-closes3/+vote HTTP/1.1
... Authorization: Basic foo.bar@xxxxxxxxxxxxx:test
... Content-Type: application/x-www-form-urlencoded
...
@@ -210,7 +210,7 @@
... """)
HTTP/1.1 303 See Other
...
- Location: http://localhost/~ubuntu-team/+poll/never-closes3/+vote-condorcet
+ Location: http://localhost/~ubuntu-team/+poll/never-closes3/+vote
...
@@ -218,7 +218,7 @@
to vote.
>>> print http(r"""
- ... GET /~ubuntu-team/+poll/never-closes3/+vote-condorcet HTTP/1.1
+ ... GET /~ubuntu-team/+poll/never-closes3/+vote HTTP/1.1
... Authorization: Basic mark@xxxxxxxxxxx:test
... """)
HTTP/1.1 200 Ok
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-10-03 15:30:06 +0000
+++ lib/lp/testing/factory.py 2010-10-11 17:56:01 +0000
@@ -717,14 +717,15 @@
naked_team.addMember(member, owner)
return team
- def makePoll(self, team, name, title, proposition):
+ def makePoll(self, team, name, title, proposition,
+ poll_type=PollAlgorithm.SIMPLE):
"""Create a new poll which starts tomorrow and lasts for a week."""
dateopens = datetime.now(pytz.UTC) + timedelta(days=1)
datecloses = dateopens + timedelta(days=7)
return getUtility(IPollSet).new(
team, name, title, proposition, dateopens, datecloses,
PollSecrecy.SECRET, allowspoilt=True,
- poll_type=PollAlgorithm.SIMPLE)
+ poll_type=poll_type)
def makeTranslationGroup(
self, owner=None, name=None, title=None, summary=None, url=None):
