1. Move all notifications related settings from og to og_notifications, converting, where appropriate, to use the messaging API.
  2. Delete unnecessary configuration options.
  3. Do not display configuration options on all user_edit forms - just the primary tab.
  4. Job queue recommendations have been removed.
  5. Reuse hook_og to communicate with notifications modules - in this case og_notifications - to relay administrative messages. The newly introduced ops are 'user request', 'user approve', 'user communicate', 'admin create' and 'admin new'. All the features and advantages of the messaging API are available for admins and other modules to customise as necessary. While most of these 'ops' are named after the message template variables already in use in og.module for consistency and possible reuse by other modules, 'user communicate' is for generic usage and is currently being used by the "e-mail all members" form. The invite form is yet to use this system.
  6. The messaging API's queueing system has been a little buggy during testing. Currently, forced queueing of bulk messages has been turned off.
  7. There might also be a better way to send and indicate bulk messages through the messaging API. Currently, recipients are looped through in og and messages transmitted individually. Needs to be investigated.
  8. The messaging API provides options to split messages into header, body and footer (etc.) - perhaps this can be made an administrative option in the future. The token module is also available for use as it is a pre-req for notifications.
  9. Notifications related fields and tables have been removed from .install. However, the upgrade path needs to be discussed before proceeding - please see notes in patch (in the .install file). Please also note in particular the changes triggered in og.install by the removal of og_uid_global.
  10. A number of configuration options have been retained in og.module rather than being moved to og_notifications in case other similar modules want to use them.

    11. During testing, using the "debug tool" messaging module will very likely prove helpful. Its output can be seen on the recipient's user account page. Please disable notifications_og and any og_mail related modules prior to applying the patch.

    This patch has only seen rudimentary testing and only with "simple mail". The D6 upgrade patch is based off this one and currently only satisfies coder module and is untested.

    Cheers,
    -K

CommentFileSizeAuthor
#3 notifications_og13.patch66.68 KBZen
notifications_og7.patch49.64 KBZen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

After a code review, I only found minor comments.

- i don't yet understand this: "'user communicate' is for generic usage". the way you are using it, i would rename the op to 'user broadcast'
- you removed a needed space at: + '!group_url'=> url("node/$node->nid", NULL, NULL, TRUE)
- remove the word 'organic': "t('Automatically enable notifications for any organic groups that I join.')"
- README.txt for OG ought to mention og_notifications module and its benefits. og_notifications should have its own brief README.
- please do investigate 6+7 and see what we can do better. we really need to use a queue for performance sake. sending a few thousand emails (yes, groups get that big) in real time is a killer.

Yes, I like your idea about doing the migration in og_notifications_enable() or og_notifications_install().

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I looked briefly at the cron features of Notifications

In particular the 'user_allowed' callback should be helpful for ensuring that the recipient is still subscribed to the group (might have cancelled since last cron run). If we don't do that, then we ought to delete from the notifications queue when a user leaves a group.

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
notifications_og13.patch66.68 KB
  1. Added og_notifications.install.
  2. Add new update to og.install.
  3. Remove mentions of dropped fields from og.module.
  4. Remove similar mentions and fields from views.inc.
  5. Add upgrade path to og_notifications.install.
  6. Removed replies hook.
  7. Removed instances of "job_queue"
  8. Remove all sections involving the use of og_uid_global.
  9. Avoid usage of "email". Replace with "notification" or "message".
  10. Fix forced queueing issues.
  11. Bulk messaging is buggy in the messaging module and an issue has been created for it. The relevant code has been commented out temporarily and a workaround is being used at the moment.
  12. Added support for e-mail messaging for use in the invite form. This has been disabled due to the bulk messaging bug. The invite form is still intact.
  13. Changed user communicate to user broadcast.
  14. A few minor string improvements.
  15. Mail / notification mentions in og_types_map and og_block & og_block_notifications need clarification.

A rudimentary upgrade function is in place. But, it needs testing and is yet to support the port of the "autosubscription" setting into the user table which is going to involve jumping through hoops. I'm looking to avoid user_loads as much as possible. IIRC, this is almost always accomplished with regex applied to the users.data field. If you are aware of a better way to do this, please let me know.

Testing: Can I get a sample database dump to test the upgrade? A sanitised g.d.o perhaps?

-K

Zen’s picture

Zen CreditAttribution: Zen commented

re: cron features - I'm pretty sure that notifications takes care of this automatically. The messaging queue, however, does not. og_notifications only uses the messaging module directly for administrative tasks and only bulk messages are queued.

-K

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I will take a look at the new patch. Thanks.

You should not need to peek into users.data at all. The autosubscribe column for a user is in og_uid_global table.

I will investigate providing a sanitized groups.drupal.org DB.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Needs work

Code review

* Why are admin messages delivered in a single delivery method and not in the recipient's desired method?
* Missing 2nd param: $flag = variable_get('og_notifications_update_required');

I am testing this now but am fighting with Notifications/Messaging a bit. Will post here again.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

So, it seems that og_notifications is not required but *some* similar module is required or else you will not get any admin email, the broadcast tab silently fails, etc. Any thoughts on how to resolve this? Perhaps we limit og_notifications to just caring about notification mail and not admin mail? Or we do a hook_requirements() which looks for implementations of 'user broadcast' hook (for example) and if there are none, it yells.

FYI, I am committing og_notifications to the HEAD branch just so that we have consistent files as requested at http://drupal.org/node/254655. It certainly isn't working there yet.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

@Zen - perhaps we should use notifications_lite to send our messages? it can be found in the notifications package and discussed at http://drupal.org/node/253109

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

So, what I think we should do is create a new DRUPAL-5-3 branch and commit this and then keep working on it there. Zen is welcome to do that at any time.

* need help text at top of user/[n]/notifications/group

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs work » Active

Issues that you've mentioned which have been fixed:

  • notifications_lite replaces the direct calls to messaging module.
  • Invitation is no longer being handled by og_notifications (due to above). It should, in any case, be a separate module or part of the invite module (or something like that ...).
  • Help text has been added.
  • og_uid_globals has been resurrected. Still needs some work.
  • queueing and bulk messaging appears to be working well now.

Broadcast tab failing silently when og_notifications is not enabled: Perhaps we can just make it a warning via hook_requirements?

-K

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Active » Fixed

This has been committed to 5 and 6. Please file new issues as needed.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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