InventoryAPI: prevent common mistakes with inventory list brain attributes

calling brain.quantity does brain.getObject().quantity

product/ERP5/bootstrap/erp5_core/ExtensionTemplateItem/portal_components/extension.erp5.InventoryBrain.py

@@ -47,9 +47,9 @@ class ComputedAttributeGetItemCompatibleMixin(ZSQLBrain):
       return item.__of__(self)
     return item
 
-class InventoryListBrain(ComputedAttributeGetItemCompatibleMixin):
+class InventoryListBrainBase(ComputedAttributeGetItemCompatibleMixin):
   """
-    Lists each variation
+    Common interface for all inventory API brains
   """
   # Stock management
   def _callSimulationTool(self, method_id, ignore_unknown_columns=True, **kw):
@@ -284,7 +284,25 @@ class InventoryListBrain(ComputedAttributeGetItemCompatibleMixin):
                                  mapping=mapping)
     return translateString('Unknown')
 
-class TrackingListBrain(InventoryListBrain):
+class InventoryListBrain(InventoryListBrainBase):
+  """ Brains for InventoryList
+  """
+  def getQuantity(self):
+    raise ValueError("Do not use getQuantity on InventoryListBrain, use brain.total_quantity instead")
+
+  def _quantity(self):
+    raise ValueError("Do not use quantity on InventoryListBrain, use brain.total_quantity instead")
+  quantity = ComputedAttribute(_quantity, 1)
+
+  def getPrice(self):
+    raise ValueError("Do not use getPrice on InventoryListBrain, use brain.total_price instead")
+
+  def _price(self):
+    raise ValueError("Do not use price on InventoryListBrain, use brain.total_price instead")
+  price = ComputedAttribute(_price, 1)
+
+
+class TrackingListBrain(InventoryListBrainBase):
   """
   List of aggregated movements
   """
@@ -305,7 +323,7 @@ class TrackingListBrain(InventoryListBrain):
     return self.date
 
 
-class MovementHistoryListBrain(InventoryListBrain):
+class MovementHistoryListBrain(InventoryListBrainBase):
   """Brain for getMovementHistoryList
   """
 

[Jérome Perrin]

@georgios.dagkakis let's see how test break if we do this. This is not final patch (maybe we can remove the implicit getObject ?), but just to know the status of tests.

/cc @vpelletier

[Jérome Perrin]

https://nexedi.erp5.net/test_result_module/20161214-1066C2F7 ... there are failures that at a first glance seem to be real mistakes

[Georgios Dagkakis]

Here so:

https://lab.nexedi.com/nexedi/erp5/blob/master/bt5/erp5_research_item/SkinTemplateItem/portal_skins/erp5_research_item/ResearchItemModule_callResearchSummaryReport.py#L45

getInventoryList using result.quantity

[Georgios Dagkakis]

More complete list (now tests are finished):

• https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/tests/testInventory.py#L962
• https://lab.nexedi.com/nexedi/erp5/blob/master/bt5/erp5_research_item/SkinTemplateItem/portal_skins/erp5_research_item/ResearchItemModule_callResearchSummaryReport.py#L45
• https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/tests/testInventoryAPI.py#L2862
• https://lab.nexedi.com/nexedi/erp5/blob/master/bt5/erp5_payroll/SkinTemplateItem/portal_skins/erp5_payroll/AccountingTransactionModule_getPaySheetLineReportSectionLineList.py#L83

[Jérome Perrin]

Yes, they really seem to be errors, but I am sure there are more that lies in some not tested reports.

We talked about this with @vpelletier , from this dicussion I noted that:

• Implicit getObject is a wrong idea at zsqlbrain level. Developers need to understand the difference between catalog brain and ERP5 documents and the cost of getting a the document when it's not strictly needed. This is why ZSQLBrainNoObject was created.
• InventoryListBrain.getObject returning a random movement from all the aggregated movements does not seem a valid use case

[Georgios Dagkakis]

InventoryListBrain.getObject returning a random movement from all the aggregated movements does not seem a valid use case

I think of this case:

payment_request_uid_list = [x.payment_request_uid for x in getInventoryList(
  group_by_payment_request=True,
   ...
)]

isn't this a valid use?

I agree it is confusing though and maybe nother approach would be better

This is why ZSQLBrainNoObject was created

Detail, but I think this return None here is useless. Value should be None anyway, so we can return that

[Jérome Perrin]

isn't this a valid use?

If you goup_by_payment_request, ( not group_by_payment_request_uid ), then automatically Simulation Tool will expose payment_request_uid on the brains, so in this case, there is no implicit getObject.

Also if you were using getObject, you have to call getSourcePaymentRequest or getDestinationPaymentRequest depending on which side your looking at the movement from.

[Jérome Perrin]

I made another try of completly removing that implicit getObject on InventoryListBrain in a1dd0148 . Test results are here

By reading that test I guess that people believe that getQuantity on inventory brain to act like .total_quantity.

[Georgios Dagkakis]

If you goup_by_payment_request, ( not group_by_payment_request_uid ), then automatically Simulation Tool will expose payment_request_uid on the brains, so in this case, there is no implicit getObject.

Hmm, should this be added to the table here? jerome/inventory_api_doc@5d33d178

[Jérome Perrin]

Hmm, should this be added to the table here?

Yes, that's right. That table was not an exhaustive list of all possible group_by parameters. In theory all source and destination categories (section, function, project, payment_request, funding, trade, advice, administration etc ) can be passed as group_by_X and group_by_mirror_X . Currently, not everything is supported, because not everything is in stock table. Over the years, we only added the most useful ones.

[Jérome Perrin]

I will probably not work on this in the near future, so I added a reference in our bug tracker as #20161220-2BBD5A ( and git pushed that commit in keep-around/* manually)

For the records, there use to be a getQuantity method on InventoryBrain (removed in 89cfd319 ), but it was also returning something different from brain.total_quantity. This was #449.