You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "rshdhere (via GitHub)" <gi...@apache.org> on 2026/10/13 07:46:39 UTC
[PR] FINERACT-2536: Publish webhook events for document commands [fineract]
rshdhere opened a new pull request, #6633:
URL: https://github.com/apache/fineract/pull/6633
Since FINERACT-2246 moved document management to the new command processing, creating, updating or deleting a document no longer notifies the webhooks subscribed to DOCUMENT events. Only the legacy SynchronousCommandProcessingService publishes HookEvents; the new CommandDispatcher runs its after-hooks, but none of them published one.
Add a HookableCommand interface through which a command declares the entity and action its webhooks subscribe to, and an after-hook that publishes the HookEvent for such commands with the same payload fields as the legacy path. As in the legacy path, publication waits for the enclosing transaction to commit, and a failure to publish is logged without failing the command. The document commands declare DOCUMENT CREATE, UPDATE and DELETE; other migrated commands can opt in the same way.
## Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [X] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [X] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [X] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [X] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [X] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [X] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [X] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [X] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6179139014
@vidakovic could you please give us feedback about this implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on code in PR #6633:
URL: https://github.com/apache/fineract/pull/6633#discussion_r4259836037
##########
fineract-command/src/main/java/org/apache/fineract/command/core/HookableCommand.java:
##########
@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.command.core;
+
+/**
+ * Implemented by commands whose successful execution must notify the webhooks registered for its event, as the legacy
+ * command processing does for every command. The names are the entity and action a hook subscribes to, e.g.
+ * {@code DOCUMENT} and {@code CREATE}.
+ */
+public interface HookableCommand {
+
+ String hookEntityName();
Review Comment:
@rshdhere why this change is required for this entity hook?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6455238828
Thanks @adamsaghy, you're right on both counts.
1. I checked `2ff0c62^`: `DocumentManagementApiResource` called `DocumentWritePlatformService` directly and never went through `commandsSourceWritePlatformService`, so DOCUMENT hooks never fired. I've reworded the commit message and the description to present this as a missing feature, not a regression fix.
2. I went with the first option. `WebhookCommandHookAfter` now copies `officeId`/`clientId` to the top level of the payload when the response has them, and sets `officeId` to null inside `response`, as `SynchronousCommandProcessingService` does. That way `TwilioHookProcessor` and `MessageGatewayHookProcessor` will work for migrated client commands that opt in. I added a test for this. I also updated the description to point out the remaining difference: `request` is now the serialized request DTO rather than the raw JSON body.
The commit was GPG-signed, but my public key wasn't on my GitHub account, so it didn't show as verified. I've added the key and force-pushed the re-signed commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6239785283
@vidakovic PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub) via issues" <is...@fineract.apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6532311801
Thanks @galovics, good catch. You're right: every command that opts in through `HookableCommand` would lose the `status: Exception` notifications the legacy path sends today.
The dispatcher already calls `CommandHookError` hooks in its catch block, so this should only need a `WebhookCommandHookError` next to the after-hook. It would:
- turn the throwable into an `ErrorInfo` with `ErrorHandler`, the way `SynchronousCommandProcessingService` does, so the `response` (`errorCode`, `statusCode`, the parsed error message) matches the legacy one
- publish straight away rather than after commit, since a failed command doesn't commit, which is also what `publishHookErrorEvent` does today
- share the payload building with the after-hook so the two can't drift
Since this one is already approved and green, I'd rather keep it scoped and do the error hook as a follow-up. I'll open a JIRA ticket and link it here. If you'd prefer it in this PR, I can add it here instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6454700676
Thanks for the PR. The DOCUMENT CREATE/UPDATE/DELETE hooks now fire, which is what FINERACT-2536 asks for. Two things in the description don't match the code:
**1. The root cause.** The description says DOCUMENT webhooks stopped firing when FINERACT-2246 moved documents to the new command processing. They never fired. Before 2ff0c62f40 (FINERACT-2246), `DocumentManagementApiResource` called `documentWritePlatformService.createDocument/updateDocument/deleteDocument` directly. It never went through `commandsSourceWritePlatformService`, and that has been true since the resource was added in 2012, so `SynchronousCommandProcessingService.publishHookEvent` was never reached for documents. Please fix the description and commit message. As written, this reads like a regression fix, and it is really a missing feature.
**2. "Same payload fields as the legacy path".** It isn't the same. The legacy `publishHookEvent` also puts top-level `officeId` and `clientId` into the payload, taken from the `CommandProcessingResult`, and nulls `officeId` inside `response` (`SynchronousCommandProcessingService` L417-424). `WebhookCommandHookAfter` only sends entityName, actionName, createdBy*, request, response and timestamp.
This matters beyond documents, because `HookableCommand` is meant to be the opt-in for every migrated command. `TwilioHookProcessor` (L107) and `MessageGatewayHookProcessor` (L97) only work when `clientId` is at the top level of the payload. Any migrated command that opts in, a client command for example, will publish the event, but SMS/Twilio hooks will quietly do nothing.
Please either:
- add `officeId`/`clientId` when the response provides them, or
- drop the "same payload fields" claim and state the difference (the payload shape, and that `request` is now the serialized request DTO rather than the raw JSON body).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6604612601
@rshdhere Thanks for the rebase, the PR looks good to me. A follow-up on the error hook from @galovics's question: I couldn't find the JIRA ticket you mentioned. Could you open it and link it here? Until `WebhookCommandHookError` exists, any command that opts in through `HookableCommand` won't send the `status: Exception` notifications the legacy path sends today, so I'd like it tracked before more commands are migrated to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy merged PR #6633:
URL: https://github.com/apache/fineract/pull/6633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub) via issues" <is...@fineract.apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6563736332
following up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6531556443
CI is all green, and is ready to be merged
lmk if any more changes are needed for this one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on code in PR #6633:
URL: https://github.com/apache/fineract/pull/6633#discussion_r4259938114
##########
fineract-command/src/main/java/org/apache/fineract/command/core/HookableCommand.java:
##########
@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.command.core;
+
+/**
+ * Implemented by commands whose successful execution must notify the webhooks registered for its event, as the legacy
+ * command processing does for every command. The names are the entity and action a hook subscribes to, e.g.
+ * {@code DOCUMENT} and {@code CREATE}.
+ */
+public interface HookableCommand {
+
+ String hookEntityName();
Review Comment:
Thanks for the review! The new pipeline doesn't know which webhook event a command corresponds to, and this interface is how a command tells it.
Webhooks are matched by **entity + action**: `FineractHookListener` calls `retrieveHooksByEvent(entityName, actionName)`, so a hook subscribed to `DOCUMENT` / `CREATE` only fires for a `HookEvent` carrying exactly those names. In the legacy path those names come from the `CommandWrapper` (e.g. `.createDocument()` sets `DOCUMENT` / `CREATE`), and `SynchronousCommandProcessingService.publishHookEvent()` uses them.
Since FINERACT-2246, documents go through the new `CommandDispatcher` instead. The new `Command<T>` carries only metadata (id, idempotency key, usernames, timestamps) and the payload; there is no entity or action name anywhere. So even an after-hook that wants to publish a `HookEvent` has nothing to match subscriptions against, which is why DOCUMENT webhooks stopped firing.
`HookableCommand` fills that gap:
- a command declares its webhook identity: `DocumentCreateCommand` returns `DOCUMENT` / `CREATE`, and so on;
- `WebhookCommandHookAfter` publishes a `HookEvent` only for commands implementing it, so it's opt-in: commands that aren't webhook-relevant are untouched, and nothing changes for the legacy path (no double events; CLIENT CREATE still fires once);
- the declaration lives next to the command, so migrating another resource to the new pipeline is just a matter of implementing the two methods.
Alternatives I considered: deriving the names from the class name (`DocumentCreateCommand` → `DOCUMENT` / `CREATE`) breaks silently on renames and doesn't fit names like `UPDATE_RESCHEDULE`; a central command → (entity, action) mapping works but has to be kept in sync with every new command. If you'd prefer either of those, or a different approach altogether, I'm happy to rework it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6512026874
Rebased on develop and dropped the `LoanApplicationTermsTest` commit.
Thanks for the review!
could you approve the workflow runs when you get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub)" <gi...@apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6484933485
@adamsaghy PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2536: Publish webhook events for document commands [fineract]
Posted by "rshdhere (via GitHub) via issues" <is...@fineract.apache.org>.
rshdhere commented on PR #6633:
URL: https://github.com/apache/fineract/pull/6633#issuecomment-6613502411
@adamsaghy Thanks for the merge! I've opened [FINERACT-3036](https://issues.apache.org/jira/browse/FINERACT-3036) for the error hook. It adds a `WebhookCommandHookError` so that commands opting in through `HookableCommand` send the same `status: Exception` notifications as the legacy path. I'll pick it up next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org