Always subscribe poral_alarms. (8a40bb1c) · Commits · nexedi / erp5

portal_alarms has always just worked straight from instance creation for as long as I can remember, so what is creating the need for such change now ? The commit message is way too short to understand this change.

@vpelletier , actually it was never subscribed when site is created. I found by incident during Wendelin's configuration where we have alarms which are never executed.

You can check internal forum , too. Thread : " "standalone" scripts doesn't enable portal_alarms".

And without being subscribed (to Timer Service) no alarm is triggered.

it was never subscribed when site is created

Which is not my experience, hence my puzzling. This looks like a recent (?) regression, or maybe the way to create the ERP5Site would be somehow different from "normal" way ("create from ZMI" ? I don't create an ERP5 instance so often, so I can't describe for certain the last one I did create).

Or am I completely misremembering ? But I would be very surprised.

And a comment closer to the actual patch: this method contains several if [not] update: blocks. In what cases is it called ? I could only find one caller, in the same file. If it can be called during Products/BTs upgrades or migrations, I would very much prefer that these events do not force resubscribing alarm_tool. I have instances where I chose to disable alarm tool, and do not want it to magically restart ticking.

@vpelletier , I tested both ways

Adding site by *-standalone script machinery
Manually adding an ERP5 site

Both ways the alarm tool was NOT subscribed. JM also said his aware of this issue.

I added "subscribe" ONLY at site creation so this code should NOT alter an existing site.

Both ways the alarm tool was NOT subscribed.

So I'm very surprised. The only other thing I can think of is that recently-ish the timerserver product we use changed (as part of WSGI work).

I added "subscribe" ONLY at site creation so this code should NOT alter an existing site.

So all these if not update are always true and the whole method could be folded back into its only caller ?

I'm not asking you to do it, I just find the way this code is structured to be very misleading if it can only really be called in a single case and never after.

@vpelletier ,it's indeed worth getting rid of "if not update" imho, too. Still care must be taken to not introduce a bug unintentionally (needless to say).

No idea how it would have been enabled automatically at site creation in the past. It would be interesting to check.

About if not update, Vincent's question is somewhat rhetorical. I don't understand why the first idea would be to try getting rid of them.

I think anyway this commit enables portal_alarms too early, because ERP5Site_reindexAll will disable it temporarily. Doing it at the end of ERP5Generator.create, by activity, after appropriate tags looks a better place.

I don't understand why the first idea would be to try getting rid of them.

This is conditional to whether they are useful at all. I expect @Tyagov to have done all checks needed on what this code is doing an when it is called exactly before doing this change. Then, if all checks came to the conclusion that "register portal_alarms on site creation only" did not need to be in an if not update: block, then it means these tests are likely dead checks (always true) and should be removed to make the code easier to understand next time (fewer things to checks, fewer decisions to take).

Of course, if they are not dead checks, then why would this change be outside of one such conditional block if the intent is still to only run at site creation ?

@vpelletier , I will check.

@Tyagov in Document/BusinessTemplate.py BusinessTemplate._install():

      # update tools if necessary
      if self.getTitle() == 'erp5_core' and self.getTemplateUpdateTool():
        from Products.ERP5.ERP5Site import ERP5Generator
        gen = getattr(site, '_generator_class', ERP5Generator)()
        LOG('Business Template', 0, 'Updating Tools')
        gen.setup(site, 0, update=1)

this is how alarm is subscribed during upgrade, I guess.

And your change here calls portal_alarms.subscribe() in setup() regardless update argument, but we have the following code just 30 lines below :

    if not update:
      self.setupWorkflow(p)
      self.setupERP5Core(p,**kw)
      self.setupERP5Promise(p,**kw)

and setupERP5Promise already calls portal_alarms.subscribe() (in certain cases).

  def setupERP5Promise(self,p,**kw):
    """
    Install the ERP5 promise configurator
    """
    template_tool = p.portal_templates
    # Configure the bt5 repository
    repository = p.getPromiseParameter('portal_templates', 'repository')
    if repository is not None:
      template_tool.updateRepositoryBusinessTemplateList(repository.split())
      template_tool.installBusinessTemplateListFromRepository(
          ['erp5_promise'], activate=True, install_dependency=True)
      p.portal_alarms.subscribe()

@Kaz, thank you. I was unaware of this code call.

I think we have 3 cases, before fix (which is easy) I'd like that we all agree.

At site creation time , yes we want subscribed *And ERP5Site_reindexAll will re-subscribe ALWAYS
at erp5_core upgrade / upgrader, then we want to keep it unchanged so configuration is KEPT as it was.
When promises are involved (they also subscribe unconditionally).

Here's patch Kaz and I like and does that:

$566

I continued the search:

portal_alarm was automatically subscribed to timerserver since 2005: dc71b7bc
it was later moved to a different superclass in 2011: 7b4a3d4c (and I see nothing obviously wrong here, just noting that's why it's not on AlarmTool class anymore)

@romain Could you comment on why you added p.portal_alarms.subscribe() in ERP5Site.setupERP5Promise in e9674c9e ? Was portal_alarms already not automatically subscribed to timerserver in 2012 (so the rgression was before that), or was it an extra precaution ?

[EDIT] I should have split the quoted line in 2...

At site creation time , yes we want subscribed

I agree.

*And ERP5Site_reindexAll will re-subscribe ALWAYS

