Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PHEE-296A] Email Api Tc #293

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[PHEE-296A] Email Api Tc #293

wants to merge 8 commits into from

Conversation

ankita10r
Copy link
Contributor

@ankita10r ankita10r commented Jun 10, 2024 •
edited
Loading

Description

The Tc design is done based on greenmail mock

`Use placeholder/pod ip of int test repo in the host field of the smtp mail config : this to be done on the chart
This will make the tc pass in ephemeral only
Once it passes and evidence attached the @gov tag can be changed to @govtodo for ci runs 
Also one failure tc to be written that gives failed to send mail in callback - this will be enabled in ci

Reason for this:

1. Gmail smtp setup requires a password to be set in gmail app
2. And mail sent by gmail cant be verifed via greenmail mock in int test
3. So greenmail mock setup is used to send mail and check server for all sent mails
4. But it faced an issue in host name(which was localhost) that works locally but fails in ci
5. Now there is no way to access integration test service using service name or ingress that’s why host name can only be in pod ip level (that only works in ephemeral mode)`
  • PR title should have jira ticket enclosed in [].

    Format: [jira_ticket] description

    ex: [phee-123] PR title.
  • Add a link to the Jira ticket.
  • Describe the changes made and why they were made.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Followed the PR title naming convention mentioned above.

  • Acknowledge that we will not merge PRs that are not passing the checks ("green") - it is your (author's) responsibility to get a proposed PR to pass all the checks, not primarily the project's maintainers.

  • The PR title should include a JIRA ticket

  • Design-related bullet points or design document links related to this PR added in the description above.

  • Updated corresponding Postman Collection or API documentation for the changes in this PR.

  • Create/update unit or integration tests for verifying the changes made.

  • Add required Swagger annotation and update API documentation with details of any API changes if applicable

  • Followed the naming conventions as given in https://docs.google.com/document/d/1Q4vaMSzrTxxh9TS0RILuNkSkYCxotuYk1Xe0CMIkkCU/edit?usp=sharing

  • Followed coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

FYI our guidelines for code reviews same as https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

@jit-ci Jit CI
Copy link
Contributor

jit-ci bot commented Jun 10, 2024

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@ankita10r ankita10r mentioned this pull request Jun 18, 2024
Open
6 tasks
fynmanoj
fynmanoj requested changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@fynmanoj fynmanoj left a comment

Choose a reason for hiding this comment

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

None of the test cases are verifying the callback.

src/test/java/org/mifos/integrationtest/cucumber/stepdef/EmailStepDef.java

@Value("${messageGateway.endpoint.email}")
private String mgMail;
@Value("${spring.mail.host}")
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to use 'spring' here

.github/workflows/jit-security.yml
@@ -256,7 +256,7 @@ jobs:
uses: jitsecurity-controls/[email protected]
with:
security_control: registry.jit.io/control-semgrep-alpine:latest

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like there are lots of formatting changes, you can remove this from this PR

.jit/jit-integration.yml
@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

is this empty file?

src/main/resources/application.yaml
endpoint:
email: "/emails"

spring:
Copy link
Contributor

Choose a reason for hiding this comment

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

you can ignore this level: spring. we don't need unless we cant avoid it

src/main/resources/application.yaml
smtp:
auth: false
starttls:
enable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

google smtp requies TLS. can we test this with TLS for more compatibility?

src/test/java/resources/email.feature
Scenario: Sending an email to the recipient with failure in callback
Given I can inject MockServer
And I can start mock server
And I can register the stub with "/sendMail" endpoint for "POST" request with status of 200
Copy link
Contributor

Choose a reason for hiding this comment

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

All tests with same callback URL may cause issues like it use to cause before. use unique strings and add a check based on URL

src/test/java/resources/email.feature
And I have tenant as "paymentBB2"
And I generate clientCorrelationId
When I send an email to the following recipients with subject "Test Email" and body "This is a test email" with callbackurl as "/sendMail" and get 202
| [email protected] |
Copy link
Contributor

Choose a reason for hiding this comment

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

move subject and body to this datatable

src/test/java/org/mifos/integrationtest/cucumber/stepdef/EmailStepDef.java
rootNode = request.getRequest().getBodyAsString();
logger.info("Rootnode value:" + rootNode);
assertThat(rootNode
.contains("Email could not be sent to [[email protected]] because of Mail server connection failed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

already mentioned in other PR, The error request body is not proper.
the message should be in sentence case

src/test/java/org/mifos/integrationtest/cucumber/stepdef/EmailStepDef.java
if (!(request.getRequest().getBodyAsString()).isEmpty() && request.getRequest().getUrl().equals("/sendMail")) {
String rootNode = null;
rootNode = request.getRequest().getBodyAsString();
logger.info("Rootnode value:" + rootNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

donot use + for concatenating log statements

src/test/java/resources/email.feature
And the email service is running
And I have tenant as "paymentBB2"
And I generate clientCorrelationId
When I send an email to the following recipients with subject "Test Email" and body "This is a test email" with callbackurl as "/sendMail" and get 202
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the test cases are verifying the callback.

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

@fynmanoj fynmanoj fynmanoj requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankita10r @fynmanoj