Can't add new choices to converted polls
ChrisKennedy - December 8, 2007 - 18:04
| Project: | Advanced Poll |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
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.
| Attachment | Size |
|---|---|
| advpoll_add_choice.patch | 1.25 KB |
#1
Nice one. Looks good to me. This also applies when deleting choices, right? (currently not possible though, see http://drupal.org/node/199103)
#2
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.
#3
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.
#4
Right, the id doesn't truly matter and it could be a different string if we wanted.
#5
I thinking about saving some rows of JS by skipping that highest id algorithm, and going for something simpler. What do you think?
#6
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).
#7
Ok then, let's get this is.
#8
http://drupal.org/cvs?commit=91558
#9
Automatically closed -- issue fixed for two weeks with no activity.