Skip to content

FINERACT-2782: Add UpdateEmailCommandHandler for /v1/email UPDATE endpoint - #6325

Open
AshharAhmadKhan wants to merge 1 commit into
apache:developfrom
AshharAhmadKhan:FINERACT-2782-email-update-handler
Open

FINERACT-2782: Add UpdateEmailCommandHandler for /v1/email UPDATE endpoint#6325
AshharAhmadKhan wants to merge 1 commit into
apache:developfrom
AshharAhmadKhan:FINERACT-2782-email-update-handler

Conversation

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor

Description

EmailApiResource exposes CREATE, RETRIEVE, UPDATE, DELETE endpoints for
/v1/email. CREATE and DELETE have working @CommandType handlers; UPDATE
does not, so calling the UPDATE endpoint throws UnsupportedCommandException
despite the endpoint existing and being documented.

Everything downstream of the handler already exists and is correct:

  • EmailWritePlatformService.update(Long, JsonCommand) — interface method
    already declared
  • EmailWritePlatformServiceJpaRepositoryImpl.update(...) — already
    implemented
  • EmailApiResource.java — already calls
    CommandWrapperBuilder().updateEmail(resourceId)
  • UPDATE_EMAIL permission — already exists in
    0002_initial_data.xml, same pattern as CREATE_EMAIL/DELETE_EMAIL

The only missing piece was the command handler itself, routing
@CommandType(entity="EMAIL", action="UPDATE") to the existing service
method. This PR adds that handler, matching the structural pattern of
CreateEmailCommandHandler and the sibling UpdateEmailCampaignCommandHandler.

Note: no test coverage exists for the /emails resource at any layer, and
a separate, pre-existing validator bug was found while investigating this
(EmailDataValidator validates against the wrong resource's constants,
ScheduledEmailConstants instead of EmailApiConstants). Both are tracked
and will be addressed in a separate ticket/PR, since they are independent
of this handler-wiring fix.

See https://issues.apache.org/jira/browse/FINERACT-2782

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

hey @adamsaghy , flaky failure please retrigger when you can.

@AshharAhmadKhan
AshharAhmadKhan force-pushed the FINERACT-2782-email-update-handler branch from 2602721 to 8f46c81 Compare August 25, 2026 09:57
@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

hey @adamsaghy , please trigger checks when you can, thanks!

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

hey @adamsaghy , failure is unrelated to my code. Please retrigger that check, thanks!

@AshharAhmadKhan
AshharAhmadKhan force-pushed the FINERACT-2782-email-update-handler branch from 54fa654 to badb657 Compare August 25, 2026 15:23
@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

hey @adamsaghy , this is ready for review. Please let me know if you would like any changes.

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

not sure why my branch is out of date...

@adamsaghy

Copy link
Copy Markdown
Contributor

not sure why my branch is out of date...

You can ignore this....

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now we have a working API, lets write an integration or E2E test for it ;)

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

i see haha, i was confused whats happening and was about to rebase again.

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

Now we have a working API, lets write an integration or E2E test for it ;)

hey adam, regarding the integration/E2E test, I think we should get #6330 merged first. It fixes the EmailDataValidator using the wrong ScheduledEmailConstants, which currently causes valid /v1/email CREATE requests using the actual email API parameters to be rejected. you merged #6331 which fixes the CREATE NPE, so #6330 is the remaining prerequisite for getting a clean end-to-end CREATE → UPDATE flow. Once #6330 is merged, I can add the integration/E2E test on top of this PR.

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

Now we have a working API, lets write an integration or E2E test for it ;)

Hey Adam, I don't want to add tests to this PR. I'd rather create a separate PR for that.

I confirmed there's no integration or E2E test coverage for /v1/email at any layer right now, so I'll write a single integration test covering the full lifecycle, CREATE, RETRIEVE, UPDATE, DELETE, against /v1/email.

All four operations share the same setup, so testing them together in one test validates the real end to end flow rather than isolated fragments.

I'll do this once #6330 is merged, since CREATE needs the validator fix in that PR to accept the real request params. What do you think?

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

Now we have a working API, lets write an integration or E2E test for it ;)

Hey Adam, I don't want to add tests to this PR. I'd rather create a separate PR for that.

I confirmed there's no integration or E2E test coverage for /v1/email at any layer right now, so I'll write a single integration test covering the full lifecycle, CREATE, RETRIEVE, UPDATE, DELETE, against /v1/email.

All four operations share the same setup, so testing them together in one test validates the real end to end flow rather than isolated fragments.

I'll do this once #6330 is merged, since CREATE needs the validator fix in that PR to accept the real request params. What do you think?

hey @adamsaghy , please tell me your preferred direction, thanks!.

@adamsaghy

Copy link
Copy Markdown
Contributor

@AshharAhmadKhan #6330 is merged

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

@AshharAhmadKhan #6330 is merged

Hey Adam, just wanted to confirm the testing approach before I proceed.

Would you prefer me to add the CREATE → RETRIEVE → UPDATE → DELETE integration/E2E test directly to this PR, or keep it as a separate PR?

I was leaning toward a separate PR to keep this one scoped to the handler fix, but happy to add it here if that's your preference.

@adamsaghy

Copy link
Copy Markdown
Contributor

@AshharAhmadKhan #6330 is merged

Hey Adam, just wanted to confirm the testing approach before I proceed.

Would you prefer me to add the CREATE → RETRIEVE → UPDATE → DELETE integration/E2E test directly to this PR, or keep it as a separate PR?

I was leaning toward a separate PR to keep this one scoped to the handler fix, but happy to add it here if that's your preference.

Both works to me. I let you decide

@AshharAhmadKhan

Copy link
Copy Markdown
Contributor Author

@AshharAhmadKhan #6330 is merged

Hey Adam, just wanted to confirm the testing approach before I proceed.
Would you prefer me to add the CREATE → RETRIEVE → UPDATE → DELETE integration/E2E test directly to this PR, or keep it as a separate PR?
I was leaning toward a separate PR to keep this one scoped to the handler fix, but happy to add it here if that's your preference.

Both works to me. I let you decide

Hey Adam, I’ll keep the integration test as a separate PR. I’m almost done with it, and I think keeping it separate will prevent scope creep.

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.

2 participants