Manual use of rate_generate_widget leads to mis-matched widget ids, breaking AHAH updates. Apparently triggerd by fix in Issue #1676746

Agree, this is a serious issue in this new version 1.7 of the module: the widget inserted using the Rate native function rate_generate_widget doesn't work any longer. For this reason for now I would have to resort back to the previous version of Rate where it worked well.

Humm, yes, I can also confirm that this appears to be something introduced in 7.x-1.7. Thankfully downgrading seems to be trivial given that 1.7 did not introduce any schema changes or anything like that. It would be interesting to see which commit this relates to.

Also, I'm changing the version as bugs should probably always be reported against dev (currently 7.x-1.7, 7.x-1.x and HEAD are the same commit in the repository).

Ok, this seem to have been introduced in #1676746: Addtional Classes and more unique ID.

The commit from that issue appears to alter the widget id to ensure it is unique. This makes good sense but it appears fix was incomplete in implementation. Though it ensures that widget ids are unique it may not have ensured the AHAH js logic is also able to find those new ids (meaning the click gets processed, but the AHAH logic does not replace the widget with updated info). I'm just spit-balling here, but a preliminary analysis seems to point to this general conclusion.

Revering the commit from that issue solves the problem without rolling-back all the way to 7.x-1.6.

I'm marking a relationship between these issues and will also post a note in that related (closed) issue. Maybe someone involved in that previous issue will be able to comment.

I ran into this bug as well. I have placed three (different) widgets on my page via panels. It looks to me like the failure is in line 40 of rate.js:

// fetch all widgets with this id as class
widget = $('.' + widget.attr('id'));

In my case, my widgets had IDs like ".rate-node-143-1-1--2", but the classes looked like ".rate-node-143-1-1". So, my widget var was empty. Thus, later in the script, when it came time to replace the widget object with what was returned via rate_vote_ahah(), it failed. So, even though the vote was correctly recorded, the UI didn't update to give feedback to the user.

I edited rate.js to fetch all widgets with the ID in question (not class):

// fetch all widgets with this id
widget = $('#' + widget.attr('id'));

That gave me the correct object, and widget.before() and widget.remove() were able to run.

I don't really understand the implications of making this change, though. I ran a quick test by adding the same widget twice to my panel -- with my change, everything still works. Both widgets are updated when a user clicks on one to vote. And the IDs seem to be unique between the two copies.

Attaching a patch with my change...

I've finally had a chance to look at this more and review. From what I can see there are couple key revelations here:

1. This is only a problem for people who include a widget by calling rate_generate_widget() manually. This initially made no sense to me I can't see any reason why calling rate_generate_widget() manually would generate different output. Then I had a look at rate_node_view() and realized that rate_generate_widget() is always called there for a node even if the display mode is set to RATE_DISPLAY_DISABLE (e.g. the "Do not add automatically" setting). I'm not sure why the module still insists on building the widget in hook_node_view() when there is no intent to display it automatically, but that just seems to be the way things are. Anyway, because rate_generate_widget() ends up getting called twice for the same widget in these cases (once in rate_node_view() and once via a manual call), drupal_html_id() also gets called twice for the same ID. This means that during the second call (possibly the manual one) a number is appended to the ID, which is what ends up breaking the AHAH.
2. On the javascript side it looks like things are setup such that the click event is only supposed to work when a widget has a class that matches its ID. The need for this kind of check makes no sense to me as I would think that the widget ID is enough to fully identify the widget. Anyway, maybe there is a good reason for this? Maybe we need to target multiple matching widgets with different ids?

Alright, so if we assume that the two points above are the way things should be (extra attention may be needed on these points, but that might be best in separate issues), it seems like we just need to be sure that the extra appended number on the widget ID (if applied) is ignored by the javascript. I think sjancich suggestion would do this (pseudo patch for rate.js ~line 40):

- widget = $('.' + widget.attr('id'));
+ widget = $('#' + widget.attr('id'));

but isn't that just the same as commenting out the widget = $('.' + widget.attr('id')); line altogether given that all widget ids are now unique?

Perhaps instead we need to just to somewhat "cleanse" the id of any appended bits... something like (pseudo patch):

- widget = $('.' + widget.attr('id'));
+ var ids = widget.attr('id').split('--');
+ widget = $('.' + ids[0]);

Though I suppose this could still cause problems if the same widget is used twice on the same page (the widget var at the start of Drupal.rateVote() might not represent the widget var that just received a click event.

Moving back to "active" and I think we need some more confirmation about why the javascript is comparing the ID to the class for each widget before this could be solved.

I tested this some more, and I think my alternate suggestion from comment #5 appears correct. I tested this with a few cases, including one where the exact same widget was manually inserted twice on a page, and all works as expected.

I see now that looking-up all widgets with a class that matches the id does make sense. This will ensure that multiple matching widgets will get an AHAH update if either of them is clicked. As noted earlier, that look-up process just needs to ignore any numbers appended to the ID by drupal_html_id()... which is what this fix does.

Patch attached. I'm hoping someone can review.

No testers? I'm actually surprised that this is not causing more of a fuss given that this bug cripples all widgets that are built programmatically (via rate_generate_widget()).

According to the stats many thousands of people have already upgraded to 1.7 and I can't imagine that all of them are only using the GUI?

This seems to be related to the thing I reported here:

https://drupal.org/node/2266371

I made the changes in #5 and it's working for me in dev and production behind nginx

Can't test it now as had to rollback to the previous version of Rate and now in a freeze state, would certainly test when attempt the next upgrade.

I applied the patch manually since it is only a few lines. It works now, tested before and it was broken.

RTBC

I tested patch in #7 and I can confirm it fixes the issue.

I experienced the same issues as discussed herein and that seem to have led up to @rjacobs patch #7 albeit when rendering a widget in a view (that uses contextual filters to grab NID from URL) that is rendered on the content type that I had the widget enabled on (however "Do not add automatically" setting on). It could not find that particular widget in the JS due to the ID being of "wrong format" as described above.

However, by using the latest DEV version the problem went way, so is this committed? I see some changes in the JS of the handling of "widget" and "ids" between 1.7 and current DEV (which is "7.x-1.6+3-dev" according to info file).

#7 fixed the issue for me.

@klase, 7.x-1.6+3-dev doesn't include 7.x-1.7 commits. In fact 7.x-1.7 is the latest version at this moment.

RTBC based on #11, #14 and me.

I had another issue with it where on comments it made a JS error. Continued the existing patch with js on line 8.

+ if (typeof widget.attr('id') != 'undefined') {
}

Patch in #16 works for me, using Drupal Commons.

Marking RTBC because of the last comment.

@jOksanen, you should leave it up to another to RTBC your own patches.

I made almost the same patch before I found this issue. RTBC

Oops, my mistake, I though somehow that #7 is the last patch, carry on.

#16 works well for me.

Drupal 7 is EOL. Issue will be closed, but patches are still here