Do not remove inactive services from profiles and templates #2637
Conversation
| continue | ||
| obj = api.get_object_by_uid(uid) | ||
| if not api.is_active(obj): | ||
| records.append(record) |
There was a problem hiding this comment.
I don't understand why this code block is necessary. Why can't we just set the records from above? IMO this makes it impossible to remove a deactivated service.
There was a problem hiding this comment.
Edit:
I understand now why this is needed and suggest the same changes as mentioned below. Also I would add the self.getServiceUIDs to avoid the record handling in the loop
| obj = api.get_object(uid) | ||
| if not active_only or api.is_active(obj): | ||
| services.append(obj) | ||
| return services |
There was a problem hiding this comment.
Can we refactor this code for better understanding as follows?
services = map(api.get_object, self.getServiceUIDs())
if active_only:
# filter out inactive services
services = filter(api.is_active, services)
return list(services)| continue | ||
| obj = api.get_object_by_uid(uid) | ||
| if not api.is_active(obj): | ||
| records.append(record) |
There was a problem hiding this comment.
I would suggest to refactor this code to allow to flush inactive services:
def setServices(self, value, keep_inactive=True):
...
uids = [record.get("uid") for record in records]
if keep_inactive:
to_check = filter(lambda uid: uid not in uids, self.getServiceUIDs())
inactive = filter(lambda uid: not api.is_active, to_check)
uids.extend(inactive)
...
| mutator = self.mutator("services") | ||
| mutator(self, records) | ||
|
|
||
| # BBB: AT schema field property | ||
| Service = Services = property(getServices, setServices) | ||
|
|
||
| @security.protected(permissions.View) | ||
| def getServiceUIDs(self): | ||
| def getServicesUIDs(self): |
There was a problem hiding this comment.
Can we stick here to the naming getServiceUIDs instead of getServicesUIDs?
Reasoning (according to ChatGPT;))
-
Clarity: The phrase "Service UIDs" is clear and suggests that it refers to the unique identifiers associated with each "Service." Using
Servicein the singular keeps the focus on the type of UIDs being retrieved rather than suggesting that there are "Services" with "UIDs." -
Consistency: This aligns with common naming conventions where the subject of the method (e.g.,
Service) is singular, while the return type (e.g.,UIDs) indicates a collection. -
Parallel to Standard Naming Conventions: Naming methods like
getUserIDs(rather thangetUsersIDs) orgetItemNames(instead ofgetItemsNames) is common and widely understood.
Summary
getServiceUIDs is concise, clear, and consistent with common naming practices, making it the better choice.
There was a problem hiding this comment.
jeje, ok.... my ChatGPT confirm yours... ;)
----8<-------------------------
The best accessor name would be getServiceUIDs.
Explanation:
-
Consistency: "ServiceUIDs" suggests you are returning multiple UIDs for the "Service" objects, and using the singular form "Service" keeps it cleaner and more intuitive.
-
Clarity: Even though "Services" is plural, using "ServiceUIDs" makes it clear that you are getting a list of UIDs related to the service objects as a whole, without the repetition of plurals.
So, getServiceUIDs is clearer and more concise.
----8<-------------------------
Oh man, this AI is teaching us already
Description of the issue/feature this PR addresses
This Pull Request achieves the functionality suggested at #2623 (comment) for both
AnalysisProfileandSampleTemplate:Current behavior before PR
Deactivated services do not come-up automatically on profiles when reactivated
Desired behavior after PR is merged
Deactivated services come-up automatically on profiles when reactivated
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.