Do not remove inactive services from profiles and templates (#2637)

* Keep inactive services in profiles

* Keep inactive services in templates

* Skip retrieval of service uids from profiles on create_analysis_request

* Changelog

* Fix doctest

* Refactor the code from getServices()

* Add keep_inactive parameter to setServices

* getServiceUIDs --> getServicesUIDs

* Revert "getServiceUIDs --> getServicesUIDs"

This reverts commit 07e1181.

* getAnalysisServiceUIDs --> getServiceUIDs

* Ensure consistency between getServices and getServiceUIDs

* Complete doctests

---------

Co-authored-by: Ramon Bartl <rb@ridingbytes.com>

CHANGES.rst

@@ -4,6 +4,7 @@ Changelog
 2.6.0 (unreleased)
 ------------------
 
+- #2637 Do not remove inactive services from profiles and templates
 - #2642 Fix Attribute Error in Upgrade Step 2619
 - #2641 Fix AttributeError on rejection of samples without a contact set
 - #2640 Fix missing custom transitions via adapter in Worksheet's analyses

src/bika/lims/content/analysisservice.py

@@ -558,25 +558,6 @@ def _default_calculation_vocabulary(self):
         dlist.add("", _("None"))
         return dlist
 
-    def after_deactivate_transition_event(self):
-        """Method triggered after a 'deactivate' transition
-
-        Removes this service from all assigned Profiles and Templates.
-        """
-        catalog = api.get_tool(SETUP_CATALOG)
-
-        # Remove the service from profiles to which is assigned
-        profiles = catalog(portal_type="AnalysisProfile")
-        for profile in profiles:
-            profile = api.get_object(profile)
-            profile.remove_service(self)
-
-        # Remove the service from templates to which is assigned
-        templates = catalog(portal_type="SampleTemplate")
-        for template in templates:
-            template = api.get_object(template)
-            template.remove_service(self)
-
     # XXX DECIDE IF NEEDED
     # --------------------
 

src/bika/lims/utils/analysisrequest.py

@@ -41,7 +41,6 @@
 from bika.lims.utils import tmpID
 from bika.lims.workflow import doActionFor
 from DateTime import DateTime
-from Products.Archetypes.config import UID_CATALOG
 from Products.Archetypes.event import ObjectInitializedEvent
 from Products.CMFPlone.utils import _createObjectByType
 from senaite.core.catalog import SETUP_CATALOG
@@ -267,14 +266,6 @@ def to_list(value):
     # Convert them to a list of service uids
     uids = filter(None, map(to_service_uid, uids))
 
-    # Extend with service uids from profiles
-    profiles = to_list(values.get("Profiles"))
-    if profiles:
-        uid_catalog = api.get_tool(UID_CATALOG)
-        for brain in uid_catalog(UID=profiles):
-            profile = api.get_object(brain)
-            uids.extend(profile.getServiceUIDs() or [])
-
     # Get the service uids without duplicates, but preserving the order
     return list(OrderedDict.fromkeys(uids).keys())
 

src/senaite/core/content/analysisprofile.py

@@ -252,20 +252,22 @@ def getRawServices(self):
         return accessor(self) or []
 
     @security.protected(permissions.View)
-    def getServices(self):
+    def getServices(self, active_only=True):
         """Returns a list of service objects
 
         >>> self.getServices()
         [<AnalysisService at ...>,  <AnalysisService at ...>, ...]
 
         :returns: List of analysis service objects
         """
-        records = self.getRawServices()
-        service_uids = map(lambda r: r.get("uid"), records)
-        return list(map(api.get_object, service_uids))
+        services = map(api.get_object, self.getRawServiceUIDs())
+        if active_only:
+            # filter out inactive services
+            services = filter(api.is_active, services)
+        return list(services)
 
     @security.protected(permissions.ModifyPortalContent)
-    def setServices(self, value):
+    def setServices(self, value, keep_inactive=True):
         """Set services for the profile
 
         This method accepts either a list of analysis service objects, a list
@@ -295,15 +297,41 @@ def setServices(self, value):
                 raise TypeError(
                     "Expected object, uid or record, got %r" % type(v))
             records.append({"uid": uid, "hidden": hidden})
+
+        if keep_inactive:
+            # keep inactive services so they come up again when reactivated
+            uids = [record.get("uid") for record in records]
+            for record in self.getRawServices():
+                uid = record.get("uid")
+                if uid in uids:
+                    continue
+                obj = api.get_object(uid)
+                if not api.is_active(obj):
+                    records.append(record)
+
         mutator = self.mutator("services")
         mutator(self, records)
 
     # BBB: AT schema field property
     Service = Services = property(getServices, setServices)
 
     @security.protected(permissions.View)
-    def getServiceUIDs(self):
-        """Returns a list of the selected service UIDs
+    def getServiceUIDs(self, active_only=True):
+        """Returns a list of UIDs for the referenced AnalysisService objects
+
+        :param active_only: If True, only UIDs of active services are returned
+        :returns: A list of unique identifiers (UIDs)
+        """
+        if active_only:
+            services = self.getServices(active_only=active_only)
+            return list(map(api.get_uid, services))
+        return self.getRawServiceUIDs()
+
+    @security.protected(permissions.View)
+    def getRawServiceUIDs(self):
+        """Returns the list of UIDs stored as raw data in the 'Services' field
+
+        :returns: A list of UIDs extracted from the raw 'Services' data.
         """
         services = self.getRawServices()
         return list(map(lambda record: record.get("uid"), services))

src/senaite/core/content/sampletemplate.py

@@ -453,20 +453,22 @@ def getRawServices(self):
         return accessor(self) or []
 
     @security.protected(permissions.View)
-    def getServices(self):
+    def getServices(self, active_only=True):
         """Returns a list of service objects
 
         >>> self.getServices()
         [<AnalysisService at ...>,  <AnalysisService at ...>, ...]
 
         :returns: List of analysis service objects
         """
-        records = self.getRawServices()
-        service_uids = map(lambda r: r.get("uid"), records)
-        return list(map(api.get_object, service_uids))
+        services = map(api.get_object, self.getRawServiceUIDs())
+        if active_only:
+            # filter out inactive services
+            services = filter(api.is_active, services)
+        return list(services)
 
     @security.protected(permissions.ModifyPortalContent)
-    def setServices(self, value):
+    def setServices(self, value, keep_inactive=True):
         """Set services for the template
 
         This method accepts either a list of analysis service objects, a list
@@ -508,12 +510,44 @@ def setServices(self, value):
                 "part_id": part_id,
             })
 
+        if keep_inactive:
+            # keep inactive services so they come up again when reactivated
+            uids = [record.get("uid") for record in records]
+            for record in self.getRawServices():
+                uid = record.get("uid")
+                if uid in uids:
+                    continue
+                obj = api.get_object(uid)
+                if not api.is_active(obj):
+                    records.append(record)
+
         mutator = self.mutator("services")
         mutator(self, records)
 
     # BBB: AT schema field property
     Services = property(getServices, setServices)
 
+    @security.protected(permissions.View)
+    def getServiceUIDs(self, active_only=True):
+        """Returns a list of UIDs for the referenced AnalysisService objects
+
+        :param active_only: If True, only UIDs of active services are returned
+        :returns: A list of unique identifiers (UIDs)
+        """
+        if active_only:
+            services = self.getServices(active_only=active_only)
+            return list(map(api.get_uid, services))
+        return self.getRawServiceUIDs()
+
+    @security.protected(permissions.View)
+    def getRawServiceUIDs(self):
+        """Returns the list of UIDs stored as raw data in the 'Services' field
+
+        :returns: A list of UIDs extracted from the raw 'Services' data.
+        """
+        services = self.getRawServices()
+        return list(map(lambda record: record.get("uid"), services))
+
     @deprecate("deprecated since SENAITE 2.6: Use getRawServices() instead")
     @security.protected(permissions.View)
     def getAnalysisServicesSettings(self):
@@ -602,12 +636,12 @@ def getAnalysisServicePartitionID(self, service):
             return ""
         return record.get("part_id", "")
 
+    @deprecate("deprecated since SENAITE 2.6: Use getServiceUIDs() instead")
     @security.protected(permissions.View)
     def getAnalysisServiceUIDs(self):
         """Returns a list of all assigned service UIDs
         """
-        services = self.getRawServices()
-        return list(map(lambda record: record.get("uid"), services))
+        return self.getServiceUIDs()
 
     @security.protected(permissions.View)
     def get_services_by_uid(self):

src/senaite/core/tests/doctests/AnalysisProfile.rst

@@ -196,3 +196,59 @@ Setting the profile works up to this state:
     >>> services = get_services(ar3)
     >>> set(map(api.get_uid, services)).issuperset(service_uids1 + [Au.UID()])
     True
+
+
+Inactive services
+.................
+
+Inactive services are not returned by default:
+
+    >>> Fe in profile1.getServices()
+    True
+
+    >>> success = do_action_for(Fe, "deactivate")
+    >>> api.get_workflow_status_of(Fe)
+    'inactive'
+
+    >>> Fe in profile1.getServices()
+    False
+
+But kept as raw data, just in case we re-activate the service later:
+
+    >>> Fe in profile1.getServices(active_only=False)
+    True
+
+    >>> api.get_uid(Fe) in profile1.getRawServiceUIDs()
+    True
+
+By default, inactive services are kept as raw data when the value is set:
+
+    >>> profile1.setServices([])
+    >>> Cu in profile1.getServices()
+    False
+    >>> Fe in profile1.getServices()
+    False
+    >>> api.get_uid(Cu) in profile1.getServiceUIDs()
+    False
+    >>> api.get_uid(Fe) in profile1.getServiceUIDs()
+    False
+    >>> api.get_uid(Cu) in profile1.getRawServiceUIDs()
+    False
+    >>> api.get_uid(Fe) in profile1.getRawServiceUIDs()
+    True
+
+Unless we use `keep_inactive=False`:
+
+    >>> profile1.setServices([], keep_inactive=False)
+    >>> Cu in profile1.getServices()
+    False
+    >>> Fe in profile1.getServices()
+    False
+    >>> api.get_uid(Cu) in profile1.getServiceUIDs()
+    False
+    >>> api.get_uid(Fe) in profile1.getServiceUIDs()
+    False
+    >>> api.get_uid(Cu) in profile1.getRawServiceUIDs()
+    False
+    >>> api.get_uid(Fe) in profile1.getRawServiceUIDs()
+    False

src/senaite/core/tests/doctests/AnalysisServiceInactivation.rst

@@ -142,8 +142,8 @@ But if we activate `Ca` again:
     True
 
 
-Deactivated service is removed automatically from profiles
-..........................................................
+Deactivated service is kept in profiles, but not returned
+.........................................................
 
 Create a profile:
 
@@ -162,14 +162,77 @@ If we deactivate `Au`:
 
 Profile does no longer contain this service:
 
-    >>> [service.getKeyword() for service in profile.getServices()]
+    >>> services = profile.getServices()
+    >>> [service.getKeyword() for service in services]
     ['Ca', 'Mg']
 
+Unless we explicitly ask to include inactive ones:
+
+    >>> services = profile.getServices(active_only=False)
+    >>> [service.getKeyword() for service in services]
+    ['Ca', 'Mg', 'Au']
+
+So the reference to inactive services is kept as a raw value:
+
     >>> len(profile.getRawServices())
-    2
+    3
 
-Re-activate `Au`:
+And if we re-activate `Au`:
 
     >>> performed = doActionFor(Au, 'activate')
     >>> api.is_active(Au)
     True
+
+The profile returns the service again:
+
+    >>> services = profile.getServices()
+    >>> [service.getKeyword() for service in services]
+    ['Ca', 'Mg', 'Au']
+
+
+Deactivated service is kept in templates, but not returned
+..........................................................
+
+Create a sample template:
+
+    >>> template = api.create(setup.sampletemplates, "SampleTemplate")
+    >>> template.setServices([Ca, Mg, Au])
+    >>> [service.getKeyword() for service in template.getServices()]
+    ['Ca', 'Mg', 'Au']
+    >>> len(template.getRawServices())
+    3
+
+If we deactivate `Au`:
+
+    >>> performed = doActionFor(Au, 'deactivate')
+    >>> api.is_active(Au)
+    False
+
+Template does no longer contain this service:
+
+    >>> services = template.getServices()
+    >>> [service.getKeyword() for service in services]
+    ['Ca', 'Mg']
+
+Unless we explicitly ask to include inactive ones:
+
+    >>> services = template.getServices(active_only=False)
+    >>> [service.getKeyword() for service in services]
+    ['Ca', 'Mg', 'Au']
+
+So the reference to inactive services is kept as a raw value:
+
+    >>> len(profile.getRawServices())
+    3
+
+And if we re-activate `Au`:
+
+    >>> performed = doActionFor(Au, 'activate')
+    >>> api.is_active(Au)
+    True
+
+The template returns the service again:
+
+    >>> services = template.getServices()
+    >>> [service.getKeyword() for service in services]
+    ['Ca', 'Mg', 'Au']