Posted by neclimdul on March 16, 2009 at 11:00pm
15 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | poll.module |
| Category: | bug report |
| Priority: | minor |
| Assigned: | c31ck |
| Status: | patch (to be ported) |
| Issue tags: | D8MI, language-content, needs backport to D6, needs backport to D7, Novice |
Issue Summary
The poll module doesn't provide any default choices on translation which is counter to the default behavior of other fields like title, body, etc.
This patch catches the translate_prepare hook and populates the choices. Patches for HEAD and D6 supplied.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| poll_default_translation_choices.patch | 682 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
| poll_default_translation_choices-D6.patch | 919 bytes | Ignored: Check issue status. | None | None |
Comments
#1
#2
I've only reviewed the drupal 7 patch.
It's very simple and obvious what it does. Let's get this in.
#3
Looks good to me too, and patches apply cleanly.
#4
#5
So this looks like a very straight-forward fix, and I imagine it is probably blatantly obvious to someone familiar with the translation system what this fixes, but it is not in anyway obvious to me. :)
Is it possible to make a small test that demonstrates the bug this is fixing? That would help me to understand why it's important it be committed.
#6
confirming that.. to reproduce, it assumes the poll content type is set to have multilingual capabilities with translations (3rd i18n option in workflow settings), a poll already created set with the default site language, and when you go to translate that node, choices don't get populated from the original node.
Great patch :)
#7
Ah-ha. Great. Thanks for the summary, caktux!
Given that that's the issue, it doesn't really make sense to add a test here, since this is just one instance of the hook. This would rather go into a more general translation system test suite testing all hooks, including this one (incidentally, I would love to see said tests written if they don't exist yet).
Committed to HEAD. Moving to 6.x for consideration.
#8
Ahem. :P
#9
This is indeed an omission in the Drupal 6 language system. I think we did not realize that it is this obvious to implement and postponed it as long as we were out in feature freeze.
Anyway, this patch looks great indeed. However, I'd like to point out an omission and get that fixed before we get this committed to Drupal 6. The cloned options also copy the vote values from the original node, which IMHO should not be done by default. The default behavior should be to null the votes out for creation of the translation (in the cloned node). There is similar code in poll_insert() in Drupal 6, but that only ever applies if the user is not a node admin. So the vote counts will be right for non-admins on cloning, but admins would need to clear out the votes on translation.
I'd love to get this committed to Drupal 6 after the above fix is committed to 7 and all changes are in one patch for Drupal 7.
#10
#11
Bah, forgot the comment. updated to handle Gabor's comments.
#12
The last submitted patch failed testing.
#13
Rerolled the D7 patch from #10, now applies cleanly to D7 and D8.
The patch for D6 is still good to go after this one gets in.
#14
Re-rolled for the great '/core' migration in D8.
#15
Looks good and simple and is discussed at great length above, should be RTBC once tests pass IMHO.
#16
There we go, all green & happy :)
#17
OK this looks fine, so I've committed/pushed to 8.x.
After that I realised 1. do we still want to backport this to 7.x and 2. that I should've knocked it back to CNW for tests. So.. let's add the tests here (or find out why it's not worth it), then consider backport or not?
#18
@amateescu: can you help with writing a test for this? My understanding is the test would do the following:
1. Create a poll node, cast a vote.
2. Mark the poll node type translatable (enable translation module).
3. Create a translation.
4. Ensure that the vote count on the translated poll is 0 (ie. the previously recorded vote is not present on the translation).
If the patch is rolled back 4 would fail, otherwise 4 would pass.
#19
Yep, I'll do that in a couple of days.
#20
I've created a test that:
- Creates a poll.
- Marks poll content type as translatable.
- Sets a vote count on the poll.
- Tries to translate the poll and checks that vote counts are not copied from the original poll.
The first patch should pass, the second rolls back the changes from #14 and should fail.
#21
The last submitted patch, 404116-default-choices-poll-translations-test-rollback-20.patch, failed testing.
#22
Tests behaved as expected. Setting to needs review.
#23
The test looks good. I think the checking for the addition of the new language is a bit too verbose. We don't need to check that twice. Checking for the drupal set message text on the resulting page is fine. I guess you used the language name from the language_list() output. However, you can just hardwire 'Dutch' in the t() arguments since it is a known value for the test. Otherwise looks pretty complete.
#24
c31ck was faster :)
#25
I'll change the test according to Gábor's comments tonight.
#26
Changed test according to #23: Check enabling of language only once and hardcoded 'Dutch' in the t() args.
The first patch should pass, the second rolls back the changes from #14 and should fail.
#27
The last submitted patch, 404116-default-choices-poll-translations-test-rollback-26.patch, failed testing.
#28
Tests behaved as expected. Setting to needs review.
#29
Tagging as current focus. Looked through the tests twice and the proposed testing method looks good and comprehensive. The patch already landed, so we only need the tests to land.
#30
Test looks excellent, but is missing some API documentation:
+++ b/core/modules/poll/poll.testundefined@@ -798,3 +798,66 @@ class PollDeleteChoiceTestCase extends PollTestCase {
+class PollTranslateTestCase extends PollTestCase {
...
+ function testPollTranslate() {
The class and test method need docblocks. See: http://drupal.org/node/1354#functions and http://drupal.org/node/1354#classes.
I won't set the status back, but it would be nice if someone could reroll to add those two docblocks. Otherwise, we'll have to have a followup issue for it which sort of violates the whole documentation gate thing. :P Tagging novice to add some docblocks to the patch in #26.
#31
Let's add the documentation first, and then set back to RTBC. Thanks!
Sounds like we're really close.
#32
@Dries: agreed. @amateescu or @c31ck can you do that? Thanks a lot!
#33
Added docblocks to the class and the test method.
#34
Looks good to me.
#35
The summaries should start with third-person verbs. Please review the referenced points in the API doc. ;)
#36
#37
#38
Sorry, this is a bit better.
#39
The last submitted patch, 404116-need-coffee.patch, failed testing.
#40
#38: 404116-need-coffee.patch queued for re-testing.
#41
It is a set of test additions only with now even better comments. So should be good to go again :)
#42
Committed to 8.x. Thanks for adding the documentation so quickly.
#43
#44
Tagging for the content handling leg of D8MI.
#45
Patch that resets the vote count values on translation and adds the test.
#46
The last submitted patch, 404116-default-choices-poll-translation-test-only-44.patch, failed testing.
#47
Tests behave as expected, resetting status.
#48
Simple re-roll of the D8 patch.
#49
Committed and pushed to 7.x. Thanks!
Bumping down to 6.x.