Fix upgrader activity dependencies

Currently the full-upgrade alarm calls the 3 upgrade-related-alarm's activeSense methods in activities. As activeSense method runs Alarm's methods in activities too, the final activities do not get the good "tag" and "after_tag" values, which creates a lack of depency between them. In consequence, if we are not running the real upgrade (= just activeSense), no need to run post-upgrade's activeSense : indeed, post-upgrade constraints will be run on what are *currently* installed, and not on the result of the previous steps of the upgrader, which lead to a wrong and misleading consistency check.

This Merge Request intends to fix it by : 
   * Adding a "activate_kw" parameter to activeSense method, which allow to configure its internal call to "activate"
   * Improving upgrader to remove the useless calls to activeSense, and properly parameterize activeSense's activity generation 

/reviewed-on nexedi/erp5!276

bt5/erp5_upgrader/PathTemplateItem/portal_alarms/promise_check_upgrade.xml

@@ -24,16 +24,13 @@
         </item>
         <item>
             <key> <string>description</string> </key>
-            <value> <string>This alarm checks that the upgrader is required. If yes, the upgrader have three steps:\n
+            <value> <string>This alarms controls the upgrade process by calling others upgrader alarms.\n
 \n
-1. Pre-upgrade\n
-This step will run all constraints that have contraint_type equal pre_upgrade.\n
+It delegates sense to calling fix on "Run Pre Upgrade" then "Run Upgrade" alarms an aggregating the results.\n
 \n
-2. Upgrader\n
-This step will run all constraints that have contraint_type equal upgrade. In this step is supposed to install/upgrade all Business Template required.\n
+It delegate fix to calling fix on "Run Pre Upgrade" then "Run Upgrade" then "Run Post Upgrade" alarms then aggregating the results.\n
 \n
-3. Post-Upgrade\n
-This step will run all constraints that have contraint_type equal post_upgrade.</string> </value>
+Note that post upgrade steps are not executed in the sense, because they require the upgrade step to be actually executed.</string> </value>
         </item>
         <item>
             <key> <string>enabled</string> </key>

bt5/erp5_upgrader/SkinTemplateItem/portal_skins/erp5_upgrader/Alarm_runFullUpgrader.py

@@ -12,27 +12,42 @@ pre_upgrade_tag = '%s-preupgrade' % tag
 upgrade_tag = '%s-upgrade' % tag
 post_upgrade_tag = '%s-postupgrade' % tag
 
-portal_alarms.upgrader_check_pre_upgrade.activate(
-  activity='SQLQueue',
-  tag=pre_upgrade_tag,
-).activeSense(fixit=fixit, params={'tag': pre_upgrade_tag})
+active_process = context.newActiveProcess()
 
-portal_alarms.upgrader_check_upgrader.activate(
-  activity='SQLQueue',
-  tag=upgrade_tag,
-  after_tag=pre_upgrade_tag,
- ).activeSense(fixit=fixit, params={'tag': upgrade_tag})
 
-portal_alarms.upgrader_check_post_upgrade.activate(
-  activity='SQLQueue',
-  tag=post_upgrade_tag,
-  after_tag=upgrade_tag,
-).activeSense(fixit=fixit, params={'tag': post_upgrade_tag})
+portal_alarms.upgrader_check_pre_upgrade.activeSense(
+  fixit=fixit,
+  activate_kw={
+    'activity': 'SQLQueue',
+    'tag': pre_upgrade_tag,
+  },
+  params={'tag': pre_upgrade_tag},
+)
+
+portal_alarms.upgrader_check_upgrader.activeSense(
+  fixit=fixit,
+  activate_kw={
+    'activity': 'SQLQueue',
+    'tag': upgrade_tag,
+    'after_tag': pre_upgrade_tag,
+  },
+  params={'tag': upgrade_tag},
+)
+
+if fixit:
+  portal_alarms.upgrader_check_post_upgrade.activeSense(
+    fixit=fixit,
+    activate_kw={
+      'activity': 'SQLQueue',
+      'tag': post_upgrade_tag,
+      'after_tag': upgrade_tag,
+    },
+    params={'tag': post_upgrade_tag},
+  )
 
 
 # start another activity to collect the results from each upgrader step
