Whew!

I took a hammer to poll module. I've done the following things:

  1. Fixed E_NOTICE errors.
  2. Fixed all of the forms to conform both to new documentation and new FAPI. These forms now behave wonderfully. There's a pleasant side effect to the way I did this change: Voting or cancelling a vote on a poll no longer takes you to the poll page, but leaves you on whatever page you were on. This is very nice if you're looking at a list of polls or voting on a poll in a block, where it's disconcerting to be taken to a different page, usually.
  3. Made all of the theme functions comply with new .tpl.php standards and documentation.
  4. Re-organized the code logically. The functions in this module were spread in places that simply didn't make sense.

This effort came out of an earlier effort to add a new UI to the poll.module; on Steven's advice I've backed that part out of this, because it's a pretty sweeping change and fairly late in the D6 process to get that in. The rest of this, IMO, is necessary. The code must be kept up to date.

CommentFileSizeAuthor
#11 poll.fixes__3.patch34.69 KBmerlinofchaos
#8 poll.fixes__2.patch32.48 KBmerlinofchaos
#2 poll.fixes__1.patch31.7 KBmerlinofchaos
#1 poll.fixes__0.patch31.68 KBmerlinofchaos
poll.fixes_.patch32.46 KBmerlinofchaos

Comments

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new poll.fixes__0.patch31.68 KB

Crap, wrong version of the patch. This one didn't complete the process of renaming a theme function.

(I renamed theme_poll_view_voting to theme_poll_vote which is slightly more sensible.)

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new poll.fixes__1.patch31.7 KB

Whoops. Last patch had broken the cancel vote button.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

new node - previewing gives this error:

notice: Undefined property: vote in /Users/Shared/www/drupal6/modules/poll/poll.module on line 533.
pwolanin’s picture

pwolanin commented

upon saving, a couple notices from node module plus:

notice: Undefined variable: i in /Users/Shared/www/drupal6/modules/poll/poll.module on line 339.

Also, I'm seeing some weird behavior when viewing the same poll when logged out or logged in on the same computer. If I voted while logged in and then I'm logged out (and the anonymous user can 'cancel own' vote) the cancel button appears for the anonymous user even though the vote was made by an authenticated user.

pwolanin’s picture

pwolanin commented

odder still - though the cancel button appears for the anonymous user, it has no effect in the case outlined above- wher an authenticated user has voted already.

Once the anonymous user has voted the cancel button works as expected.

pwolanin’s picture

pwolanin commented

also, after voting if I go to the URL like node/6/results I get 'Access denied', rather than being shown the same results as at node/6

pwolanin’s picture

pwolanin commented

On the bright side - overall the module with this patch works far better than the crappy broken module in 5.x.

another problem (also present in 5.x) if I edit the node, it allows me to vote again, even though my vote has already been recorded.

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new poll.fixes__2.patch32.48 KB

Ok, new patch fixes the notice errors pwolanin identified.

Also, I'm seeing some weird behavior when viewing the same poll when logged out or logged in on the same computer. If I voted while logged in and then I'm logged out (and the anonymous user can 'cancel own' vote) the cancel button appears for the anonymous user even though the vote was made by an authenticated user.

I can't duplicate this behavior on my setup. I suspect this means there's something subtly wrong in the piece of code that checks to see if the current user has voted, but I don't see how it can work this way. It's very confusing.

also, after voting if I go to the URL like node/6/results I get 'Access denied', rather than being shown the same results as at node/6

This is controlled by the 'inspect all votes' permission, which lets you see results even if you haven't voted. Otherwise the way to see results after voting is to visit the node itself. I think this is pretty much the intended behavior;

merlinofchaos’s picture

merlinofchaos commented

Editing a poll clears the history of who voted for what. This is an annoying side effect of the fact that choices have no IDs, so when you save the node, it can't tell if the choices you put in (even if unchanged) are the same or not. So it just wipes the whole thing.

That's kind of dorky. I'm not sure if I have the energy to fix it with the current poll UI.

dries’s picture

dries commented

I haven't tested this patch yet, but I did had a first look at the code:

  1. Reading the PHPdoc, it is not clear what $raw_links is, or what 'raw' means here. Does it mean 'insecure'? What links are we talking about? How is it different from $links? These parameters could benefit from some additional information. I'm not a fan of the word "raw" -- it's not clear what it means.
  2. $rest is defined as 'anything else that has been added'? You mean added by additional modules? It might be good to mention this explicitly.

Looks good so far, but I'll review it some more/better later on.

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new poll.fixes__3.patch34.69 KB

New patch addresses Dries' PHP Doc comments, plus it moves the : into the theming layer.

To explain, a common complaint about poll module is that in the block, it puts a : after the question. This happens because the title of the poll is put into the #title field of the radios form widget. This patch moves that title to poll-vote.tpl.php -- though it retains the :. (I'm not actually sure it should, that : is kind of annoying there.)

moshe weitzman’s picture

moshe weitzman commented

just want to point out the pending AHAH patch for poll.module at http://drupal.org/node/155870 and and its later cousin (AHA fapi 2) - http://drupal.org/node/157752. not sure if this patch should merge with the first one above.

merlinofchaos’s picture

merlinofchaos commented
Status: Needs work » Needs review

Since I completely left out my UI enhancements, the ahah patch can follow this (or vice versa) if we really want. Alternatively, I can probably restore my UI enhancements and provide it as a better patch than the ahah patch.

I just realized I'd never set this back to review after my last version of the patch. No wonder no one has been reviewing it.

dries’s picture

dries commented
Status: Needs review » Fixed

I committed this patch. Thanks Earl. I'll update the AHAH patches to mention that this patch went in first.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
yngens’s picture

yngens commented

will this patch work if i apply it against 5.x?

merlinofchaos’s picture

merlinofchaos commented

No, this patch relied on new features in 6.x. It won't work in 5.x