OG notifications_group update

This patch looks great, committed. Thanks.

About the questions I'll try to quickly reply all of them. For some of them better open a thread for each though.

1. It would be useful to have a switch in hook_notifications that will control display of subscription type on the notifications user page. In this particular case, the group type link and tab on the user/notifications page is rather redundant.

Seems like a good idea. The current system allows some more flexibility though. Open to discussion and patches (new thread better). A setting or a permission for display/hide such pages may make some sense too.

2. The group management page - user/notifications/group - is served by notifications_group_user_page. Unlike the other tabs, this page allows management of both groups and grouptypes (see screenshot in #1). The form is an amalgam of two notifications_user_form calls for group and grouptype which is separated again in the _submit function. My intention was / is to reuse the generic form rather than roll a separate version.

Each subscription type has its own UI specific needs, though we've tried to handle it in a way as generic as possible. However, most of the modules here just attempt to provide some simple subscription options and avoid to over complicate the user pages.

3. However, notifications_user_form seems to have issues with multi-field subscription types. Consequently, group types updates are handled directly in notifications_group_user_page_submit.

Check out the taxonomy user pages, maybe a similar system can be used. However if the current generic submit is not working for that I'd be a bit concerned of adding yet more complexity there, simple improvements will be welcomed though.

4. The group + grouptype form mimics the options available via the notifications_ui block, which IMO, should be recommended over hook_link options for OG installs. The UI block form is currently a little buggy, not displaying default values correctly, access control issues etc.

Yes, the UI part needs some polishing yet, patches are welcomed. Again we'll try to keep it as simple as possible. As I see it the greatest feature it has is not that it provides all the possible options, but that it can be disabled and replaced by some custom UI.

5. Niche cases are yet to be handled esp. for directly subscribed nodes. By default, private groups appear to still be accessible to the user even after unsubscribing. I'm also unsure how other OG sub-modules affect access control and whether we should play nice.

Yes, you're right that current UI is a bit loose on enforcing visibility of items. There's some additional access checking though on queue processing so view access to all nodes is checked before sending them out (This is is needed too in case a node is unpublished after a notification has been queued, etc...).

6. Should I receive a notification to group posts/comments I make myself? Does it affect threading? Digests?

There's a global option (Notifications settings) for that (working since Beta2, was not working before). This is useful for debugging, possibly not for anything else.

7. Duplicate mail: Currently the user can subscribe to both the group and node types within the group. While this can lead to duplicate notifications, this isn't necessarily a bad thing and is left upto the discretion of the user. The user could, for e.g., want his group notifications in a daily digest and story posts alone in PMs.

Right. This is handled on queue processing and these notifications are de-duped only for the same sending method and same interval. Basically, de-duping occurs when digesting, so no more than one notification for the same event is sent in the same message.
Now I think of it again, this may fail for inmediate sending (?)

8. Should all calls to og_mail (and the mail templates) be moved to notifications_group?

Not sure about it. I suggest you take a look at 'Notifications Lite' (module and help pages), maybe a fallback option would be useful for it.

9. job_queue still required?

For og? As notifications handles its own queueing I guess not.

10. How are unpublished groups handled in general?

TBD. We should check for access to groups/nodes before sending out messages, but before that I guess some 'subscriptions disabling' or hiding would help too so events aren't even queued.

11. The notifications tables could use better indexing.

Yes, possibly. Will be a good new thread :-)
Just to take into account, indexing may slow down inserts and as the framework is architected, it queues *all* notifications on page request to be processed later on cron, and this may be a critical performance point, so we should carefully decide which fields to index and why.
(However if we get any report about performance issues, the option is still open to just store events and do the queue population on cron too)

12. Terminology confusion with OG "subscriptions" and notification "subscriptions".
13. Should the module be renamed notifications_og?

This module was really basic and raw, now it looks like it's becoming something :-) So you are welcomed if you want to take ownership of it.

You can rename it or move it around packages. It may make more sense on the OG package itself or maybe as stand alone (if you want to grow it too much). If you prefer it here though, on this package, I can add you to the cvs committers list....

Thanks for this great patch and suggestions.

First, some feedback. Then a couple of buglets ...

7. We'll definately want to dedupe for immediate send as well. OG adds likelihood of duplicates.

8. Yes, we should be working toward this module completely replacing the current notifications in og.

9. Not needed

12. OG has changed - it now uses join/leave so the term 'subscribe' is no longer ambigous. It belongs to this module :)

13. When you guys are ready, I think we can move rename the module to og_notifications and move it to og project. Ideally Zen has time for a D6 port as well.

RESTRICT CONTENT TYPES
At admin/messaging/notifications/content, we see too many content types under 'Group subscriptions'. Get your list of types from og_get_types('group_post'). This is a new function og 5.7

MISSING GROUP NAMES
On comment form, the group subscription checkbox reads "Posts in group". The group name isn't showing because the variable $node->og_groups_names is gone in og 5.7. Replace with $node->og_groups_both[$gid]. Also, we should add group name to items like "Book page posts in this group".

Hey, this is great news!

Module removed

This is still showing up in my notifications. It is a duplicate entry to the one in notifications, so OG has updated but not this module. A patch for Notifications should come out. Duplicates appear to crash it.