Email Notifications Plugin for FullBlogPlugin... where is it?

bumping up the priority and severity... will need to implement this soon for a project. Still can't find the code.

The code ships with the plugin itself, so there is no separate download for it. It uses setup.py requirement to only enable loading the plugin if FullBlogPlugin is also available.

When available, it likely needs to be enabled for the project too - depending on how you installed it and what pattern you already have used to enable the main plugin. But the docs at AnnouncerPlugin/PluginSupport/FullBlogPlugin should really mention the short how-to:

1. Install FullBlogPlugin
2. Enable Announcer support

   [components]
   announcer.opt.fullblog.* = enabled
    3. Configure.... (as already described in docs)
   

I've made the FullBlogPlugin and provided the information for how to bundle it, but I don't actually use AnnouncerPlugin so I cannot verify that this works. Could you please verify the how-to, and if OK update the wiki docs and close this ticket?

Thanks.

Replying to osimons:

Could you please verify the how-to, and if OK update the wiki docs and close this ticket?

I'm in the process of installing the plugin and doing a feature comparison against FullBlogNotificationPlugin, so I'll update the docs and possibly open some tickets.

Replying to rjollos:

Replying to osimons:

Could you please verify the how-to, and if OK update the wiki docs and close this ticket?

I'm in the process of installing the plugin and doing a feature comparison against FullBlogNotificationPlugin, so I'll update the docs and possibly open some tickets.

Any news on that? Just curious.

Replying to hasienda:

Any news on that? Just curious.

Just repeating the statement I made on IRC this evening for all to hear - I've previously made the commitment to support the FullBlog extension to the AnnouncerPlugin, and still intend to do that. I'm bumping up the priority so that it remains on my near-term list of things to do.

From my point of view the core support code is not blog code but announcer code, and I think this makes it an announcer extension that should be kept where it is. Also, the fact that I don't use or support the announcer plugin myself means I won't maintain the code either. Having it in blog source and docs makes little sense.

The FullBlog module is missing the requires_authentication method for each of the components implementing IAnnouncementSubscriber. That fixes the major issue with the plugin, and I'll provide a patch shortly. On quick glance, the Bitten and Announcer modules appear to have the same issue.

Patch against r12305 of the trunk.

t9110-r12305-1.patch fixes the FullBlog module and also updates the documentation. I need to do a bit more testing, but I'm posting it here for feedback, and this is most likely the version of the patch that I'd like to commit.

Replying to rjollos:

The FullBlog module is missing the requires_authentication method for each of the components implementing IAnnouncementSubscriber. That fixes the major issue with the plugin, and I'll provide a patch shortly. On quick glance, the Bitten and Announcer modules appear to have the same issue.

True. I've been coming another road to the same conclusion. Obviously update of optional components has been forgotten after [9235].

On-topic - some minor nit-picks on your patch:

