Ok, I have read:
http://drupal.org/node/1497016
http://drupal.org/node/1342632#comment-5450624
and og.test, but I still couldn't get my head around this other group thing, could someone provide a use case for this field?

Thanks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

For example, as admin you edit a node created by another user. If the node is assigned to a group that doesn't belong to the admin, that group will appear in the "other groups". Remember, the group-audience field options are per user (because each user is subscribed to different groups).

jrao’s picture

Ok, played this for a while, let me know if my understanding is correct:
1. This secondary field is only used for editing group content
2. When editing, the groups associated with the content is divided between primary field and secondary field, primary field will only show groups the current user belongs to
3. I'm guessing this design is chosen so that when a user cannot see all the groups the content associated with, he can still save the content and the save won't wipe out the groups he cannot see in the primary field.

amitaibu’s picture

Category: support » task
Priority: Minor » Normal
Status: Active » Needs review
FileSize
3.49 KB

> I'm guessing this design is chosen so that when a user cannot see all the groups the content associated with, he can still save the content and the save won't wipe out the groups he cannot see in the primary field.

Indeed, finally I got to implementing this -- Probably needs testing - @jrao, any chance you'll add some tests... ;)

jrao’s picture

I'd like to help, but I'm not sure I understand the purpose of this patch, I thought the primary/secondary implementation is already completed, i.e. if I'm admin1 of group1, and I edit node1 which belongs to group1 and group2, the edit page will show group1 in primary field, group2 in secondary field, and after save node1 is still linked to group1 and group2. What is the scenario this patch will fix?

amitaibu’s picture

Patch fixes the case where a non admin user edits the node, but they don't have access to the secondary field. The patch uses the "invisible" field to keep the "hidden" group ids

jrao’s picture

Ok, let's see if this works.

amitaibu’s picture

This is same patch form #6, only removed all the tabs from og.test, so it's easier to review. Review will follow.

amitaibu’s picture

Status: Needs review » Needs work

That's really great of you! Some nitpicking;

+++ b/og.moduleundefined
@@ -810,17 +811,41 @@ function og_field_attach_form($entity_type, $entity, &$form, &$form_state, $lang
+      entity_load($group_type, $gids);

It's a leftover, lets remove this.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+      'description' => 'Test primary/second field in UI, see http://drupal.org/node/1519702.',

No need to reference issue.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+  function setUp() {

There's a lot of setup going here, maybe we can add some PHPdocs to explain a bit what were doing.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    // Create group managers for these tests, need 'administer group' permission to see secondary field

Missing dot in end of line. Also make sure that comments don't pass the 80chars line.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+  protected function assertNoOption($id, $option, $message = '') {

Let's move this function to be the last one in the test Class.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    // Set Node access strict variable.
+    variable_set('og_node_access_strict', TRUE);

Isn't it strict by default? Anyway, this can probably be moved to the setup part.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    $this->assertResponse('200', t('Group manager allowed to access to edit group content node.'));

I think we don't need this assert - we have other tests that should cover it.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    $this->assertNoOption($field_id, $this->group2->nid, t('Group2 doesn\'t appear in primary field.'));

Instead of escaping (e.g. doesn\'t) we can start text with "

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    $this->assertText($this->group1->title, t('Group1 exists in group content node page.'));
+    $this->assertText($this->group2->title, t('Group2 exists in group content node page.'));

Since we are checking in next lines og_is_member(), I think we can remove these asserts.

+++ b/og.testundefined
@@ -824,3 +824,169 @@ class OgMigrate7000TestCase extends UpgradePathTestCase {
+    $this->assertNoText($instance['label'], t('Secondary field label does not exist.'));

Maybe better "...field isn't accessible by user."?

jrao’s picture

Status: Needs work » Needs review
FileSize
11.19 KB

Ok, should be fixed now.

amitaibu’s picture

I think your editor is still using tabs, and you have extra white space (use dreditor to see them).

jrao’s picture

Ok, white space and tab should be fixed now. Could I run dreditor against local patch files?

amitaibu’s picture

Status: Needs review » Fixed

> Could I run dreditor against local patch files?

No, but you can setup your Eclipse/ Aptana to trim whitespace, and tabs -> spaces.

Thanks for your great work on the tests -- you received your first commit credit in OG. I hope to give you more... :)

jrao’s picture

Ok, no problem, didn't know Eclipse could remove trailing whitespace, thanks for the tip.

amitaibu’s picture

Status: Fixed » Closed (fixed)

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