Bug #1997
closedForum post notification didn't indicate image attachments
0%
Description
The email notification for a post that I added to the CBox group, which had 8 jpgs attached, didn't indicate any attachments to the post. Was that because of the file type?
Files
Updated by Boone Gorges about 12 years ago
This problem arose from the following changeset in BuddyPress Group Email Subscription: https://github.com/boonebgorges/buddypress-group-email-subscription/commit/526b80c617fe9058a859ac4eb4cfb1d42d333aa0 In brief, the second param passed to the 'bp_ass_forum_reply_content' and related filters used to be a BP activity item. But since these functions were refactored to fire on a bbPress action rather than a BP activity action, the activity item does not exist. As a result, any function that attempts to filter these values in a way that uses this second parameter (and operates on bbPress content, since other activity content is processed in a different way) doesn't work anymore. For CAC, that means the following functions:
cac_full_forum_notification()
cac_attachment_message_notification()
Ray, I've added you as a watcher here so you can give advice. On one hand, it's straightforward enough to rewrite these CAC functions to do things properly (though we'll probably need to pass at least the post/topic IDs to the filters in BPGES). On the other hand, if this is broken for CAC, it's possible that there are other installations that have hooked to the filters in a similar way, in which case their filters would be broken as well. We could be more backward compatible if we assembled a faux BP activity object and passed it along to the filter, for the purposes of these older plugins. In my backpat-loving-heart, I think this second option is the right one, but I may be going overboard here, as it certainly won't be very elegant looking. What do you think?
Updated by Raymond Hoh about 12 years ago
Hi Boone, big facepalm on my part :(
I love your idea of creating the faux activity object! I'm just about finished a commit for GES.
One quick thing, do you mind if I omit the 'action' and 'primary_link' columns in the faux activitiy object? I believe no one will be looking at those columns during filter checks. We can't add the 'id' column for obvious reasons!
Updated by Boone Gorges about 12 years ago
- Target version changed from 1.3.17 to 96
Hi Ray,
No problemo :) We were doing something fairly obscure anyway!
Sure, go ahead and leave 'action' and 'primary_link' out. I'd think the main items of importance are 'component', 'type', 'item_id', and 'secondary_item_id' (and maybe 'user_id').
Updated by Raymond Hoh about 12 years ago
Check your pull requests! :)
Btw, I did add in 'action' and 'primary_link' for the hell of it! 'action' won't be 100% identical because it strips the hyperlinks, but that could be considered a good thing?
Updated by Boone Gorges about 12 years ago
FYI, this change has been put into the distribution version of BPGES. I'll double check that it's solved the problem next time I upgrade the Commons.
Updated by Boone Gorges about 12 years ago
- Status changed from Assigned to Resolved
- Target version changed from 96 to 1.4
The plugin's been updated in 1.4.
Updated by Matt Gold about 12 years ago
- Status changed from Resolved to Assigned
- Target version changed from 1.4 to 1.4.3
Just received a message notification for a post that has attachments, but the message didn't indicate that attachments were present. Reopening, though if you'd prefer a new ticket, please let me know.
Updated by Boone Gorges about 12 years ago
- Estimated time set to 1.00 h
- Severity changed from Normal to Trivial
Updated by Matt Gold almost 12 years ago
- Target version changed from 1.4.3 to 1.4.6
Updated by Matt Gold almost 12 years ago
- Severity changed from Trivial to Low impact
Don't think this is trivial ;)
Updated by Dominic Giglio almost 12 years ago
Boone,
I've been reading through 1.4.6 issues to see if there's anything I can help with (in addition to my own assigned issues).
I think I may have noticed a connection between this issue and issue #2121 (Email notification of forum post omits hyperlinked word). In your original comment you link to the commit that has changed the behavior described in this issue. On lines 104 and 127 of bp-activity-subscription-functions.php
, $the_content
is being filtered through a call to html_entity_decode( strip_tags( stripslashes( $post->post_text ) ), ENT_QUOTES )
. Could this be removing words that've been "hyperlinked in the post" as Matt describes in #2121?
Also, in comment-4 Ray says:
'action' won't be 100% identical because it strips the hyperlinks
While I don't fully understand the use of the "faux activity" object, could this addition be affecting the post's hyperlinked content inadvertently?
Updated by Dominic Giglio almost 12 years ago
Additionally, this issue is 2 months old.
Issue #2121 (Email notification of forum post omits hyperlinked word) is 10 days old and issue #2129 ([& hellip ;]) is eight days old. It would seem plausible that both of those issue could be related to this one. If Matt didn't see missing hyperlinks or the [& hellip ;] until the updates mentioned in this issue, they could be caused by the html_entity_decode( strip_tags( stripslashes( $post->post_text ) ), ENT_QUOTES )
on lines 104 and 127 of bp-activity-subscription-functions.php
. yes/no?
Updated by Boone Gorges almost 12 years ago
Dom - The line you reference is probably related to #2121 (strip_tags()
) and #2129 (html_entity_decode()
), but probably not this ticket.
The link between GES and forum attachments is totally custom. It's handled by the function cac_attachment_message_notification()
in bp-custom.php (around 690). Basically, that function does the following:
- Catches the content of the email notification before it's sent
- Uses the $activity information passed by the filter (that's the $content
param of the function) to look up the corresponding forum post
- Uses the forum post info to look up whether the post has any attachments
- If attachments are found, fetches the information about them, and appends a message to the end of the forum notification content before sending it back to the GES plugin.
If it's not working, I'm guessing it comes down to one of three things:
1) cac_attachment_message_notification()
is not running at all (the hook isn't firing or something)
2) The second parameter being passed to the function, the "faux activity item" created by GES, is incorrectly structured
3) There's a deeper bug in cac_attachment_message_notification()
that's preventing it from finding the attachments
My money is on 2, since the problem recently cropped up, and the faux activity item is new as well.
Dom, see what you can find. If you find yourself running in circles, let me know and I'll do a couple strategic debugs that might help you in the right direction.
Updated by Raymond Hoh almost 12 years ago
Since I'm the one that caused this kerfuffle in the first place, it makes sense for me to clean this mess up! :)
I took a look at the codebase and the filters and parameters look like they should work. I'm going to do a round of debugging right now to figure out what's happening.
Updated by Raymond Hoh almost 12 years ago
Okay, found out the problem was not due to the faux activity object.
It was one of those "cart before horse" situations because of the dreaded GES commit by me as outlined here.
Fixed in commit 2e70c39.
Before I mark this as resolved, give it a test on cdev and let me know if the GES email is supposed to look the way it does.
Updated by Boone Gorges almost 12 years ago
- Status changed from Assigned to Resolved
Good catch, Ray! Works like a charm.
Updated by Matt Gold almost 12 years ago
Yes! Screenshot attached. Awesome to see attachment notifications showing up again. Many thanks.