Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

Version: 6.x-dev » 7.x-dev
zroger’s picture

I've only reviewed the drupal 7 patch.

It's very simple and obvious what it does. Let's get this in.

stella’s picture

Looks good to me too, and patches apply cleanly.

stella’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs review

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.

caktux’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

webchick’s picture

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.

webchick’s picture

Version: 7.x-dev » 6.x-dev

Ahem. :P

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

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.

neclimdul’s picture

neclimdul’s picture

Status: Needs work » Needs review

Bah, forgot the comment. updated to handle Gabor's comments.

Status: Needs review » Needs work

The last submitted patch failed testing.

amateescu’s picture

Version: 7.x-dev » 8.x-dev
Assigned: neclimdul » Unassigned
Status: Needs work » Needs review
FileSize
531 bytes

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.

amateescu’s picture

Re-rolled for the great '/core' migration in D8.

Gábor Hojtsy’s picture

Issue tags: +D8MI

Looks good and simple and is discussed at great length above, should be RTBC once tests pass IMHO.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

There we go, all green & happy :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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?

Gábor Hojtsy’s picture

@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.

amateescu’s picture

Assigned: Unassigned » amateescu

Yep, I'll do that in a couple of days.

c31ck’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 404116-default-choices-poll-translations-test-rollback-20.patch, failed testing.

c31ck’s picture

Status: Needs work » Needs review

Tests behaved as expected. Setting to needs review.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

amateescu’s picture

Assigned: amateescu » Unassigned

c31ck was faster :)

c31ck’s picture

Assigned: Unassigned » c31ck

I'll change the test according to Gábor's comments tonight.

c31ck’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 404116-default-choices-poll-translations-test-rollback-26.patch, failed testing.

c31ck’s picture

Status: Needs work » Needs review

Tests behaved as expected. Setting to needs review.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +sprint

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.

xjm’s picture

Issue tags: +Novice

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.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's add the documentation first, and then set back to RTBC. Thanks!

Sounds like we're really close.

Gábor Hojtsy’s picture

@Dries: agreed. @amateescu or @c31ck can you do that? Thanks a lot!

c31ck’s picture

Status: Needs work » Needs review
FileSize
3.47 KB

Added docblocks to the class and the test method.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

xjm’s picture

The summaries should start with third-person verbs. Please review the referenced points in the API doc. ;)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
no_commit_credit’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.43 KB
no_commit_credit’s picture

FileSize
3.43 KB

Sorry, this is a bit better.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice, -D8MI, -sprint

The last submitted patch, 404116-need-coffee.patch, failed testing.

c31ck’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +D8MI, +sprint

#38: 404116-need-coffee.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

It is a set of test additions only with now even better comments. So should be good to go again :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks for adding the documentation so quickly.

amateescu’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: -sprint +Needs backport to D6, +Needs backport to D7
Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

c31ck’s picture

Patch that resets the vote count values on translation and adds the test.

Status: Needs review » Needs work

The last submitted patch, 404116-default-choices-poll-translation-test-only-44.patch, failed testing.

c31ck’s picture

Status: Needs work » Needs review

Tests behave as expected, resetting status.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Simple re-roll of the D8 patch.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 7.x. Thanks!

Bumping down to 6.x.

Albert Volkman’s picture

Re-roll of initial patch by neclimdul.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.