It only does so because it unsubscribes portal_alarms when it starts running, so alarms do not trigger mid-reindexation. Note that it only resubscribes alarm_tool if it was subscribed to begin with (it calls portal.portal_alarms.isSubscribed()), which looks correct to me.

I agree with points 2 and 3 .

Could you comment on why you added p.portal_alarms.subscribe() in ERP5Site.setupERP5Promise in e9674c9e?

I do not remember.

But I guess I added it because portal_alarms was not subscribed (I don't believe I added this line as an extra precaution).

I've fixed as we discussed in !1086 (merged)

So what will be done about the fact that manage_afterAdd somehow does not work properly ?

Because this is still an issue, would be a much better place to register portal_alarms to timerserver than doing it in such very different piece of code (AlarmTool registering itself rather than ERP5Site registering AlarmTool).

I do not understand why we seem to settle (now twice in a row) on incomplete investigations on the effect of this code, despite issues being raised, clearly listed.

So next question is do we want that after adding a portal_alarms tool to have itself subscribe at first place ? And I do mean "self" subscribe.

Let's agree on this first, then implement.

IMO it's ERP5Site creation code (or higher level "logic") that should subscribe both portal_alarms / portal_activities and self subscribe is to be redundant.

And I do mean "self" subscribe.

So you mean:

  def manage_afterAdd(self, item, container):
    self.subscribe()
    Folder.manage_afterAdd(self, item, container)

Source: dc71b7bc

Let's agree on this first, then implement.

Did you notice:

a commit from Jean-Paul from 2005 adding that feature
everybody here expecting that feature to be present

@vpelletier , of course we're sure we want this feature - i.e. have portal_alarms be subscribed. I really only meant if the tool should subscribe itself or if ERP5Site generation tool should subscribe it. Maybe here we do not understand each other ?

I have no strict objections in what place although as I said I prefer "ERP5Site creation code". And regarding "everybody here expecting that feature to be present" - do you mean that portal_alarms being subscribed or portal_alarms subscribing itself after adding ?

I really do not either get your point either. Feel free to go ahead and explain yourself with a patch. For me there's no point in writing more as I simple do not understand what you want / need. I'm sorry.

Sadly I cannot give even a pseudo-code patch, as I do not know why JP's code stopped working.

To me, the best final state, from a code point of view, is:

AlarmTool.manage_afterAdd must be made to work. I'm quite convinced this is the regression I suspect happened some time before Romain's 2012 change.
Romain's code subscribing portal_alarms should be removed (and it already is removed with your second patch)
your code subscribing portal_alarms should be removed, as it would then become redundant with a working AlarmToo.manage_afterAdd (this I could of course give a patch for, but it is not the most important part)

Which, from a run-time point of view, means:

portal_alarms is subscribed upon its creation by its own manage_afterAdd, which AFAIK happens only when ERP5Site is being created (along with the many other tools, erp5_core/erp5_xhtml_style/... BTs being installed, etc)
portal_alarms is then never again subscribed without a user specifically subscribing portal_alarms (currently, this means going to the ZMI and clicking the "subscribe" button)

The hard part is understanding why AlarmTool.manage_afterAdd is not doing its job: it looks well-implemented and in a method which I would expect is exactly called when we want it to be.

Is is called ?
It is somehow called too early/too late ?
Is it trying to actually subscribe the correct path (ex: temporary ID during creation) ?
Is something else unsubscribing it later (ex: when renaming to a final ID) ?

I have a gut feelings of a few possible causes (in no particular order):

This method being moved to a mixin class which contains super() calls may not be doing what we think it is doing. I have fought a lot with ERP5Folder inheritance lately, I thought I understood how super works even in complex (ERP5-scale) multi-inheritance cases, but I now know I am not able to mentally model it accurately. So... Maybe ?
ERP5Type.CopySupport (which I suspect is responsible for calling manage_afterAdd) may not be calling it properly. I know it was changed gradually (with myself doing several invasive changes there in the last years, although more recently than 2012). This could cause the examples I give in the previous list.
TimerServer class changed some time ago as part of wsgi work. This would have happened much too late for a 2012 regression, and seems unlikely to cause such issue, but it is strongly-related code so... Maybe ?

I have a low confidence in each of these gut feelings. The real issue may be something completely different, but hopefully it gives ideas of things to check.

Hopefully this clarifies my point of view on both the goal and the beggining of a path which should lead there. I promise I'm trying to be as clear as I can, and am quite distraught that I'm failing so much.

@vpelletier , thank you. I see your point (misunderstanding from both sides). I will try to find some time next week to investigate.

Here is the reason (typo in 7b4a3d4c)...

$ git show 7b4a3d4cc4a46067461d40882533787de673b16d | egrep -A1 '(^diff|def manage_afterAdd)' | grep -B3 -A1 manage_afterAdd
diff --git product/ERP5/Tool/AlarmTool.py product/ERP5/Tool/AlarmTool.py
index b0eca89d59..016ed5c25f 100644
--
-  def manage_afterAdd(self, item, container):
-    self.subscribe()
--
diff --git product/ERP5/mixin/timer_service.py product/ERP5/mixin/timer_service.py
new file mode 100644
--
+  def manage_afterAdd(self, *args, **kw):
+    self.unsubscribe() # <-- !!!!!

Haha, I completely missed that while reading it. So it was the mixin after all, but totally not for the reason I suspected.

@Kaz, thanks. I've fixed and once tests pass will push.