Skip to content

SAK-52335 Search Refactor ECP events registrations and remove unused services#14363

Draft
kunaljaykam wants to merge 1 commit intosakaiproject:masterfrom
kunaljaykam:SAK-52335
Draft

SAK-52335 Search Refactor ECP events registrations and remove unused services#14363
kunaljaykam wants to merge 1 commit intosakaiproject:masterfrom
kunaljaykam:SAK-52335

Conversation

@kunaljaykam
Copy link
Member

@kunaljaykam kunaljaykam commented Feb 6, 2026

Summary by CodeRabbit

  • Refactor
    • Unified and simplified search indexing across chat, commons, conversations, messages, forums, and site content for more consistent and reliable update handling.
    • Standardized event handling so content updates are tracked and registered uniformly, reducing configuration complexity and improving indexing reliability.

@kunaljaykam kunaljaykam marked this pull request as draft February 6, 2026 06:30
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

Walkthrough

Multiple EntityContentProducer implementations now also implement EntityContentProducerEvents, replace per-event add/remove lists with centralized EVENT_ACTIONS/eventActions maps, always register with SearchIndexBuilder (remove SearchService gating), add getTriggerFunctions(), and update Spring bean wiring to use attribute refs and remove deprecated service properties.

Changes

Cohort / File(s) Summary
Chat Module
chat/chat-impl/impl/src/java/org/sakaiproject/chat2/model/impl/ChatContentProducer.java, chat/chat-impl/impl/src/webapp/WEB-INF/components.xml
Implements EntityContentProducerEvents, introduces static EVENT_ACTIONS, simplifies init() to always register with SearchIndexBuilder, adds getTriggerFunctions(), and switches bean wiring to attribute-based refs.
Commons Module
commons/impl/src/java/org/sakaiproject/commons/impl/CommonsContentProducer.java, commons/impl/src/webapp/WEB-INF/components.xml
Adds EntityContentProducerEvents, centralizes event-to-action mapping in EVENT_ACTIONS, removes SearchService dep from bean, always registers with SearchIndexBuilder, and adds getTriggerFunctions().
Conversations Module
conversations/impl/src/main/java/org/sakaiproject/conversations/impl/ConversationsEntityContentProducerImpl.java
Implements EntityContentProducerEvents, replaces event lists with EVENT_ACTIONS, simplifies getAction()/matches(), adds getTriggerFunctions(), and removes config-gated registration.
Message Search Adapters
message/search-adapters/impl/src/java/org/sakaiproject/search/component/adapter/message/MessageContentProducer.java, message/search-adapters/impl/src/webapp/WEB-INF/messageAdapterComponents.xml
Now implements EntityContentProducerEvents; initializes eventActions from addEvents/removeEvents in init() and registers unconditionally; DI changed to attribute refs; removed ServerConfigurationService/SearchService refs.
Message Forums Module
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/entity/MessageForumsEntityContentProducer.java, msgcntr/messageforums-component-impl/src/webapp/WEB-INF/components.xml
Implements EntityContentProducerEvents, generified event lists (List<String>), adds eventActions map and getTriggerFunctions(), refactors init() to populate map and register, and removes SearchService/ServerConfigurationService bean props.
Site Content Producer
search/search-impl/impl/src/java/org/sakaiproject/search/component/adapter/site/SiteContentProducer.java, search/search-impl/impl/src/webapp/WEB-INF/siteAdapterComponents.xml
Adds EntityContentProducerEvents, replaces add/remove lists with static EVENT_ACTIONS, uses it in getAction()/matches(), always registers with SearchIndexBuilder, adds getTriggerFunctions(), and removes SearchService/ServerConfigurationService bean injections.

Possibly related PRs

Suggested labels

do-not-close

Suggested reviewers

  • ern
  • ottenhoff
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically describes the main refactoring effort: consolidating EntityContentProducer event registrations into centralized maps and removing unused services like SearchService and ServerConfigurationService across multiple modules.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kunaljaykam
Copy link
Member Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

✅ Actions performed

Full review triggered.

@adrianfish
Copy link
Contributor

@kunaljaykam Is there any reason why we can't just merge EntityContentProducer and EntityContentProducerEvents? What is the logic behind having getTriggerFunctions as an optional piece? I would, personally, add a method called getTriggerEvents (I would remove the word Function as it implies the method returns some functions that can be applied elsewhere) to EntityContentProducer and just remove that extra interface from the existing content producers. You'd have to update the index builder pieces to call getTriggerEvents as well, obviously.

However, maybe there's a good reason that getTriggerFunctions is in a separate interface? Seems weird to me.

@ern
Copy link
Contributor

ern commented Feb 6, 2026

agreed functions is not a great name they're simply events that the service needs to fetch, also if the ECP registration needs to call this then it absolutely makes sense for this to not be a separate interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants