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.

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

[Feature] Ad hoc email command #12325

Merged
merged 9 commits into from
Jan 7, 2025
Merged

[Feature] Ad hoc email command #12325

merged 9 commits into from
Jan 7, 2025

Conversation

petertgiles
Copy link
Contributor

@petertgiles petertgiles commented Dec 17, 2024

🤖 Resolves #11542

👋 Introduction

Adds an Artisan command and Laravel notification for sending ad hoc emails with just the first name and last name.

🕵️ Details

The command will select users to notify by one of three criteria:

  • specific email addresses
  • specific notification families
  • all users

🧪 Testing

  1. Update your api/.env file with the "team" api key
  2. Rerun ./artisan optimize:clear
  3. Start a queue worker
  4. Update one user to have your GCNotify registered email address
  5. Test various forms of the Artisan command
  • Can use email criteria
    ./artisan send-notifications:ad-hoc-email cdaa82f1-7f76-4d9e-a82e-328c41a6ab62 0831e036-8dfb-4f5e-8b31-867b2c526d77 [email protected] [email protected]
  • Can use notification family criteria
    ./artisan send-notifications:ad-hoc-email cdaa82f1-7f76-4d9e-a82e-328c41a6ab62 0831e036-8dfb-4f5e-8b31-867b2c526d77 --notificationFamily=JOB_ALERT --notificationFamily=APPLICATION_UPDATE
  • Can use notify all criteria (OR logic)
    ./artisan send-notifications:ad-hoc-email cdaa82f1-7f76-4d9e-a82e-328c41a6ab62 0831e036-8dfb-4f5e-8b31-867b2c526d77 --notifyAllUsers
  • Respects language preferences
  • Can not use 0 or 2 or 3 criteria types
  • Warns of missing email addresses
  • Errors on invalid notification families
  • first and last name passed to GC Notify

📸 Screenshot

image

@brindasasi brindasasi self-requested a review December 18, 2024 15:58
Copy link
Contributor

@brindasasi brindasasi left a comment

Choose a reason for hiding this comment

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

Code looks good so far. Will verify in dev

return $builder;
}

private function builderFromNotifictionFamilies(array $notificationFamilies): Builder
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typo - builderFromNotificationFamilies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

($notifyAllUsers ? 1 : 0);

