-
Notifications
You must be signed in to change notification settings - Fork 56
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
Monthly Resource Digest #594
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Marcy, great job on this! I think the approach looks really good and my only change requests are syntax related. This will be a nice addition 🙂
|
||
auth()->user()->save(); | ||
if ($user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the user.preferences.update
route be moved up into the auth
middleware group? If so that would avoid the need for this if condition.
'is_subscriber' => 'nullable|boolean', | ||
]); | ||
|
||
$user->is_subscriber = $request->has('digest-subscriber'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think track_id
and digest-subscriber
can be added to the validate
check before updating.
|
||
$user->is_subscriber = false; | ||
$user->unsubscribe_token = null; | ||
$user->save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do this in a single $user->update()
call.
app/Mail/ResourceDigestEmail.php
Outdated
$this->resources = $resources; | ||
$this->user = $user; | ||
$this->unsubscribeUrl = $unsubscribeUrl; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be rewritten like the following and then the properties on lines 16-18 can be removed.
} | |
public function __construct( | |
public $resources, | |
public $user, | |
public $unsubscribeUrl | |
) {} |
Co-authored-by: Andrew Morgan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
$user->is_subscriber = false; | ||
$user->unsubscribe_token = null; | ||
$user->save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$user->is_subscriber = false; | |
$user->unsubscribe_token = null; | |
$user->save(); | |
$user->update([ | |
'is_subscriber' => false, | |
'unsubscribe_token' => null, | |
]); |
When I can, I prefer using update because it feels more concise (despite being one more line) and tidy.
public function up(): void | ||
{ | ||
Schema::table('users', function (Blueprint $table) { | ||
$table->boolean('is_subscriber')->default(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to add ->after()
and specify a column that I want the new column to appear after so the timestamps (created_at, etc.) are always last.
This option is only available in MySQL and MariaDB, so we couldn't use it for a Postgres project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 👏
This PR gives users the ability to opt-in to a monthly digest that lists resources added within the last 30 days. If no resources have been added, an email is not sent.
I'll be adding a New Resource page, accessible from the auth menu, in a follow-up PR.