Messaging system #158
Messaging system #158
Conversation
|
@ChayanDass Please follow good commits and PR practices:
|
pkg/email/email_service.go
Outdated
| return service | ||
| } | ||
|
|
||
| func (s *AsyncEmailService) Send(to, subject, html string) { |
There was a problem hiding this comment.
Just a small refactor. Let's write interfaces and their methods together.
pkg/email/email_service.go
Outdated
|
|
||
| service := &AsyncEmailService{ | ||
| emailQueue: make(chan EmailData, emailQueueSize), | ||
| licenseQueue: make(chan LicenseJob, licenseQueueSize), |
There was a problem hiding this comment.
I think it would be better to take a more generic approach by using only the emailQueue and not having separate licenseQueue and bulkQueue as they ultimately send the jobs to emailQueue after formatting data into email format.
Right now, the flow is:
LicenseJob ---> licenseQueue ---> formatted EmailData ---> emailQueue
BulkJob ---> bulkQueue ---> formatted EmailData ---> emailQueue
There was a problem hiding this comment.
I’ve implemented a wrapper function to simplify the process.
Now, the email service works as a standalone service, the controller only calls the wrapper to create the email and then pushes it to the email queue.
Using the wrapper keeps the controller clean and avoids unnecessary logic there.
now in future , if need more email types , then creating a new templete would be enough , no need to chnge on the email service ..
pkg/email/email_service.go
Outdated
| zap.Int("success", job.Success), | ||
| zap.Int("failed", job.Failed), | ||
| ) | ||
| s.bulkQueue <- job |
There was a problem hiding this comment.
Call Send() directly from where we currently enqueue licenseQueue and bulkQueue.
There was a problem hiding this comment.
yes , i tried this , but it increses the api response time or any error may lead to panic situation
There was a problem hiding this comment.
Ok, got it! Thanks for clarifying!
|
|
||
| } | ||
|
|
||
| func LogError(msg string, fields ...zap.Field) { |
There was a problem hiding this comment.
Any reason for having this function wrapper?
logger.LogError and logger.Error doesn't have much difference?
There was a problem hiding this comment.
no need of wrapper, but this wrapper is useful if in future any need of change the structure of logs
pkg/api/licenses.go
Outdated
|
|
||
| if email.IsEmailServiceRunning() { | ||
|
|
||
| go email.Email.QueueBulkInsertEmail(email.BulkInsertJob{ |
There was a problem hiding this comment.
It would be better to send mail to all admins on any update/create/import
There was a problem hiding this comment.
Should I keep all admins as cc / bcc? Or send separate emails
There was a problem hiding this comment.
I think sending separate emails will be better as we can personalize content(if needed, in the future) or log per-recipient success/failure.
ChayanDass
force-pushed
the
mail2
branch
4 times, most recently
from
a3bab29 to
3015683
Compare
|
i have added all the suggestions , and for email to admins, i have modify the wrapper function to add a another email to queue and added all the admins to "to", all the admins will get a personlized single email , @deo002 , could you check the latest changes..... Todo :
|
Signed-off-by: Chayan Das <[email protected]>
| username = s.from | ||
| } | ||
|
|
||
| d := gomail.NewDialer(s.host, s.port, username, s.pass) |
There was a problem hiding this comment.
Wouldn't it be better to use a global instance of this variable which is initialised once and then used again and again?
| logger *zap.Logger | ||
| wg sync.WaitGroup | ||
| mu sync.Mutex | ||
| running bool |
There was a problem hiding this comment.
From what I understand, running and closed variables are used to prevent double starts and prevent calling queue if service is not initialized.
We wont ever face the problem of double starts as Start is only called once in the Init function which is called once in main.
While calling Queue as well, we check whether the service is initialized via Email == nil.
Thus, running and closed are redundant and can be removed.
| retryDelay time.Duration | ||
|
|
||
| // IsRunning cache | ||
| lastCheckMu sync.Mutex |
There was a problem hiding this comment.
I think processWithRetries should suffice and isRunning function is not needed. What do you suggest?
isRunning will save network calls to the smtp server in case it is unreachable(which it will rarely be) and will add one network to all other cases
| } | ||
| } | ||
|
|
||
| func (s *AsyncEmailService) Queue(ctx context.Context, email EmailData) error { |
| return | ||
| } | ||
| go func() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) |
There was a problem hiding this comment.
Can we declare this context in the Queue function itself?
| flag.Parse() | ||
|
|
||
| // Start the email service | ||
| EnableSMTP, _ := strconv.ParseBool(os.Getenv("ENABLE_SMTP")) |
There was a problem hiding this comment.
rename variable to enableSMTP
| if err := db.DB.Where(&models.User{UserLevel: &level}).First(&user).Error; err != nil { | ||
| log.Fatalf("Failed to find a super admin") | ||
| } | ||
| var total, success, failed int |
There was a problem hiding this comment.
variables declared but not initialized
3 participants
Bulk Import
license update
functions cover
note : currently, only the license changes are included