[
https://issues.apache.org/jira/browse/FINERACT-2785?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ashhar Ahmad Khan resolved FINERACT-2785.
-----------------------------------------
Resolution: Fixed
> EmailDataValidator validates against ScheduledEmailConstants instead of
> EmailApiConstants
> -----------------------------------------------------------------------------------------
>
> Key: FINERACT-2785
> URL: https://issues.apache.org/jira/browse/FINERACT-2785
> Project: Apache Fineract
> Issue Type: Bug
> Reporter: Ashhar Ahmad Khan
> Priority: Minor
> Fix For: 1.16.0
>
>
> *What is broken*
> EmailDataValidator is the only validator wired into /v1/email's CREATE and
> UPDATE endpoints (confirmed via EmailWritePlatformServiceJpaRepositoryImpl,
> which calls validator.validateCreateRequest(command) /
> validator.validateUpdateRequest(command)). It currently checks incoming JSON
> parameters against ScheduledEmailConstants.CREATE_REQUEST_PARAMETERS /
> UPDATE_REQUEST_PARAMETERS, constants belonging to a completely different
> feature, the scheduled/report mailing job system (whose actual validator is
> ReportMailingJobValidator).
> Concretely, EmailMessageAssembler.assembleFromJson() reads groupId, clientId,
> staffId, emailMessage from the request, the real fields for /v1/email,
> defined in EmailApiConstants. But EmailDataValidator rejects
> groupId/clientId/staffId outright with UnsupportedParameterException, because
> they don't exist in ScheduledEmailConstants's allowed parameter set. It also
> demands fields that don't apply to this resource at all, like
> stretchyReportId, startDateTime, name, emailRecipients,
> emailAttachmentFileFormatId.
> Net effect, it is currently impossible to create or update a valid email via
> /v1/email's public API using the fields it's actually documented to accept.
> *Why it's broken*
> Full repo grep confirms EmailDataValidator's own copies of
> isValidEmail/validateEmailRecipients/validateStretchyReportParamMap have zero
> callers anywhere, so they're dead code. Strong evidence this class was copy
> pasted from the scheduled mailing job validator and never adapted for
> /v1/email.
> *Changes made*
> * EmailDataValidator.validateCreateRequest() and validateUpdateRequest() now
> check checkForUnsupportedParameters(...) against
> EmailApiConstants.CREATE_REQUEST_DATA_PARAMETERS /
> EmailApiConstants.UPDATE_REQUEST_DATA_PARAMETERS instead of the
> ScheduledEmailConstants sets.
> * Removed all field level validation belonging to the scheduled mailing
> job's fields, since none of it applies to /v1/email, and didn't add any new
> field level rules in its place (see note below). Both methods now just do a
> blank JSON check and an unsupported parameter check against the correct
> constants.
> * Removed dead imports (ScheduledEmailConstants,
> ScheduledEmailAttachmentFileFormat, LocalDateTime, DateTimeFormatter,
> ApiParameterError, DataValidatorBuilder, PlatformApiDataValidationException,
> ArrayList, List, JsonElement). Left
> isValidEmail/validateEmailRecipients/validateStretchyReportParamMap
> untouched, including the imports they still need.
> * Removed the now dead private method
> throwExceptionIfValidationWarningsExist(...), since nothing calls it anymore.
> * Added EmailDataValidatorTest.java (6 tests) proving the fix. Covers valid
> /v1/email fields no longer throwing on create, unsupported parameters still
> getting rejected on both create and update, and blank JSON still throwing
> InvalidJsonException on both.
> *Note:* While working on this I found other three things that I'm still
> investigating. They will be fixed separately. These are as follows, and they
> need more proper investigation before they can be done:
> * EmailApiConstants.CREATE_REQUEST_DATA_PARAMETERS is missing emailSubject,
> even though EmailMessageAssembler reads it via
> EmailApiConstants.subjectParamName. Need to check the Swagger docs and git
> blame before deciding if this is even a bug.
> * EmailApiConstants.UPDATE_REQUEST_DATA_PARAMETERS only allows emailMessage,
> so groupId/clientId/staffId work on create but get rejected on update. This
> is existing behavior, not something this fix introduced, just flagging it
> since my new tests surfaced it.
> * EmailMessageAssembler.assembleFromJson() calls
> EmailMessage.pendingEmail(group, client, staff, null, ...), passing
> emailCampaign as null. pendingEmail() unconditionally calls
> .setStatusType(emailCampaign.getStatus()), which will NPE. So /v1/email
> CREATE is probably broken today for a completely separate reason. Haven't
> runtime confirmed this yet, will file its own ticket once I do.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
