TestingParty08: collapsed/expanded comments

I was going to look at this but after playing around with comments I'm a little confused as to what functionality this issue refers to. What were you meaning by "collapsed/expanded comments"?

Fixes:

• Makes CommentHelperCase::commentExists aware of threading settings.
• CommentHelperCase::setCommentForm was not working. Updated hard-coded comment_form_location values to use constants from comment.module
• CommentHelperCase::setCommentsPerPage did nothing.
• CommentInterfaceTest: several tests failed after the above fixes were implemented.

Tests:

• Comment threading, all 4 modes

I tested this manually because I was looking into some bugs with the "Collapsed" mode where it was not ever displaying the body of the comments when in "Collapsed" mode. (I also just noticed that the "Add new comment" or whatever link isn't showing up in "Collapsed" mode, or maybe it isn't showing at all.) I posted a patch in #394548: Collapsed comments do not work that tests for the "Collapsed" mode working correctly as well as patching the comments.module to actually display comment body when in "Collapsed" mode. My testing is definitely not as rigorous as yours, but it doesn't seem to be testing the fact that the comments work in "Collapsed" mode, at least from my testing.

dandaman: the point of collapsed mode, afaik, is to hide the body of comments until you click through the subject to read the whole thing.

Just a few suggestions to meet the Coding Style guidelines, and one for comment readability. Beyond the little petty issues below, the code itself looks great, and seems to do what it's supposed to when I test it.

Good work!

• All your inline comments need to start with a capital letter, and end with a period
• variable_set($name.'_article', $value); - should be a space on both sides of the dot when concatenating.
• + //$this->drupalGet('node/' . $this->node->nid); - I thought commented out code wasn't supposed to be in core... if you don't need it, take it out for readability.
• for comment clarity / readability, some of the comments could use grammatical fixes. eg.,
  

  +    // In threaded mode, the replies on the oldest comment should be bumped to the first page.
  +    // And comment 2 should be bumped to the second page.
  +    // Again, we're not testing paging, 
  +    //  but this way's easier than checking the order of DOM elements.
  

  could be rewritten as

  // In threaded mode, the replies on the oldest comment should be displayed on the first page,
  // while comment 2 should be bumped to the second page. This is not a test for paging; it's just 
  // easier than checking the order of DOM elements.

brianV: thanks for the input.
here's a new version... haven't retested, but should be good to go

Looks good. RTBC as far as I can see.

I will leave it for another person to get eyes on it to confirm.

of course... another attempt.

Setting to 'needs review' so the testing bot will try again.

aaronbauman, I'm aware of that. What I found, though, in manual testing was that if I had the comments set to "Collapsed" and clicked on the title of the comment, it went to a new URL, but it looked just the same, with all the comments collapsed. In other words, after clicking on the comment, it did not display the comment. In issue #394548: Collapsed comments do not work I made an attempt at patching and testing that bug.

dan: oh i see.
that is clear now.
yeah, i can reproduce that issue on my install as well.

this patch so far only tests that the comment is collapsed,
not that the comment gets uncollapsed on click...

I think that should be included in this test, but that means the test will fail until #394548: Collapsed comments do not work is addressed.
so i'm marking this postponed until that patch gets rolled.

catch: my patch #394548: Collapsed comments do not work does have both a patch and a test to verify the fix. I don't see this patch necessarily needing the bugfix of my patch in order to be useful, as aaron's patch does almost every other rigorous test of the commenting system, and mine covers this small edge case of collapsed comments. I was just tracking this issue because I originally was trying to figure out what to write for this test during the Code Sprint of DrupalCon DC and then stumbled on the bug in my issue. To make aaron's tests more complete, I guess it would be helpful to have it all in one patch.

ok, so here's the patch, rolled together with patch #394548: Collapsed comments do not work

i'm not sure if this is the preferred way to go, but here it is...

adding ending newline... does it work now, testbot?

Warning: I'm too tired to do more than code-style currently. But my late-night gaze can examine the code practically byte for byte, so that's fine. :)

Firstly, there are a ton of lines that contain trailing white space (either at the end of the code, or in the form of identation on a blank line). You can remove all of these with s/ *$//g in perl or sed, so I won't mention them individually.

+      if($thread_setting == COMMENT_MODE_THREADED_EXPANDED
+        || $thread_setting == COMMENT_MODE_FLAT_EXPANDED) {

"if" needs a space before the condition. Also, if you move the OR operator to the end of the preceding line, you get consistency with the earlier line *and* you can align the two comparisons by their == sign.

+  /**
+   * Get comment setting for article content type
+   *
+   * @param string $name 
+   *   Name of variable
+   * @param string $default 
+   *   Default value to return if $name variable is not set
+   * @return mixed 
+   *   Value of $name variable or $default if $name variable is not set
+  **/

The description should have a period and the comment block should end in */, not **/.

After that follow a lot of changes that actually *fix* code-style. :)

 /**
+ * Threading tests for comments.
+**/
+class CommentThreadingTest extends CommentHelperCase {

+   * We don't need special tests in this function for collapsed vs. expanded
+   * with respect to showing or hiding comment body. Those tests are built 
+   * into CommentHelperCase::commentExists .
+  **/

These two comment blocks should be terminated by */, and there's another blank line with white space between them.

+    $reply2 = $this->postComment(null, $this->randomName(), $this->randomName(), FALSE, TRUE);
+
+
+    $pattern = '/<div class="indented">(<a id="new">)?<a id="comment-' . $reply1->id . '">/';

AFAIK, there should be only one blank line in a row. The $reply2 variable name may be okay, but I'm not sure.

+      t('Threaded, expanded reply to a reply is double-indented'), 'Comment');
+
+
+  }

Here's another one.

Cleaned up re: notes from Arancaytar as well as some other coding-standards related stuff.