Skip to content

Adding service component calls to methods in RecordCommunitiesService #2002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

monotasker
Copy link

@monotasker monotasker commented Apr 3, 2025

❤️ Thank you for your contribution!

Description

The unique methods in RecordCommunitiesService lack the call to service component methods that are usual in record service operations. This PR

  • adds those calls in the add, remove, set_default, and bulk_add methods
  • and adds a components property in the service config that reads components from the RDM_RECORD_COMMUNITIES_SERVICE_COMPONENTS variable if present (defaults to [])

The placement of the component calls in each method is intended to allow maximum customization of the service call's input data before the data is actually applied to change the records.

Note that in bulk_add I've wrapped the set_default argument value in a dictionary (making it mutable) so that it can be updated inside the components.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

@monotasker monotasker force-pushed the add_component_hooks branch from ededef9 to c6133b8 Compare April 7, 2025 13:12
Copy link
Contributor

github-actions bot commented Jun 7, 2025

This PR was automatically marked as stale.

@github-actions github-actions bot added the stale label Jun 7, 2025
@tmorrell tmorrell removed the stale label Jun 9, 2025
@tmorrell tmorrell added this to v13.0.0 Jun 9, 2025
@tmorrell tmorrell moved this to Triage in v13.0.0 Jun 9, 2025
@@ -149,6 +149,11 @@ def add(self, identity, id_, data, uow):
record = self.record_cls.pid.resolve(id_)
self.require_permission(identity, "add_community", record=record)

# Run components
for component in self.components:
if hasattr(component, "add"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would align it with the permission so that is more readable in the instance's custom component you implement

Suggested change
if hasattr(component, "add"):
if hasattr(component, "add_community"):

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussion: Also, should we add this hook before the community.add(...) call or after @slint ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzacharo I'm happy to commit that suggested term change. My main concern with the placement is just making sure that the component method has access to the incoming data and the record object before that record has been changed. That way the data can be modified if desired before being applied.


# Run components
for component in self.components:
if hasattr(component, "remove"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this to _remove after the permission checks and rename to remove_community. Also, the question on where to add the component hook is valid here too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's agreement on moving this to _remove, I'm happy to make that change.

Here again, my interest when it comes to placement is just making sure that we can modify the data (and see the record state) before the data has been applied.

@monotasker
Copy link
Author

I'm just working on moving the component call locations and updating the tests. I'll squash the commits together and should have it ready for another review later today

…rvice

* Adds service component calls in the add, remove, set_default, and
  bulk_add methods. The component calls expect component method
  names matching the applicable permissions. The `remove` component call
  is actually made in the `_remove` method so that it can follow
  the permission check there for each individual community.

* The placement of the component calls in each method is intended to allow
  maximum flixibility in modifying the service call's input data before
  performing the action on the records.

* Adds a `components` property in the service config that reads components
  from the RDM_RECORD_COMMUNITIES_SERVICE_COMPONENTS variable if present
  (defaults to [])

* Note that in bulk_add I've wrapped the set_default argument value in a
  dictionary (making it mutable) so that it can be updated inside the
  components.

Adding errors to arguments passed into RecordCommunitiesService components for remove method

fix: Updating service component method names in RecordCommunitiesService and tests

temp: for rebasing
@monotasker monotasker force-pushed the add_component_hooks branch from aa2396f to 8a17f4c Compare June 26, 2025 13:53
@monotasker
Copy link
Author

Alright, I've updated the PR and squashed the commits together (with a proper commit message). Sorry for the messy commits earlier, and thanks for your patience.

I renamed the methods as suggested and moved the component calls for remove into _remove. I have kept each of the component calls before the data in each case is actually applied to the record. (E.g., before record.parent.communities.add is called in the add method.) This is to allow modification of the data before it is applied to the record. I don't see any disadvantages of this placement, and it has the added benefit of providing the component method with the initial state of the record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

3 participants