if ($optionTypesCount != 1) {
throw new \Error('Must filter users using exactly one of the option types');
Copy link
Contributor

Choose a reason for hiding this comment

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

This can simply use InvalidArgumentException .. but your choice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right - that's more descriptive.
switch to InvalidArgumentException

$userCount = $users->count();

if ($this->confirm('Do you wish to send notifications to '.$userCount.' users?')) {
$progressBar = $this->output->createProgressBar($userCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

oo.. progressbar!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying out some of the new Laravel 11 features. 😉

@brindasasi
Copy link
Contributor

Sorry for delaying on this what I noticed so far

  • If I gave wrong template ID but valid email addresses , it gives me success message even though email doesn't get sent out
  • separtating the email id params indivdiaully like. this are not very practical when we have to send bulk emails.
    --emailAddress=[email protected] --emailAddress=[email protected]
    It would be better to have an array with comma separated

testing few more things

Copy link
Contributor

@brindasasi brindasasi left a comment

Choose a reason for hiding this comment

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

As mentioned, when the template is mistakenly given it is failing silently
image

when there is multiple critieras given , I got different exception rather than the invalidargument .. but I'm not too worried about this .. as long as it breaks we know
But if the above one is fixed , would be great.
image

Also if we can have comma separated emails for bulk email would be great.
Other than that this looks awesome!

@petertgiles
Copy link
Contributor Author

Thanks for the review! Some feedback on your requests:

If I gave wrong template ID but valid email addresses , it gives me success message even though email doesn't get sent out

This is because sending emails is an asynchronous process. They aren't sent out when the command is run so we can't immediately display API errors like invalid template ID. That will be logged in log files when the queue worker tries to send it. I don't think we want to make this synchronous either. We need to be able to slow it down to satisfy the API rate limiter and be able to retry later if the service goes offline. This is easy to forget so updated the messages to try to be a bit clearer:
use word queue in messages

separtating the email id params indivdiaully like. this are not very practical when we have to send bulk emails.
--emailAddress=[email protected] --emailAddress=[email protected]
It would be better to have an array with comma separated

The AC of the issue didn't specify how the options should be sent so I went with the built-in Laravel option: When defining an option that expects multiple input values, each option value passed to the command should be prefixed with the option name. If we want to manually parse a comma separated list we can do that but let's decide on a method and add it to the AC. @tristan-orourke , preferences?

when there is multiple critieras given , I got different exception rather than the invalidargument

In your screenshot it looks like your log file is not writable which is an unrelated error. I think we will get the right exception for multiple criteria.
image

@tristan-orourke
Copy link
Member

Ah, I didn't realize Laravel had a built-in option like that for list inputs. Hmmm. When we need to use this command, we're likely to be starting from a comma-separated list of emails... However, it would only take a bit of find/replace in a text editor to modify that. Let's stick to the Laravel spec, and keep it as is.

@petertgiles petertgiles requested a review from brindasasi January 6, 2025 19:59
@brindasasi
Copy link
Contributor

Thanks for the review! Some feedback on your requests:

If I gave wrong template ID but valid email addresses , it gives me success message even though email doesn't get sent out

This is because sending emails is an asynchronous process. They aren't sent out when the command is run so we can't immediately display API errors like invalid template ID. That will be logged in log files when the queue worker tries to send it. I don't think we want to make this synchronous either. We need to be able to slow it down to satisfy the API rate limiter and be able to retry later if the service goes offline. This is easy to forget so updated the messages to try to be a bit clearer: use word queue in messages

separtating the email id params indivdiaully like. this are not very practical when we have to send bulk emails.
--emailAddress=[email protected] --emailAddress=[email protected]
It would be better to have an array with comma separated

The AC of the issue didn't specify how the options should be sent so I went with the built-in Laravel option: When defining an option that expects multiple input values, each option value passed to the command should be prefixed with the option name. If we want to manually parse a comma separated list we can do that but let's decide on a method and add it to the AC. @tristan-orourke , preferences?

when there is multiple critieras given , I got different exception rather than the invalidargument

In your screenshot it looks like your log file is not writable which is an unrelated error. I think we will get the right exception for multiple criteria. image

This error was on dev. I can try again. Could be related to the issue you solved yesterday

@brindasasi
Copy link
Contributor

Thanks for the review! Some feedback on your requests:

If I gave wrong template ID but valid email addresses , it gives me success message even though email doesn't get sent out

This is because sending emails is an asynchronous process. They aren't sent out when the command is run so we can't immediately display API errors like invalid template ID. That will be logged in log files when the queue worker tries to send it. I don't think we want to make this synchronous either. We need to be able to slow it down to satisfy the API rate limiter and be able to retry later if the service goes offline. This is easy to forget so updated the messages to try to be a bit clearer: use word queue in messages

separtating the email id params indivdiaully like. this are not very practical when we have to send bulk emails.
--emailAddress=[email protected] --emailAddress=[email protected]
It would be better to have an array with comma separated

The AC of the issue didn't specify how the options should be sent so I went with the built-in Laravel option: When defining an option that expects multiple input values, each option value passed to the command should be prefixed with the option name. If we want to manually parse a comma separated list we can do that but let's decide on a method and add it to the AC. @tristan-orourke , preferences?

when there is multiple critieras given , I got different exception rather than the invalidargument

In your screenshot it looks like your log file is not writable which is an unrelated error. I think we will get the right exception for multiple criteria. image

This error was on dev. I can try again. Could be related to the issue you solved yesterday

image
It works now in dev!

Copy link
Contributor

@brindasasi brindasasi left a comment

Choose a reason for hiding this comment

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

LGTM!

@petertgiles petertgiles enabled auto-merge January 7, 2025 16:31
@petertgiles petertgiles added this pull request to the merge queue Jan 7, 2025
Merged via the queue into main with commit ebbc179 Jan 7, 2025
11 checks passed
@petertgiles petertgiles deleted the 11542-email-command branch January 7, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Improved laravel Command for sending arbitrary email notifications
3 participants