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

Monthly Resource Digest #594

Merged
merged 40 commits into from
Sep 27, 2024
Merged

Monthly Resource Digest #594

merged 40 commits into from
Sep 27, 2024

Conversation

MarcyLina
Copy link
Contributor

@MarcyLina MarcyLina commented Aug 9, 2024

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.

@MarcyLina MarcyLina marked this pull request as draft August 9, 2024 20:21
@MarcyLina MarcyLina changed the title Mla/monthly resource digest WIP Monthly Resource Digest Aug 16, 2024
@MarcyLina MarcyLina marked this pull request as ready for review August 16, 2024 13:33
@MarcyLina MarcyLina self-assigned this Aug 16, 2024
@MarcyLina MarcyLina added the enhancement New feature or request label Aug 16, 2024
Copy link
Contributor

@andrewmile andrewmile left a 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) {
Copy link
Contributor

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');
Copy link
Contributor

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.

app/Http/Controllers/PreferenceController.php Outdated Show resolved Hide resolved

$user->is_subscriber = false;
$user->unsubscribe_token = null;
$user->save();
Copy link
Contributor

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.

$this->resources = $resources;
$this->user = $user;
$this->unsubscribeUrl = $unsubscribeUrl;
}
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 be rewritten like the following and then the properties on lines 16-18 can be removed.

Suggested change
}
public function __construct(
public $resources,
public $user,
public $unsubscribeUrl
) {}

Copy link
Contributor

@techenby techenby left a comment

Choose a reason for hiding this comment

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

Looks good :)

Comment on lines 13 to 15
$user->is_subscriber = false;
$user->unsubscribe_token = null;
$user->save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$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);
Copy link
Contributor

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.

Copy link
Contributor

@andrewmile andrewmile left a comment

Choose a reason for hiding this comment

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

Great job! 👏

@MarcyLina MarcyLina merged commit 69f5b6c into main Sep 27, 2024
@MarcyLina MarcyLina deleted the mla/monthly-resource-digest branch September 27, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants