Support from Acquia helps fund testing for Drupal Acquia logo

Comments

snig created an issue. See original summary.

snig’s picture

snig’s picture

formatC'vt’s picture

Status: Needs review » Needs work

Confirm bug.
Patch makes duplicates of comments.

kandrupaler’s picture

Hi guys, I'm glad I'm not the only one facing this problem. Was freaking out thinking it's something I've done! But I have good news. Maybe.

@formatC'vt Thanks for the patch. I tried it out and it indeed makes duplicates everywhere. In your patch, replacing...

$commands[] = array('command' => 'ajaxCommentsAfter', 'selector' => '.comment-wrapper-nid-' . $comment->nid . '> .ajax-comment-wrapper, .comment-wrapper-nid-' . $comment->nid . '> .indented', 'html' => $comment_output);

with:

$commands[] = array('command' => 'ajaxCommentsAfter', 'selector' => '.comment-wrapper-nid-' . $comment->nid . '> .ajax-comment-wrapper:last, .comment-wrapper-nid-' . $comment->nid . '> .indented', 'html' => $comment_output);

Solves the problem...!!!

Can you pls check? I'm not an expert in PHP or JS or AJAX (or anything, for that matter!), but I was trying to solve this problem come what may. It seems to work for me...

formatC'vt’s picture

Status: Needs work » Needs review

thanks, needs review

jmato’s picture

I'm facing the same problem, i think the cause of the comment duplication it's that the chosen selectors are incorrect, the correct selectors would be either .comment-wrapper-nid-x > .ajax-comment-wrapper:last OR .comment-wrapper-nid-x > .indented:last, but not the two at the same time, why? because the selector needs to match only one element, matching more than one element will add the comment after every matched element.

In my case the container with the replies to a comment (with the class .indented) its on the same level of the DOM structure than the comment, i don't know if that change according to the theme or other factors, example of the structure.

.ajax-comment-wrapper (comment 1)
.indented (the container of the replies to comment 1)
    .ajax-comment-wrapper (one reply to comment 1)
    .ajax-comment-wrapper (other reply to comment 1)
.ajax-comment-wrapper (comment 2)
.ajax-comment-wrapper (comment 3)

In the @snig patch the code is adding the posted comment after every first level comment for the first selector and after every first level of reply to any first level comment for the second selector.

Comment 1: matches 1st selector
->Will add the comment here
    Repy 1.1: matches 2nd selector
->Will add the comment here
Comment 2: matches 1st selector
->Will add the comment here
    Repy 2.1: matches 2nd selector
        Reply 2.1.1: not matching any selector only direct childs of .comment-wrapper-nid-x
->Will add the comment here

In the solution proposed by @kandrupaler the first selector, .comment-wrapper-nid-x > .ajax-comment-wrapper:last will match only the last first level comment and all the first level replies to a first level comment for the second selector.

Comment 1: no matches
    Repy 1.1: matches 2nd selector
->Will add the comment here
Comment 2: matches 1st selector (last first level comment)
->Will add the comment here
    Repy 2.1: matches 2nd selector
        Reply 2.1.1: not matching any selector only direct childs of .comment-wrapper-nid-x
->Will add the comment here

I can't find a way to build a selector without making a database query, at least on the php code, in the js code it's possible, but looking the super clean js code i thought the developers left the logic for the server side.

The solution I've found was fetch the last two comments for the node from the database, the last its the posted comment, the previous it's was the last comment before the posted one, in the previous comment we look the value of the comment pid, if it's 0 then we are at a first level comment, then we use the .comment-wrapper-nid-x > .ajax-comment-wrapper:last selector, if it's different than 0 then we are facing a reply, the we use the .comment-wrapper-nid-x > .indented:last selector, example of code:

insert on line 382

$result = db_query('SELECT cid, pid FROM {comment} WHERE nid = :nid AND status = :status ORDER BY cid DESC LIMIT 2', array(':nid' => $form['#node']->nid, ':status' => 1))->fetchAllAssoc('cid');
sort($result);

r = replace, i = insert

     // Check sort by comment_goodness.
r    if (_ajax_comments_get_comment_sort_order($node) == 1 && !$result[0]->pid ) {
        ...
        $commands[] = array('command' => 'ajaxCommentsAfter', 'selector' => '.comment-wrapper-nid-' . $comment->nid . ' > .ajax-comment-wrapper:last', 'html' => $comment_output);
      }
i     else if (_ajax_comments_get_comment_sort_order($node) == 1 && $result[0]->pid ) {
i       $commands[] = array('command' => 'ajaxCommentsAfter', 'selector' => '.comment-wrapper-nid-' . $comment->nid . ' > .indented:last', 'html' => $comment_output);
i     }
      else {
       // Newer first. Append comment to top.
       ...

I'm not uploading a patch right now because I don't know if will work with pagination and other stuff, maybe in a few days, anyway, hope that helps.

Edit: Didn't see that the @snig patch also was modifying the ajax_comments.js file :(

kandrupaler’s picture

@jmato - thanks for looking into this. You say -

In the solution proposed by @kandrupaler the first selector, .comment-wrapper-nid-x > .ajax-comment-wrapper:last will match only the last first level comment and all the first level replies to a first level comment for the second selector.

Maybe I don't get it, but my fix is working with comments at all positions and levels. I just tried replying to a 3rd-level reply of a 1st-level comment which is neither first nor last, and it worked.

To make things more clear, I added the comment "reddester" below and my fix worked perfectly.

White
  Whiter
     Whitest

Red
  Redder
     Reddest
        Reddester

Blue
  Bluer
     Bluest

What am I missing?

Edit: I'm displaying AJAX comments in a view using the "List Comments" feature.

AsadKamil’s picture

The patch applied cleanly.
Thanks

root@asad-Vostro-3550:/var/www/html/git/ajax_comments# git apply -v ajax_comments-fixed_new_comment_adding_to_wrong_position.patch 
Checking patch ajax_comments.js...
Checking patch ajax_comments.module...
Applied patch ajax_comments.js cleanly.
Applied patch ajax_comments.module cleanly.
root@asad-Vostro-3550:/var/www/html/git/ajax_comments# 
kandrupaler’s picture

@AsadKamil, the patch can be applied, of course, but it has issues.

jmato’s picture

@kandrupaler I don't know if it happens with views (i using commentsblock to display comments) but the problem for me arises only with first level comments, replies are displayed fine.

Can you test adding a new first level comment with a reply as the last comment displayed? BTW my fix don't work with pagination :)

Edit: Can you post the HTML structure with the relevant classes as I did in the previous comment? example:

.ajax-comment-wrapper (comment 1)
.indented (the container of the replies to comment 1)
    .ajax-comment-wrapper (one reply to comment 1)
kandrupaler’s picture

@jmato, OOPS! You're right, my fix isn't a fix:-)

The AJAX was adding the comment in the right place, but after a page refresh, reddester has landed itself in the blue area (I didn't have any replies to "Blue"):

White
  Whiter
     Whitest

Red
  Redder
     Reddest

Blue
     Reddester

So apart from pagination (which I don't care about right now) your patch works perfectly?

jmato’s picture

@kandrupaler OOOOPS too, now thinking that your fix it's actually a fix, I didn't see that the original patch also was patching the ajax_comments.js file and that changes all, I will do some testing later.

jmato’s picture

@kandrupaler I've do some testing, the @snig patch it's working well, inserting the comment in the correct place, not making duplicates and neither the replying problem you mentioned it's happening here, a views related issue?

kandrupaler’s picture

@Jmato, views + ajax_comments definitely has this problem. I haven't tested this without views. This problem is making me question whether we need indented/threaded replies in the first place. On the whole, lots of issues using this module. I did think of switching over to facebook commenting instead of this, but that defeats our purpose of having more registrations.

jmato’s picture

@kandrupaler, can you post the code of the view you are using to show the comments? you can see the code by exporting the view.

snig’s picture

I can't reproduce any duplications

kandrupaler’s picture

@snig I'm seeing duplication when using views.

pmusaraj’s picture

Here is a smaller patch that avoids any chances of duplication by simply targeting the last div regardless of its class name.

qzmenko’s picture

Status: Needs review » Needs work

@pmusaraj not good solution. Because in default template file comment-wrapper.tpl.php last div element would be wrapper of comment form. And comments would be inserted after form.

zahord’s picture

Hi all,

I added a fix to solve the issue with the comments positions, I hope this can help

Regards!

zahord’s picture

Status: Needs work » Needs review
zahord’s picture

FileSize
552 bytes

Updated file

zahord’s picture

FileSize
646 bytes

I'm adding a new patch with the selector to detect the position updated

qzmenko’s picture

Status: Needs review » Needs work

Hello @zahord.
I still believe that it's not very good to rely on the h2 tag, as many developers replace it with another tag in custom comment template.

  • qzmenko committed 8873f67 on 7.x-1.x
    Issue #2679879 by zahord, snig, pmusaraj, kandrupaler, jmato, formatC'vt...
qzmenko’s picture

Version: 7.x-1.2 » 7.x-1.x-dev
Status: Needs work » Fixed

Fixed in dev branch.

Status: Fixed » Closed (fixed)

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