-active_process = context.newActiveProcess()
-context.activate(after_tag=post_upgrade_tag).Alarm_postFullUpgradeNeed(
+context.activate(after_tag=(upgrade_tag, post_upgrade_tag)).Alarm_postFullUpgradeNeed(
   active_process=active_process.getRelativeUrl())
 
 # Nothing else to do, so we can disable.

bt5/erp5_upgrader/SkinTemplateItem/portal_skins/erp5_upgrader/Alarm_runPostUpgrade.py

@@ -4,7 +4,7 @@
 activate_kw = params or {}
 
 with context.defaultActivateParameterDict(activate_kw, placeless=True):
-  active_process = context.newActiveProcess()
+  active_process = context.newActiveProcess(activate_kw=activate_kw)
 
 context.ERP5Site_checkUpgraderConsistency(fixit=fixit,
   activate_kw=activate_kw,

bt5/erp5_upgrader/SkinTemplateItem/portal_skins/erp5_upgrader/Alarm_runPreUpgrade.py

@@ -4,7 +4,7 @@
 activate_kw = params or {}
 
 with context.defaultActivateParameterDict(activate_kw, placeless=True):
-  active_process = context.newActiveProcess()
+  active_process = context.newActiveProcess(activate_kw=activate_kw)
 
 context.ERP5Site_checkUpgraderConsistency(fixit=fixit,
   activate_kw=activate_kw,

bt5/erp5_upgrader/SkinTemplateItem/portal_skins/erp5_upgrader/Alarm_runUpgrader.py

@@ -17,7 +17,7 @@ if tool_portal_type in portal_type_list:
 activate_kw = params or {}
 
 with context.defaultActivateParameterDict(activate_kw, placeless=True):
-  active_process = context.newActiveProcess()
+  active_process = context.newActiveProcess(activate_kw=activate_kw)
 
   method_kw = {'fixit': fixit,
     'filter': {"constraint_type": 'upgrader'},

bt5/erp5_upgrader/TestTemplateItem/portal_components/test.erp5.testUpgrader.py

@@ -135,12 +135,19 @@ class TestUpgrader(ERP5TypeTestCase):
         id=script_constraint_id)
     script_constraint.edit(script_id=self.script_id, constraint_type="pre_upgrade")
 
-  def stepCreatePersonPropertySheet(self, sequence=None):
+  def _createPersonEmptyPropertySheet(self):
     portal = self.portal
     property_sheet = getattr(portal.portal_property_sheets, self.property_sheet_id, None)
     if property_sheet is None:
       property_sheet = portal.portal_property_sheets.newContent(
         portal_type="Property Sheet", id=self.property_sheet_id)
+    return property_sheet
+
+  def stepCreatePersonEmptyPropertySheet(self, sequence=None):
+    self._createPersonEmptyPropertySheet()
+
+  def stepCreatePersonPropertySheet(self, sequence=None):
+    property_sheet = self._createPersonEmptyPropertySheet()
     script_constraint_id = "person_old_reference_constraint"
     script_constraint = getattr(property_sheet, script_constraint_id, None)
     if script_constraint is None:
@@ -306,6 +313,11 @@ class TestUpgrader(ERP5TypeTestCase):
       alarm_id="promise_check_upgrade")
     self.assertTrue(sense, detail_list)
 
+  def stepCheckFullUpgradeNotRequired(self, sequence=None):
+    sense, detail_list = self._checkAlarmSense(
+      alarm_id="promise_check_upgrade")
+    self.assertFalse(sense, detail_list)
+
   def stepCheckPostUpgradeNotRequired(self, sequence=None):
     sense, detail_list = self._checkAlarmSense(
       alarm_id="upgrader_check_post_upgrade")
@@ -743,3 +755,97 @@ class TestUpgrader(ERP5TypeTestCase):
     """
     sequence_list.addSequenceString(sequence_string)
     sequence_list.play(self)
+
+  def test_sense_full_upgrade_do_not_sense_post_upgrade(self):
+    """
+    Check that the post-upgrade consistency check is not run
+    when running the activeSense method of the full-upgrade alarm,
+    as post-upgrade will give inconsistent result
+    """
+    sequence_list = SequenceList()
+    sequence_string = """
+      stepRunUpgrader
+      stepTic
+      stepCreatePerson
+      stepValidatePerson
+      stepSetConstraintInPersonModulePortalType
+      stepTic
+      stepCheckFullUpgradeNotRequired
+      stepCheckPostUpgradeRequired
+    """
+    sequence_list.addSequenceString(sequence_string)
+    sequence_list.play(self)
+
+  def _setPersonTitleConstraintForUpgraderStep(self, step):
+    portal = self.portal
+    skin_folder = portal.portal_skins.custom
+    script_id = "Person_%s_setTitleConstraint" % step
+    custom_script = getattr(skin_folder, script_id, None)
+    if custom_script is None:
+      skin_folder.manage_addProduct['PythonScripts'].manage_addPythonScript(script_id)
+      custom_script = getattr(skin_folder, script_id)
+      custom_script.ZPythonScript_edit('fixit=False, **kw',
+        "{step_string}='M. {step_string}'\n"
+        "if context.getTitle() != {step_string}:\n"
+        "  if fixit:\n"
+        "    context.setTitle({step_string})\n"
+        "  else:\n"
+        "    return [\"Person's title is wrong\",]\n"
+        "return []".format(step_string=step)
+      )
+
+    property_sheet_id = self.property_sheet_id
+    property_sheet = getattr(portal.portal_property_sheets, property_sheet_id, None)
+    script_constraint_id = "person_title_%s_constraint" % step
+    script_constraint = getattr(property_sheet, script_constraint_id, None)
+    if script_constraint is None:
+      script_constraint = property_sheet.newContent(
+        portal_type="Script Constraint",
+        id=script_constraint_id
+      )
+    script_constraint.edit(
+      script_id=script_id,
+      constraint_type="%s" % step)
+
+  def stepSetPersonTitlePreUpgradeConstraint(self, sequence=None):
+    self._setPersonTitleConstraintForUpgraderStep('pre_upgrade')
+
+  def stepSetPersonTitleUpgradeConstraint(self, sequence=None):
+    self._setPersonTitleConstraintForUpgraderStep('upgrader')
+
+  def stepSetPersonTitlePostUpgradeConstraint(self, sequence=None):
+    self._setPersonTitleConstraintForUpgraderStep('post_upgrade')
+
+  def stepCheckPersonTitleHasBeenSetByPersonTitlePostUpgradeConstraint(self, sequence=None):
+    person = sequence['person']
+    title = person.getTitle()
+    self.assertEqual(title, 'M. post_upgrade')
+
+  def stepCheckPersonTitleHistory(self, sequence=None):
+    person = sequence['person']
+    self.assertEqual(
+      [x.changes for x in self.portal.person_module['1'].Base_getZODBHistoryList()[-3:]],
+      [('title:M. pre_upgrade',), ('title:M. upgrader',), ('title:M. post_upgrade',)])
+
+  def test_upgrade_activities_are_run_sequentially(self):
+    """
+    Check that activities spawned by the upgrader are always run in the same
+    order : pre-upgrade, upgrade, post-upgrade
+    """
+    sequence_list = SequenceList()
+    sequence_string = """
+      stepCreatePerson
+      stepValidatePerson
+      stepCreatePersonEmptyPropertySheet
+      stepSetConstraintInPersonPortalType
+      stepSetPersonTitlePreUpgradeConstraint
+      stepSetPersonTitleUpgradeConstraint
+      stepSetPersonTitlePostUpgradeConstraint
+      stepTic
+      stepRunFullUpgrader
+      stepTic
+      stepCheckPersonTitleHasBeenSetByPersonTitlePostUpgradeConstraint
+      stepCheckPersonTitleHistory
+    """
+    sequence_list.addSequenceString(sequence_string)
+    sequence_list.play(self)
\ No newline at end of file

product/ERP5/Document/Alarm.py

@@ -103,7 +103,7 @@ class Alarm(XMLObject, PeriodicityMixin):
     return '%s_%s' % (self.getRelativeUrl(), id)
 
   security.declareProtected(Permissions.AccessContentsInformation, 'activeSense')
-  def activeSense(self, fixit=0, params=None):
+  def activeSense(self, fixit=0, activate_kw=(), params=None):
     """
     This method launches the sensing process as activities.
     It is intended to launch a very long process made
@@ -113,6 +113,8 @@ class Alarm(XMLObject, PeriodicityMixin):
     The result of the sensing process can be obtained by invoking
     the sense method or by requesting a report.
     """
+    activate_kw = dict(activate_kw)
+
     portal_membership = self.getPortalObject().portal_membership
     if fixit or not self.getEnabled():
       checkPermission = portal_membership.checkPermission
@@ -135,7 +137,9 @@ class Alarm(XMLObject, PeriodicityMixin):
         # able to notify the user after all processes are ended
         # We do some inspection to keep compatibility
         # (because fixit and tag were not set previously)
-        tag = self.__getTag(True)
+        if 'tag' not in activate_kw:
+          activate_kw['tag'] = self.__getTag(True)
+        tag = activate_kw['tag']
         method = getattr(self, method_id)
         func_code = method.func_code
         try:
@@ -149,13 +153,13 @@ class Alarm(XMLObject, PeriodicityMixin):
         name_list = func_code.co_varnames[:func_code.co_argcount]
         if 'params' in name_list or has_kw:
           # New New API
-          getattr(self.activate(tag=tag), method_id)(fixit=fixit, tag=tag, params=params)
+          getattr(self.activate(**activate_kw), method_id)(fixit=fixit, tag=tag, params=params)
         elif 'fixit' in name_list:
           # New API - also if variable number of named parameters
-          getattr(self.activate(tag=tag), method_id)(fixit=fixit, tag=tag)
+          getattr(self.activate(**activate_kw), method_id)(fixit=fixit, tag=tag)
         else:
           # Old API
-          getattr(self.activate(tag=tag), method_id)()
+          getattr(self.activate(**activate_kw), method_id)()
         if self.isAlarmNotificationMode():
           self.activate(after_tag=tag).notify(include_active=True, params=params)
 
@@ -163,6 +167,7 @@ class Alarm(XMLObject, PeriodicityMixin):
     # is always invoked by system user.
     sm = getSecurityManager()
     newSecurityManager(None, nobody)
+
     try:
       _activeSense()
     finally: