AshharAhmadKhan opened a new pull request, #6330: URL: https://github.com/apache/fineract/pull/6330
Fixes: https://issues.apache.org/jira/browse/FINERACT-2785 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. Related: #6325 (adds the missing UPDATE command handler for /v1/email, this bug was found while writing integration tests for that PR) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