• change/improve messages, but adhere to the common style of incomplete sentences (no tailing dot)
• no need for triple double-quotes on one-liner strings
• not sure anymore about triple double-quotes in multi-line messages (used them myself a lot, but it tends to produce ugly, white-space infested msgid's while a series of single double-quoted forms a nice msgid - so consider leaving it as it is)

Apart from that it looks good to me.

OT: Blog is not installed here, so corresponding support code is disabled as well, and I don't see related errors as well, but AcctMgr is. When I go to prefs/subscriptions as authenticated user, all seems fine apart from the NOT IMPLEMENTED flag on AcctMgr notifications, but an AttributeError: 'AccountManagerAnnouncement' object has no attribute 'requires_authentication' jumps out right after I log-out while on that page. I'll go and fix that part.

Replying to hasienda:

... * not sure anymore about triple double-quotes in multi-line messages (used them myself a lot, but it tends to produce ugly, white-space infested msgid's while a series of single double-quoted forms a nice msgid - so consider leaving it as it is)

I've seen the ugly whitespace issue as well under other circumstances, so I'm mindful of it, but it doesn't seem to be a problem in this case. Anything I should be looking for besides the rendering of this message?:

The screen capture is from Trac 1.1.1dev. I'll test it out in Trac 0.11 as well, and try to figure out why the extra whitespace is stripped.

OT: Blog is not installed here, so corresponding support code is disabled as well, and I don't see related errors as well, but AcctMgr is. When I go to prefs/subscriptions as authenticated user, all seems fine apart from the NOT IMPLEMENTED flag on AcctMgr notifications, but an AttributeError: 'AccountManagerAnnouncement' object has no attribute 'requires_authentication' jumps out right after I log-out while on that page. I'll go and fix that part.

Where would you like me to place unit tests for the FullBlog module? announcer/opt/tests?

Replying to rjollos:

Replying to hasienda:

... * not sure anymore about triple double-quotes in multi-line messages (used them myself a lot, but it tends to produce ugly, white-space infested msgid's while a series of single double-quoted forms a nice msgid - so consider leaving it as it is)

I've seen the ugly whitespace issue as well under other circumstances, so I'm mindful of it, but it doesn't seem to be a problem in this case. Anything I should be looking for besides the rendering of this message?:

Sorry, this should have been clearer. It'll render ok, just a matter of cluttering the message catalogs for translators with white-space. And be sure to drop the tailing dots, please.

The screen capture is from Trac 1.1.1dev. I'll test it out in Trac 0.11 as well, and try to figure out why the extra whitespace is stripped. Where would you like me to place unit tests for the FullBlog module? announcer/opt/tests?

Yes, that looks sensible.

Another note after getting AcctMgr notification back to life (still in need of a face-lift):

Do not use BoolSubscriptionSetting like seen in announcer.opt.bitten.announce, because this would try to store settings the old way (schema revision < 4) in session_attribute db table. Of course this applies to SubscriptionSetting as well. Subscription and SubscriptionAttribute from announcer.model are the way to go these days.

I'll try to rework this later on and even remove the whole announcer.util.settings, if it's not essential i.e. for converting historic settings to the new schema.

Replying to hasienda:

... Sorry, this should have been clearer. It'll render ok, just a matter of cluttering the message catalogs for translators with white-space. And be sure to drop the tailing dots, please.

Ah, thanks for the explanation. I should have the changes fully tested and ready for commit by tomorrow.

I noticed while working on this ticket that the Watch this wiki contextual navigation item is shown for anonymous users, but the Watch this blog contextual navigation item is not shown for anonymous users. I've investigated the latter, and see there is an early return in FullBlogWatchSubscriber.post_process_request before the contextual navigation item is added:

       if req.authname == "anonymous":
            return (template, data, content_type)

So I wonder, is it correct that the wiki contextual navigation item is shown for anonymous?

One other thing I should address eventually: when a blog post with a comment is deleted, a subscriber will receive a notification for blog post deletion and blog comment deletion. It seems desirable to only send a notification about the blog post deletion. If we delete the post, then it is implied that all comments will have been deleted.

Replying to rjollos:

One other thing I should address eventually: when a blog post with a comment is deleted, a subscriber will receive a notification for blog post deletion and blog comment deletion. It seems desirable to only send a notification about the blog post deletion. If we delete the post, then it is implied that all comments will have been deleted.

Let's have a dedicated ticket for this, so that we can make a unique changelog entry => #10621.

Patch against r12354 of the trunk.

Requesting review of the attached patch t9110-r12354-1.patch, which:

1. Incorporates feedback from comment:9.
2. Wraps strings for translation.
3. Fixes multiple notification events (one for comment deletion and one for post deletion) when a post is deleted that has comments (comment:14 / #10621). Now, only a post deletion notification is sent.
4. Extracts instantiation of the AnnouncerSystem object into the constructor, allowing us to inject a MockAnnouncerSystem object that will enable us to write unit tests for cases such as (3), by recording the calls to the send method.
5. Testing showed that FullBlogBloggerSubscriber does not require authentication. The requires_authentication for all five of the subscribers could use a second eye.

If you agree with (4), my next step will be to write the MockAnnouncerSystem class and start adding unit tests for issues encountered in this ticket.

I'll be posting future changes to a Git branch here.

Replying to rjollos:

Requesting review of the attached patch t9110-r12354-1.patch, which: 1. Incorporates feedback from comment:9.

Ok. Thanks for taking care.

2. Wraps strings for translation.

Ok as well.

3. Fixes multiple notification events (one for comment deletion and one for post deletion) when a post is deleted that has comments (comment:14 / #10621). Now, only a post deletion notification is sent.

Fine.

4. Extracts instantiation of the AnnouncerSystem object into the constructor, allowing us to inject a MockAnnouncerSystem object that will enable us to write unit tests for cases such as (3), by recording the calls to the send method.

We discussed that on IRC. See some more thoughts in my recent comment to #10627, please.

5. Testing showed that FullBlogBloggerSubscriber does not require authentication. The requires_authentication for all five of the subscribers could use a second eye.

FullBlogBloggerSubscriber is similar to UserChangeSubscriber, so your assertion is true. I've checked the other subscribers too, and all of the don't require authentication.

OT: I found that TicketCustomFieldSubscriber is currently defined to require authentication, but this would prevent anonymouns users from setting an opt-out preference. But because Cc: fields may contain email addresses and custom Cc: fields may do so as well, this seems undesirable. Do you agree? If Yes, I'd change it right-away.

Replying to hasienda:

4. Extracts instantiation of the AnnouncerSystem object into the constructor, allowing us to inject a MockAnnouncerSystem object that will enable us to write unit tests for cases such as (3), by recording the calls to the send method.

We discussed that on IRC. See some more thoughts in my recent comment to #10627, please.

I'll revert this part of the patch and apply today then. falkb for one is probably eager to have this fixed.

I'll take a look at TicketCustomFieldSubscriber and get back to you shortly.

Replying to hasienda:

FullBlogBloggerSubscriber is similar to UserChangeSubscriber, so your assertion is true. I've checked the other subscribers too, and all of the don't require authentication.

I was about the commit this, but wait. Are you saying that all 5 of the subscribers for FullBlog don't require authentication? I currently have FullBlogWatchSubscriber and FullBlogMyPostSubscriber as requiring authentication. The former is because I was thinking that "watching" required authentication (see also comment:14). The latter is based on the comment from the API doc.

    def requires_authentication():
        """Returns True or False.  If the user is required to be authenticated
        to create the subscription, then return True.  This applies to things
        like ticket owner subscriber, since the ticket owner can never be the
        sid of an unauthenticated user and we have no way to lookup users by
        email address (as of yet).
        """

The FullBlogMyPostSubscriber (notify me when any blog post that I created is changed) seems analogous to being a ticket reporter, which I assume would be similar to the ticket owner example described in the API doc. I think that I tested all 5 as both authenticated and unauthenticated, but I can't say for certain. I haven't fully wrapped my head around this though, so I wouldn't be surprised if I'm wrong here.

Replying to hasienda:

OT: I found that TicketCustomFieldSubscriber is currently defined to require authentication, but this would prevent anonymouns users from setting an opt-out preference. But because Cc: fields may contain email addresses and custom Cc: fields may do so as well, this seems undesirable. Do you agree? If Yes, I'd change it right-away.

I think the answer to this lies in the answer to my previous comment, and the usefulness of my comments come down to whether or not I really understand the API doc. From the API doc, it sure sounds like the user can't be looked-up by email address in order to make a decision as to whether the email should be sent or not.

Replying to rjollos:

Replying to hasienda:

FullBlogBloggerSubscriber is similar to UserChangeSubscriber, so your assertion is true. I've checked the other subscribers too, and all of the don't require authentication.

I was about the commit this, but wait. Are you saying that all 5 of the subscribers for FullBlog don't require authentication?

Yes.

I currently have FullBlogWatchSubscriber and FullBlogMyPostSubscriber as requiring authentication. The former is because I was thinking that "watching" required authentication (see also comment:14). The latter is based on the comment from the API doc. <snip> The FullBlogMyPostSubscriber (notify me when any blog post that I created is changed) seems analogous to being a ticket reporter,

Yes indeed, at least in the default configuration, where anonymous (optionally with email, but still not authenticated) reporters are accepted.

which I assume would be similar to the ticket owner example described in the API doc.

No, exactly there is the difference: Ticket owners are required to be authenticated users, while reporters are not (per default). There is no such thing like blog owner in the blog realm, only you assume reporter == owner there, right? Subtle, but from tickets you know the difference. You can't alter the reporter == initial/original author, but assign the owner at will.

Additional note regarding the cited doc-string for requires_authentication:

While it's rather easy to code a 'sid-from-email' resolver, I'd only trust, if the email has been verified, i.e. by means of acct_mgt.register.EmailVerificationModule. Most critically: Never trust username - email associations from unauthenticated session IDs.

We have a patch (see above) but I wonder where it still has a little catch in it. Can I help out somehow?

I started working on it again last evening, which led to #10675. I should have the patch for this ticket committed by later today.

This one is the only issue that let my stay away from introducing 1.0 to the production system

I've made some progress. I had to jump onto some other work, but I hope to have this fixed soon.

Replying to falkb:

We have a patch (see above) but I wonder where it still has a little catch in it. Can I help out somehow?

IMHO the latest patch has many unrelated changes, that should not get committed with reference to this ticket. Would be rather hard to find out about the reason behind later on.

OTOH it seems to hold really critical changes to at least bring the blog support back into life. So I'm sorry for have allowed this to slip through. Its rather hard for me to take over from Ryan, because I don't have the blog plugin code in my own test environment, but if he could follow-up, I'd still consider picking out the relevant changes myself for everyone’s convenience.

Now that I'm confident working with Git, I'll plan to break up the patch to a proper series of changesets and post them to GitHub for review before commiting back here.