updated the mail sending functionality for DEV env and seperated the DEV and PROD env execution#20
updated the mail sending functionality for DEV env and seperated the DEV and PROD env execution#20NeonShivam wants to merge 6 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Don't do this. We don't need to containerize our service, but if we do, use Buildpack support which is bundled in. This kind of flat Dockerfile is not appropriate for Spring Boot apps.
There was a problem hiding this comment.
ok, I will check and work on this
There was a problem hiding this comment.
We already have a compose.yaml. Any changes should go into that. I also want to understand the motivation behind this. Is this for our dev workflow (i.e. container based development)? What is this addition buying us?
There was a problem hiding this comment.
Now I have put everything inside docker-compose.yml for now
There was a problem hiding this comment.
There are too many structural changes in this file, where I'm not sure what the actual addition/removal is. Unfortunately we don't (yet) have a robust automated PR checker which would just flag actual issues so we don't need to review this file. Until then, changes need to be made judiciously or explained.
|
|
||
| @Autowired | ||
| private EmailService emailService; | ||
| private EmailService emailService; // Ensure this is autowiring the INTERFACE |
There was a problem hiding this comment.
The point of autowiring is Spring injects the implementation.
I don't see anyway you would be able to enforce this comment anyhow.
There was a problem hiding this comment.
I know you weren't working on this aspect yet, but the real code smell here is the direct call to EmailService from the controller. Sending email is an outcome of a business process, this call should be made from inside one of the other services.
There was a problem hiding this comment.
I have now used the method saveVisitor to send mail which is inside VisitService
|
|
||
| void sendMeetingNotification(String email, String whomToMeet); | ||
|
|
||
| // Optional: for QA to inspect sent emails (only relevant for the mock) |
There was a problem hiding this comment.
You've just hit on my exact issue with "design for testability" approach. It messes up the interfaces. I don't have a cleaner approach in mind for this right now, we can keep this for now.
There was a problem hiding this comment.
My problem with this class is it's violating SOLID. There is only 1 responsibility - the mechanic of sending emails. Everything else needs to be pulled out to other classes/interfaces. It doesn't matter whether it's an OTP or an alert or a report, for this interface it's just an email send operation. The clients have to send the right payloads.
There was a problem hiding this comment.
I have now seperated the interfaces and created specific to functionality like
EmailService,
OtpNotificationService,
VisitorNotificationService,
MeetingNotificationService,
VisitorReportService
| properties: | ||
| hibernate: | ||
| format_sql: true | ||
| datasource: |
There was a problem hiding this comment.
This shouldn't be necessary. The prescribed mechanism for dev DB connections is to use Spring Boot Service Connections (is that the correct name?) which are auto injected. If you want to use a local DB, that's fine, but don't commit it, specially credentials.
There was a problem hiding this comment.
Don't comment this. It took me a week to bring this back after the last dev commented it out. It works fine, you need to tune your setup so it works for you.
There was a problem hiding this comment.
Don't comment this. It took me a week to bring this back after the last dev commented it out. It works fine, you need to tune your setup so it works for you.
There was a problem hiding this comment.
Don't commit commented out tests. Test should run and pass before code is committed.
2 participants
in this PR changes made are -