This is handled by Organization being a mandatory field on the stormperson form.

We should do a quick check in stormperson_form() for the existence of organizations, and throw an appropriate error if there are none found.

CommentFileSizeAuthor
#13 storm-1820170-organization_checks-13.patch731 byteswillwh
#11 storm-1820170-organization-checks-11.patch733 byteswillwh
#9 storm-1820170-organization-checks-9.patch739 byteswillwh
#6 storm-1820170-organization_checks-4.patch738 byteswillwh
#3 storm-1820170-organization_check-3.patch1.05 KBwillwh
#1 storm-1820170-organization_check-1.patch1.05 KBwillwh

Comments

willwh’s picture

willwh commented
Status: Active » Needs review
StatusFileSize
new storm-1820170-organization_check-1.patch1.05 KB

The actual error thrown here could probably use some tidy up.

If you want to test this patch - make sure you start with a fresh storm install (i.e. run 'drush stre' or 'drush storm-reinstall' - or do this manually, disable > uninstall > renable all modules.)

juliangb’s picture

juliangb commented
Status: Needs review » Needs work 
+++ b/stormperson/stormperson.module
@@ -359,10 +359,13 @@ function stormperson_form(&$node) {
+      foreach ($result as $organization) {
+        $organizations[$organization->nid] = $organization->title;
+        if (!isset($node->organization_nid)) $node->organization_nid = $organization->nid;

Not sure why the indentation has changed for this bit?

+++ b/stormperson/stormperson.module
@@ -359,10 +359,13 @@ function stormperson_form(&$node) {
+      } if(!count($organizations) > 0) {
+        $message = t('You must first (!link).', array('!link' => l('add an Organization', 'node/add/stormorganization')));
+        drupal_set_message($message, 'error');
+      }

We should start the if on a new line, and also add a blank line before the org_nid field just to aid readability.

!count($orgs) > 0 sounds like a double negative. How about if(empty($orgs))?

For the message, sounds on the right lines, but I'd add "before you can add a Person", for some extra clarity.

willwh’s picture

willwh commented
Status: Needs work » Needs review
StatusFileSize
new storm-1820170-organization_check-3.patch1.05 KB

Like so? :)

juliangb’s picture

juliangb commented
Status: Needs review » Needs work 
+++ b/stormperson/stormperson.module
@@ -359,10 +359,15 @@ function stormperson_form(&$node) {
-  foreach ($result as $organization) {
-    $organizations[$organization->nid] = $organization->title;
-    if (!isset($node->organization_nid)) $node->organization_nid = $organization->nid;
-  }
+    foreach ($result as $organization) {
+      $organizations[$organization->nid] = $organization->title;
+
+      if (!isset($node->organization_nid)) $node->organization_nid = $organization->nid;
+    }

I know it's splitting hairs, but still a bit confused as to why the indentation has changed...

juliangb’s picture

juliangb commented
Status: Needs work » Needs review

Didn't mean to change status (dreditor does it automatically)... since we are just talking about alignment rather than functionality ;)

willwh’s picture

willwh commented
StatusFileSize
new storm-1820170-organization_checks-4.patch738 bytes

Okay........ like so? :)

Status: Needs review » Needs work

The last submitted patch, storm-1820170-organization_checks-4.patch, failed testing.

juliangb’s picture

juliangb commented 
+++ b/stormperson/stormperson.module
@@ -363,6 +363,10 @@ function stormperson_form(&$node) {
   }
+    if (empty($organizations) {

We've lost a closing bracket.

willwh’s picture

willwh commented
StatusFileSize
new storm-1820170-organization-checks-9.patch739 bytes

Here we go :)

willwh’s picture

willwh commented
Status: Needs work » Needs review
willwh’s picture

willwh commented
StatusFileSize
new storm-1820170-organization-checks-11.patch733 bytes

Let's try this for readability.

juliangb’s picture

juliangb commented
Status: Needs review » Needs work 
+++ b/stormperson/stormperson.module
@@ -363,6 +363,11 @@ function stormperson_form(&$node) {
+    $message = t('Before you add a Person, you must first (!link).', array('!link' => l('add an Organization', 'node/add/stormorganization')));

Final thing (was double checking the patch applies well), is we should remove the brackets around !link. The message looks a bit odd otherwise.

willwh’s picture

willwh commented
Status: Needs work » Needs review
StatusFileSize
new storm-1820170-organization_checks-13.patch731 bytes

Roger that!

juliangb’s picture

juliangb commented
Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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