I found out that the advpoll_choices refactoring patch should have updated the nodeFormAutoAttach() algorithm for generating new IDs when you dynamically add a choice.

The current algorithm adds 1 to the last choice's id when creating the new choice - this is okay if the last choice has the highest id, but if it doesn't the new choice will overwrite an existing choice. It turns out that poll.module happens to create the ids in reverse order, so if you convert a poll into an advpoll and then try to add more choices you will see this bug. This would eventually have been uncovered when we add dynamic re-ordering, but it's good that we caught it beforehand.

Patch should fix it.

CommentFileSizeAuthor
#2 advpoll_add_choice.patch1.99 KBChrisKennedy
advpoll_add_choice.patch1.25 KBChrisKennedy

Comments

anders.fajerson’s picture

anders.fajerson commented
Status: Needs review » Reviewed & tested by the community

Nice one. Looks good to me. This also applies when deleting choices, right? (currently not possible though, see http://drupal.org/node/199103)

ChrisKennedy’s picture

ChrisKennedy commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new advpoll_add_choice.patch1.99 KB

True, and now that you mention it I need to tweak the algorithm slightly to account for potential deleted choices.

I also removed the edit-changed line used in this area of the code, since that variable is only used for non-javascript node editing.

anders.fajerson’s picture

anders.fajerson commented

Seems to work great. The id of the dynamically added choices doesn't really matter though? E.g they could just as well be new-choice-1, new-choice-2? Just an observation.

And I can't reproduce 2. (removing all choices) reported at http://drupal.org/node/199103#comment-652905, so that's all good.

ChrisKennedy’s picture

ChrisKennedy commented

Right, the id doesn't truly matter and it could be a different string if we wanted.

anders.fajerson’s picture

anders.fajerson commented

I thinking about saving some rows of JS by skipping that highest id algorithm, and going for something simpler. What do you think?

ChrisKennedy’s picture

ChrisKennedy commented

Well, it's used for the name, id, and label reference. If we take out the unique ID at the very least the label won't link to the input, and I don't think there will be a much simpler way to create a unique id considering that a single jQuery replace() converts the name, id, and label reference all at once.

I think (but I'm not sure) that it will be more of a pain to replace the current regex to account for separate string constructions. Plus if you remove choices and then add them I think it would be more work to maintain a new incremented id (i.e. if we removed new-choice-1 and added a new choice we would have to decrement the new choice counter, but not if we removed an old choice).

anders.fajerson’s picture

anders.fajerson commented
Status: Needs review » Reviewed & tested by the community

Ok then, let's get this is.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Reviewed & tested by the community » Fixed

http://drupal.org/cvs?commit=91558

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.