Conversation
Introduce typed properties (tries, timeout, maxExceptions) and backoff Use MailerLite facade and handle authentication/create exceptions Split handle into create/update flows with group handling and logging Add helper static constructors and improve error handling and retries
Integrate MailerLiteService to send job notification campaigns and attempt Laravel notification fallback when MailerLite is disabled or fails. Add constructor flags (useMailerLite, fallbackToNotification) and factory helpers, improve logging, query/filtering logic, and minor formatting.
Summary of ChangesHello @theihasan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates MailerLite into the application to modernize and enhance the job notification system and overall subscriber management. By leveraging MailerLite, the application gains more sophisticated capabilities for delivering personalized job alerts and managing user subscriptions, improving the reliability and flexibility of email communications. The changes involve introducing a new service layer for MailerLite interactions, refactoring existing job processes to utilize this new integration, and providing comprehensive configuration options. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces MailerLite integration for sending job notifications. It adds a new dependency, configuration, a service for MailerLite, and updates the job notification logic.
The implementation has some critical issues that need to be addressed. Specifically, a new MailerLite campaign is created for every single user, which is not scalable and will likely cause issues with API limits. The fallback mechanism to the default Laravel notifications is also commented out, which could lead to users not receiving notifications if MailerLite fails.
I've also pointed out some areas for improvement regarding code structure and best practices, such as using dependency injection instead of service location and moving raw HTML into Blade views for better maintainability.
Please review the comments for detailed feedback.
| // $notificationSent = true; | ||
| // } | ||
|
|
||
| if (! $notificationSent) { |
There was a problem hiding this comment.
The fallback logic to use Laravel's native notification system is currently commented out (lines 141-155). This means if the MailerLite service fails, no notification will be sent to the user, and this block will just log an error. This could be a critical issue if you intend to have a fallback. The $fallbackToNotification property in the constructor is also rendered useless. Please consider uncommenting the fallback block to ensure notification delivery.
| public function sendJobNotificationCampaign(User $user, Collection $jobs): ?array | ||
| { | ||
| try { | ||
| // Create campaign content | ||
| $subject = $this->generateSubject($jobs->count()); | ||
| $htmlContent = $this->generateHtmlContent($user, $jobs); | ||
| $plainContent = $this->generatePlainContent($user, $jobs); | ||
|
|
||
| // Get or create user in MailerLite | ||
| $this->ensureSubscriberExists($user); | ||
|
|
||
| // Create and send campaign | ||
| $campaign = MailerLite::campaigns() | ||
| ->subject($subject) | ||
| ->name("Job Digest for {$user->name} - ".now()->format('Y-m-d H:i')) | ||
| ->from($this->defaultFromName, $this->defaultFromEmail) | ||
| ->html($htmlContent) | ||
| ->plain($plainContent) | ||
| ->forFreePlan() // Ensure compatibility with all MailerLite plans | ||
| ->create(); | ||
|
|
||
| if ($campaign) { | ||
| // Send immediately to the specific user | ||
| // Note: In a real implementation, you'd want to use segments or groups | ||
| // For now, we'll log the campaign creation | ||
| Log::info('MailerLite campaign created successfully', [ | ||
| 'user_id' => $user->id, | ||
| 'campaign_id' => $campaign['id'] ?? null, | ||
| 'campaign_name' => $campaign['name'] ?? null, | ||
| 'job_count' => $jobs->count(), | ||
| ]); | ||
|
|
||
| return $campaign; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
This implementation creates a new MailerLite campaign for each individual user. This is highly inefficient and not the intended use of campaigns in most email marketing services. It will quickly lead to a very large number of campaigns in your MailerLite account, potentially hitting API rate limits or service plan limits. The comment on line 45 also indicates this is not the final implementation.
A better approach would be to use MailerLite's transactional emails if available, which are designed for one-to-one communications like this.
If you must use campaigns, you should consider creating a single campaign for a batch of users and using personalization tags. Creating a campaign per user is not scalable.
| $mailerLiteService = app(MailerLiteService::class); | ||
| $campaign = $mailerLiteService->sendJobNotificationCampaign($user, $matchingJobs); |
There was a problem hiding this comment.
Resolving the MailerLiteService from the container inside a loop (each) is inefficient as it happens for every user. It's better to use dependency injection on the handle method to get an instance of the service once.
You can change the handle method signature to public function handle(MailerLiteService $mailerLiteService): void and then pass the $mailerLiteService variable down through the closures. This avoids repeated service location and improves performance. Since I cannot comment on the handle method directly, please apply this change manually.
$campaign = $mailerLiteService->sendJobNotificationCampaign($user, $matchingJobs);| // Add source tracking | ||
| $builder = $builder->withField('source', 'geezap_newsletter_signup') | ||
| ->withField('signup_date', now()->toDateString()) | ||
| ->withField('signup_ip', request()->ip() ?? 'unknown'); |
There was a problem hiding this comment.
Using request()->ip() inside a queued job is unreliable because the job might be processed in a context without an active HTTP request (e.g., from a CLI worker). This could result in an incorrect or missing IP address.
It's safer to pass the IP address to the job's constructor when it's dispatched from a controller. You can then store it as a property and use it here.
Example change in constructor:
public function __construct(
public string $email,
// ... other properties
public ?string $ipAddress = null
) {}And in this method:
->withField('signup_ip', $this->ipAddress ?? 'unknown'); ->withField('signup_ip', $this->ipAddress ?? 'unknown');| private function generateHtmlContent(User $user, Collection $jobs): string | ||
| { | ||
| $jobsHtml = $jobs->map(function ($job) { | ||
| $location = $job->is_remote ? 'Remote' : ($job->city ?? 'Location not specified'); | ||
| $salary = $this->formatSalary($job); | ||
| $url = route('job.show', $job->slug); | ||
|
|
||
| return " | ||
| <div style='border: 1px solid #e5e7eb; border-radius: 8px; padding: 16px; margin-bottom: 16px; background: #ffffff;'> | ||
| <h3 style='margin: 0 0 8px 0; color: #1f2937;'> | ||
| <a href='{$url}' style='color: #3b82f6; text-decoration: none;'>{$job->job_title}</a> | ||
| </h3> | ||
| <p style='margin: 4px 0; color: #6b7280;'><strong>{$job->employer_name}</strong></p> | ||
| <p style='margin: 4px 0; color: #6b7280;'>📍 {$location}</p> | ||
| ".($salary ? "<p style='margin: 4px 0; color: #059669;'>💰 {$salary}</p>" : '')." | ||
| <p style='margin: 8px 0 0 0;'> | ||
| <a href='{$url}' style='background: #3b82f6; color: white; padding: 8px 16px; text-decoration: none; border-radius: 4px; display: inline-block;'>View Job</a> | ||
| </p> | ||
| </div> | ||
| "; | ||
| })->join(''); | ||
|
|
||
| return " | ||
| <div style='font-family: Arial, sans-serif; max-width: 600px; margin: 0 auto; padding: 20px;'> | ||
| <h1 style='color: #1f2937; border-bottom: 2px solid #3b82f6; padding-bottom: 16px;'> | ||
| Hello {$user->name}! 👋 | ||
| </h1> | ||
|
|
||
| <p style='color: #374151; font-size: 16px; line-height: 1.6;'> | ||
| We found <strong>{$jobs->count()}</strong> new job".($jobs->count() > 1 ? 's' : '')." that match your preferences: | ||
| </p> | ||
|
|
||
| {$jobsHtml} | ||
|
|
||
| <div style='border-top: 1px solid #e5e7eb; padding-top: 16px; margin-top: 24px; text-align: center; color: #6b7280;'> | ||
| <p>Want to update your job preferences? <a href='".route('preferences')."' style='color: #3b82f6;'>Update Preferences</a></p> | ||
| <p style='font-size: 14px;'>You're receiving this because you've enabled job notifications in your Geezap account.</p> | ||
| </div> | ||
| </div> | ||
| "; | ||
| } | ||
|
|
||
| /** | ||
| * Generate plain text content for the email | ||
| */ | ||
| private function generatePlainContent(User $user, Collection $jobs): string | ||
| { | ||
| $jobsList = $jobs->map(function ($job) { | ||
| $location = $job->is_remote ? 'Remote' : ($job->city ?? 'Location not specified'); | ||
| $salary = $this->formatSalary($job); | ||
| $url = route('job.show', $job->slug); | ||
|
|
||
| return " | ||
| {$job->job_title} | ||
| Company: {$job->employer_name} | ||
| Location: {$location}".($salary ? "\nSalary: {$salary}" : '')." | ||
| View Job: {$url} | ||
|
|
||
| --- | ||
| "; | ||
| })->join(''); | ||
|
|
||
| return " | ||
| Hello {$user->name}! | ||
|
|
||
| We found {$jobs->count()} new job".($jobs->count() > 1 ? 's' : '')." that match your preferences: | ||
|
|
||
| {$jobsList} | ||
|
|
||
| Want to update your job preferences? Visit: ".route('preferences')." | ||
|
|
||
| You're receiving this because you've enabled job notifications in your Geezap account. | ||
|
|
||
| Best regards, | ||
| The Geezap Team | ||
| "; | ||
| } |
There was a problem hiding this comment.
Generating raw HTML and plain text content within a service class makes it hard to read and maintain. It's better to move this content into dedicated Blade view files.
For example:
- HTML:
resources/views/emails/mailerlite/job-notification.blade.php - Plain text:
resources/views/emails/mailerlite/job-notification.txt.blade.php
You can then render them using view(...)->render(). This separates presentation from logic and makes the templates much easier to manage.
Remove the unnecessary find() + conditional branch and call named(...)->subscribe() directly. Add a Sentry logger import (unused).
1 participant
No description provided.