add a new ID generator that uses node-id defined in zope.conf

product/ERP5Type/Core/Folder.py

@@ -214,6 +214,16 @@ class FolderMixIn(ExtensionClass.Base):
     self._setLastId(my_id) # Make sure no reindexing happens
     return my_id
 
+  def _generatePerNodeId(self):
+    """
+    Generate id base on node-id defined in zope.conf,
+    useful for import and mass creation
+    of objects inside a module using activities
+    We also append random id
+    """
+    activity_tool = self.getPortalObject().portal_activities
+    return "%s-%s" %(activity_tool.getCurrentNode(), self._generateRandomId())
+
   def _generatePerNodeNumberId(self):
     """
     Generate id base on node number, useful for import and mass creation

[Julien Muchembled]

@vpelletier We need getCurrentNode() in another place where the activity tool is not easy to get whereas getCurrentNode() doesn't use self. And anyway, for performance reasons, we'd like to avoid fetching the activity tool for nothing. The idea is to move getCurrentNode() and getServerAddress out of ActivityTool, and even move it to a more generic place like ERP5 or ERP5Type. A suggestion is ERP5Type.Utils.

/cc @rporchetto @seb

[Vincent Pelletier]

@jm Ok for moving could-be-functions out of class.

About the location of resulting method, as node_id is part of CMFActivity product configuration, it would feel more natural to me to put that method in CMFActivity module.

/cc @rporchetto @seb

[Sebastien Robin]

The possibility to identify a node is useful outside CMFActivity. I prefer that we move it to another more generic place, even though for now implementation depends on CMFActivity product configuration.

Ideally, we should even update zope.conf configuration, but there is no benefit of doing it right now.

So I would vote for a generic place, with a comment explaining that for historical reasons we still use cmfactivity configuration in zope.conf.

Does this sounds acceptable ?

[Vincent Pelletier]

I do not want to introduce such inconsistency when it's so easy to just import from CMFActivity the needed function.

Ideally, we should even update zope.conf configuration, but there is no benefit of doing it right now.

Importing it easy.

Changing how configuration is generated forbids updating ERP5 products without redeploying instances with the updated SR.

So I would vote for a generic place

Please define "generic", and explain what is wrong with importing such function from CMFActivity.

[Vincent Pelletier]

And I forgot the mandatory cc to work around gitlab brain-dead non-notifications: @rporchetto @seb @jm .

[Sebastien Robin]

Vincent, it's mostly a mater of taste. Importing from CMFActivity or another place, it works in both cases from a technical point of view. But I think the identification of a node should have nothing to do with CMFActivity, since it can have various usages. So my taste is to have it outside CMFActivity, so as part of ERP5Type or ERP5.

Zope.conf can be changed neither, or in 10 years, or when it will be replaced by something else. There is possibility to use compatibility code if needed to not prevent update of products.

So I prefer a place outside CMFActivity to show the direction (feature independent of CMFActivity), and you prefer to avoid inconsistency.

But honestly, I don't care so much. I would like to see progress on this. So we can follow your preference. May be @tatuya has suggestions, since I think he should take care about interfaces ?

/cc @rporchetto @jm @vpelletier @tatuya

[Tatuya Kamada]

I have the same opinion with seb. I mean:

• node-id configuration should be inside ERP5 Product or ERP5Type Product
• progress has priority because this is a small topic
• backward-compatibility counts
• I do not care so much

I explain why I think it should be inside ERP5 or ERP5Type.

and explain what is wrong with importing such function from CMFActivity.

<product-config CMFActivity>
  node-id web-user-0
</product-config>

Having a node-id-in-CMFActivity for web nodes is not correct from its role. If it is an activity nodes, of course node-id-in-CMFActivity is right and correct, but all zope nodes has node-id, for better or for worse.

If my understanding of node-id is wrong, please let me know.

[Vincent Pelletier]

Another solution which is fine for me is:

• write a new product, which can be easily reused
• make it pull its node id from its own configuration section
• make ERP5 SR put the same value in CMFActivity's (for backward compatibility, at least) and in the new configuration section

node-id configuration should be inside ERP5 Product or ERP5Type Product

Then it is impossible for CMFActivity to access it. CMFAcitivty is not supposed to import from ERP5* products. Maybe it is fine to have it implemented elsewhere in addition to having it in CMFActivity (to avoid having to import it from ERP5* products), and we will forever have both node id notions. But I am not seduced by this idea.

Having a node-id-in-CMFActivity for web nodes is not correct from its role.

This is a circular definition: an activity node is any node whose identifier is in the list of activity nodes. So it is by design that all nodes have a CMFActivity identifier, because that is how we can make them activity node or non-activity nodes.

[EDIT] too many typos and missed logical links

[Vincent Pelletier]

Proposed solution would eventually lead to the suppression of CMFActivity's node id notion, finalising the move. But this will have to wait for people to catch up with the new SR version, which can take years.

[Tatuya Kamada]

@vincent Thank you for your explanation.

This is a circular definition: an activity node is any node whose identifier is in the list of activity nodes. So it is by design that all nodes have a CMFActivity identifier, because that is how we can make them activity node or non-activity nodes.

If all nodes should have a CMFActivity identifier by design. I can not justify the reason to move getCurrentNode() from CMFActivity to somewhere else.

[Julien Muchembled]

write a new product, which can be easily reused

Arg...

Then it is impossible for CMFActivity to access it.

There are already so many imports from ERP5Type, and we're not going to remove them. We are diverging so much from Zope/CMF that keeping ERP5 optional in products like CMFActivity/CMFCategory/ZSQLCatalog does not make sense.

[Vincent Pelletier]

Arg...

Yeah, I don't like it very much either.

There are already so many imports from ERP5Type

I know. Nevertheless, import dependencies still exist and are still directional. So why should we make our future life harder by increasing the amount of spaghetty imports ?

I have still not seen any concrete example of why one would consider it "not reusable" in its current location. Can you provide one ?

[Sebastien Robin]

Ok. New product would indeed avoid circular dependencies. But then this will make other discussions, and so on...

So @rporchetto , please make getCurrentNode a method that we can directly import from CMFActivity.

/cc @vpelletier @tatuya @jm

[Roque]

Dear all,

the change was made. Could you please review it? https://lab.nexedi.com/nexedi/erp5/commit/8620d7733aae2baf0884c3fd7e6b8ecc1f542241

If it's okay, I'll merge it to master.

Regards

/cc @vpelletier @tatuya @jm @seb

[Tatuya Kamada]

Hello Roque!

The code looks almost OK for me except the security configurations.

security.declareProtected(CMFCorePermissions.ManagePortal, 'getServerAddress')

Do we want the methods as protected? not public?

Also I have two small comments.

• Having test cases for the two methods is better.
• The comment on getServerAddress() explains nothing practical. I think we should write the definition in this occasion.

[Julien Muchembled]

Does a ClassSecurityInfo object at module-level do anything ?

[Roque]

ClassSecurityInfo is obsolete there, I'll remove it.

[Roque]

Dear all,

thank you for the review, I committed the changes that correspond to your comments. Here the merge request: !647 (merged)

Regards, Roque

/cc @vpelletier @tatuya @jm @seb