Comments

jrao’s picture

Status: Active » Needs review
StatusFileSize
new23.77 KB
berdir’s picture

Status: Needs review » Needs work

Thanks for the patch, review is below.

You ported the existing roles tests to privatemsg_group, right? It wouldn't be necessary to have all of them, because it *should* not matter for the recipient type implementing module if the recipients are added through the batch or cron, the roles tests are at the same time also tests for the privatemsg recipient type API. However, doesn't hurt to have them.

+++ b/privatemsg_groups/privatemsg_groups.infoundefined
@@ -0,0 +1,8 @@
+files[]=privatemsg_groups.module

Its not necessary to list the .module file here. That is always included anyway.

+++ b/privatemsg_groups/privatemsg_groups.infoundefined
@@ -0,0 +1,8 @@
+files[]=privatemsg_groups.test
\ No newline at end of file
diff --git a/privatemsg_groups/privatemsg_groups.module b/privatemsg_groups/privatemsg_groups.module

git diff doesn't like not having an empty line at the end of a file, so let's add one.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+/**
+ * Implements hook_perm().

Should be hook_permissions().

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+      'description' => t('Allows to write messages to all users which belong to own organic group.'),
+    ),    ¶
+    'view organic groups recipients' => array(

Trailing spaces.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+    return TRUE;
+  }
+  ¶
+  // Check if user has permission to write to own groups.
+  if (!user_access('write privatemsg to own organic groups')) {
+    return FALSE;
+  }
+    ¶
+  if ($recipient) {
+    $group_type = _privatemsg_groups_get_group_type($recipient->type);
+    $gid = $recipient->recipient;
+    return og_user_access($group_type, $gid, 'write privatemsg to group');
+  }
+    ¶
+  // Give access only to users that have group subscriptions.
+  $gids = og_get_groups_by_user();

More trailing spaces here. Also many more below. Try looking at the git diff in console that supports colors, that should highlight them. You could also configure your IDE/editor to remove them, most support that.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+  $recipients = array();
+  foreach ($gids as $gid) {
+    $group_type = _privatemsg_groups_get_group_type($type);
+    $group = entity_load_single($group_type, $gid);
+    $recipient = clone $group;

$type is the same to all $gids so it should be possible to load them all together using entity_load() and then loop over the loaded entities to add the recipient properties. That should be faster as it only executes a single query.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+    ->propertyCondition('entity_type', 'user')
+    ->propertyCondition('state', OG_STATE_ACTIVE);
+  return $query->count()->execute();;  ¶

Two semicolons.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+  $group_type = _privatemsg_groups_get_group_type($type);
+  $gids = og_get_all_group($group_type); ¶
+  $groups = entity_load($group_type, $gids);
+  $suggestions = array();
+  foreach ($groups as $gid => $group) {

Is it not possible to query for this directly? I assume that there could be a large amount of groups on a site...

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+      $name = entity_label($group_type, $group);
+      if (strpos($name, $fragment) !== false && !in_array($name, $names)) {

Ah, I see, because we depend on the label. Hm, that is unfortunate...

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+  // Get all type keys except user.
+  $types = privatemsg_recipient_get_types();
+  unset($types['user']);
+  $types = array_keys($types);
+
+  foreach ($types as $type) {
+    if (!_privatemsg_groups_is_group_recipient_type($type)) {
+      continue;

Can't we use og_get_all_group_bundle() here and then build $type? Messing with privatemsg_recipient_get_types() here is kinda weird.

+++ b/privatemsg_groups/privatemsg_groups.moduleundefined
@@ -0,0 +1,248 @@
+function _privatemsg_groups_get_group_recipient_type($group_type) {
+  return 'og:' . $group_type;
+}
+
+function _privatemsg_groups_is_group_recipient_type($group_recipient_type) {
+  return strpos($group_recipient_type, 'og:', 0) === 0;
+}
+
+function _privatemsg_groups_get_group_type($group_recipient_type) {
+  return str_replace('og:', '', $group_recipient_type);

A one line docblock for these would be nice.

+++ b/privatemsg_groups/privatemsg_groups.testundefined
@@ -0,0 +1,352 @@
+      'name' => 'Privatemsg Groups functionality',
+      'description' => 'Tests sending messages to organic groups',
+      'group' => 'Privatemsg',
+    	'dependencies' => array('og'),

Tabs here, also a few below in setUp().

+++ b/privatemsg_groups/privatemsg_groups.testundefined
@@ -0,0 +1,352 @@
+    // add user as group member

Should be formatted according to the coding standard: // Add user as group member.

+++ b/privatemsg_groups/privatemsg_groups.testundefined
@@ -0,0 +1,352 @@
+    ¶
+    drupal_static_reset('privatemsg_recipient_get_types'); //REMINDER: need to patch privatemsg_recipient_get_types: $types = &drupal_static(__FUNCTION__, NULL);

No need for that reminder comment here. Especially because it has been commited now :)

+++ b/privatemsg_groups/privatemsg_groups.testundefined
@@ -0,0 +1,352 @@
+
+  function testSendMessagetoGroupAPI() {
+    $recipient = clone $this->group1;

A short docblock with a single sentence describing what's tested would be great. Also for those below. I take these don't existing privatemsg_roles.test either...

jrao’s picture

Status: Needs work » Needs review
StatusFileSize
new24.15 KB

Ok, all fixed now.

Sylense’s picture

Does this patch only work with OG 7.x-2.x? I tried it with OG 7.x-1.4 and the private messages screen doesn't render properly

jrao’s picture

Yes. OG 7.x-1.x and 7.x-2.x has an architecture difference, see #1342632: Deprecate OG group entity

nicodv’s picture

Hello, first, thanks for the module, i was so needing it.
I'm not a coder, but when I install it, i get this error:

PHP Fatal error: Call to undefined function og_get_all_group_bundle() in /Applications/MAMP/htdocs/tester/sites/all/modules/contrib/privatemsg_groups/privatemsg_groups.module on line 63

Any idea what to do?

thanks
nico

jrao’s picture

Please make sure you're using og 7.x-2.x, this won't work with 7.x-1.x

berdir’s picture

Can we maybe add a versioned dependency here?

jrao’s picture

Sorry, I'm not sure how to add it, any examples?

berdir’s picture

Have a look at http://drupal.org/node/542202.

Adding a (2.x) suffix should work, the problem is that it is a bit problematic with git checkout because that has no version (git_deploy.module helps there). Please try it out if it works.

nicodv’s picture

My apologies, i read it but forgot it. Now i installed everything in a new test site and i get this errors when i try to type a name in the To: field in the write a message form:

Warning: Missing argument 4 for privatemsg_groups_autocomplete(), called in /Applications/MAMP/htdocs/001/sites/all/modules/contrib/privatemsg/privatemsg.pages.inc on line 702 and defined in privatemsg_groups_autocomplete() (line 189 of /Applications/MAMP/htdocs/001/sites/all/modules/contrib/privatemsg_groups/privatemsg_groups.module).

Notice: Undefined variable: type in privatemsg_groups_autocomplete() (line 190 of /Applications/MAMP/htdocs/001/sites/all/modules/contrib/privatemsg_groups/privatemsg_groups.module).

Notice: Undefined index: controller class in entity_get_controller() (line 7655 of /Applications/MAMP/htdocs/001/includes/common.inc).

BTW, i didn't have to apply any patch, the module is already included in the 7.x-2.x version, right? At least I compared both modules and there were no diff.

Sylense’s picture

Would it be possible to backport the patch to work with OG 7.x-1.x?

jrao’s picture

StatusFileSize
new24.15 KB

og 7.x-2.x dependency added, if og is checked out from git you'll need git_deploy to give it a version for the dependency to work.

jrao’s picture

nicodv: Please make sure you're using latest dev version of privatemsg, I don't think privatemsg_groups is included in 7.x-2.x yet, it's only available in patch format

Sylense: I'm sure it's possible but I'm not sure there's a point to do this since og is moving away from 7.x-1.x architecture.

Sylense’s picture

Thanks jrao. I actually went ahead and successfully uninstalled and upgraded to 7.x-2.x and it works great.

berdir’s picture

Status: Needs review » Fixed

Thanks, commited.

audriusb’s picture

Hi,

I needed version for OG 1.x and backported it myself. Here is the patch.

Status: Fixed » Closed (fixed)

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

jayaram’s picture

Do you have any issues ? Is it working ?

I'm also looking for a similar functionality for OG 7 - 1.x

I used to back ported patch submitted by audriusb but i get an error which says

"Fatal error: __clone method called on non-object in C:\xampp\htdocs\drupal7\sites\all\modules\privatemsg\privatemsg_groups\privatemsg_groups.module on line 188"

Did anyone else get this and know how to fix it ?

jayaram’s picture

MY bad, its an error in my database! the backport for OG 1.x works!

audriusb’s picture

I am glad You figured out what was wrong

hanskuiters’s picture

I used the patch in #13 to create the module. But it doesn't seem to do anything. On creating a new private message the autocomplete shows all users, not just the ones in my group. I tested as group administrator and as group member. I believe I've set all the permissions right (/admin/people/permissions and /admin/config/group/permissions).

I use OG 7.x-2.x-beta2 and Private messages 7.x-2.x-dev

Am I missing something?

hanskuiters’s picture

Well, I did miss something. I missed that the code from #13 was allready in the Private Messages module. So I deleted and uninstalled my module, cleared the cache and used the default Private Message OG. But still no luck. Again I can see all users, not just the ones in my group.

berdir’s picture

That's not what this module does. It provides groups as recipients. Implementing access control based on groups is something different and will need to be implemented in a new issue.

hanskuiters’s picture

Ok, thanks. So I missed something again, sorry for that.

goaldg’s picture

Hi,

I'm using OG 7.x-1.5 and Privatemsg 7.x-1.3 with no problem.

Then I wanted to add Private Msg OG and it didnt work.

As i'm using the 7.x-1x OG version I tried to apply the patch in #17 to Privatemsg 1.x-1.3, then to Privatemsg 1.x-1.x-dev, and then to Privatemsg 1.x-2.x-dev having different error messages (i should have copy..) when activating the module.

Maybe i just dont know how correctly apply that patch which seems to work ... which version of each module should i use ? Privatemsg 1.x-2.x-dev and then replace the /privatemsg_groups/privatemsg_groups.module files by the patch in #17 ? Is that right ?

Thx ..

sreher’s picture

Hi,
this patch works great. Thanks, especially audriusb for backporting to 1.x.

At the moment the admin can write to all group members and they could answer him.
The answers are not displayed to the other participants.

Is there a possibility to show all messages in a group thread to all participants?