ERP5: check constraints in notificiation tool's sendMessage

product/ERP5/Tool/NotificationTool.py

@@ -233,6 +233,7 @@ class NotificationTool(BaseTool):
                   attachment_list=None, attachment_document_list=None,
                   notifier_list=None, priority_level=None,
                   store_as_event=False,
+                  check_consistency=True,
                   message_text_format='text/plain',
                   event_keyword_argument_dict=None,
                   portal_type_list=None):
@@ -271,6 +272,13 @@ class NotificationTool(BaseTool):
       store_as_event -- whenever CRM is available, store
                         notifications as events
 
+      check_consistency -- Check that the created events match their constraints.
+                           If any of the event have an unsatisified constraint, a
+                           ValueError is raised.
+                           Note that if `store_as_event` is true, some draft
+                           events are created anyway, so caller may want to
+                           abort transaction.
+
       event_keyword_argument_dict -- additional keyword arguments which is used for
                                      constructor of event document.
 
@@ -317,12 +325,6 @@ class NotificationTool(BaseTool):
             raise IndexError, "Can't find person document which reference is '%s'" % person
           else:
             person = person_value
-        email_value = person.getDefaultEmailValue()
-        if email_value is None:
-          # For backward compatibility. I recommend to use ValueError.(yusei)
-          raise AttributeError, "Can't find default email address of %s" % person.getRelativeUrl()
-        if not email_value.asText():
-          raise AttributeError, "Default email address of %s is empty" % person.getRelativeUrl()
         to_person_list.append(person)
 
     # prepare low-level arguments if needed.
@@ -367,6 +369,12 @@ class NotificationTool(BaseTool):
       event.setAggregateValueList(attachment_document_list)
       event_list.append(event)
 
+    if check_consistency:
+      for event in event_list:
+        constraint_message_list = event.checkConsistency()
+        if constraint_message_list:
+          raise ValueError(constraint_message_list)
+
     for event in event_list:
       if event.isTempObject() or (not portal.portal_workflow.isTransitionPossible(event, 'start')):
         event.send(**low_level_kw)

product/ERP5/tests/testNotificationTool.py

@@ -369,11 +369,9 @@ class TestNotificationTool(ERP5TypeTestCase):
     """
     Check that notification fails when the destination hasn't a email adress
     """
-    self.assertRaises(
-      AttributeError,
-      self.portal.portal_notifications.sendMessage,
-      recipient='userWithoutEmail', subject='Subject', message='Message'
-    )
+    with self.assertRaises(ValueError):
+      self.portal.portal_notifications.sendMessage(
+          recipient='userWithoutEmail', subject='Subject', message='Message')
 
   def test_08_PersonWithoutEmail(self):
     sequence_list = SequenceList()

[Romain Courteaud]

@jerome this changes introduces compatibility issues, if constraint checking is activated by default.

Then, wouldn't it be better to raise a ValidationFailed error instead of a ValueError. ValidationFailed at least is handled by Base_workflowStatusModify, and so, error will be correctly displayed to user if email sending is rejected during a workflow transition.

[Jérome Perrin]

@romain thanks, you're right. This change does break compatibility when using portal_notification.sendMessage to send email notification with some constraints defined on Mail Message portal type.

I am working on a patch making check_consistency False by default. The check for recipient email address are anyway done again in MailMessage_send so this case will fail early enough and we do not need to have this check in notification tool (this we our problem in the first place).

---

About raising a ValidationFailed, one potential problem is that Base_workflowStatusModify commits the transactions, so if we pass store_as_event=True, we may keep in event_module some events that were not actually sent, especially when multiple recipients.

Another problem might be that the constraint message will likely not be user friendly enough in that context (because they are usually written as an validation error when validating the event).

This check_consistency does not change the fact that the caller of sendMessage is expected to check that conditions are OK before calling sendMessage. This check_consistency is just so that we have an error instead of creating invalid events. I never saw it as something that should be used for user interaction.

[Jérome Perrin]

I did that in a31abf5b it's in for_tesrunner_1, we'll see the results and I'll merge that tomorrow morning. If you prefer I can also revert for now.

[Jérome Perrin]

There was a problem in that first commit. I amended it as 3d1a8811 and now tests are passing:

https://nexedi.erp5.net/test_result_module/20161007-5AA51526

https://nexedi.erp5.net/test_result_module/20161007-86BBCD2

OK to merge this one ?

[Romain Courteaud]

ok