Skip to content

Fixed: Trans Choice in Unaccepted Asset Email #16237

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

akemidx
Copy link
Member

@akemidx akemidx commented Feb 12, 2025

Fixed an issue where the body of an email did not pluralize items when multiple items were unaccepted.

Screenshot 2025-02-11 at 8 02 32 PM

Copy link

what-the-diff bot commented Feb 12, 2025

PR Summary

  • Enhanced String Localization in Notification Emails
    The notification email system's 'item_checked_reminder' message has been improved to handle singular and plural forms more effectively. This was achieved through a new pipe-separated string approach.

  • Improved Grammatical Accuracy in Notification Messages
    Fixed a small grammatical issue in the 'unaccepted_asset_reminder' string. Now it uses 'Asset(s)' instead of 'Asset', appropriately representing both single and multiple assets.

  • Optimized Pluralization in Notification Messages
    Adjusted the way the 'item_checked_reminder' message is utilized in the notification system. Instead of the previous method, we now use the trans_choice function, which is more effective in handling pluralization.

  • Improved Readability in Notification Templates
    Addressed a minor formatting issue inside our notification email templates where the greeting line had incorrect spacing. After this fix, the greeting line is more readable and visually appealing.

@@ -88,7 +88,7 @@
'upcoming-audits' => 'There is :count asset that is coming up for audit within :threshold days.|There are :count assets that are coming up for audit within :threshold days.',
'user' => 'User',
'username' => 'Username',
'unaccepted_asset_reminder' => 'You have Unaccepted Assets.',
'unaccepted_asset_reminder' => 'You have Unaccepted Asset(s).',
Copy link
Member

Choose a reason for hiding this comment

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

This weeks awkward to me - I think we'd want to trans_choice the subject here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

So, yea, I figured that this would catch your attention.

I am unsure how to get that to actually work. The email text is in markdown, but the actual email title is built different. its running off a function public function envelope(): Envelope line 30 in UnacceptedAssetReminderMail.php

There is probably a pretty tricky way to get it to work tho...

@akemidx akemidx requested a review from marcusmoore March 17, 2025 20:43
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

A couple things to update. I think the merge conflict will go away if you remove the changes to web.php.

Comment on lines 32 to 34
$this->count = $count;
$from = new Address(config('mail.from.address'), config('mail.from.name'));
$subject = trans_choice('mail.unaccepted_asset_reminder', $count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think $this->count = $count; should be removed and $count should be $this->count on line 34.

routes/web.php Outdated
Comment on lines 343 to 357
Route::get('/mailable', function (){
$checkout_info = [];
$acceptance = CheckoutAcceptance::first();
//dd($acceptance);
$count = [];

$checkout_info['acceptance'] = $acceptance;

return new App\Mail\UnacceptedAssetReminderMail($checkout_info,2);
});





Copy link
Collaborator

Choose a reason for hiding this comment

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

This was for testing and needs to be removed.

@akemidx akemidx changed the base branch from master to develop April 1, 2025 20:24
@akemidx akemidx requested a review from marcusmoore April 1, 2025 21:12
@akemidx
Copy link
Member Author

akemidx commented Apr 1, 2025

EX of new subject line
Screenshot 2025-04-01 at 5 12 57 PM

@marcusmoore
Copy link
Collaborator

For clarity this changes the subject line for single items as well (from You have Unaccepted Assets to Youy have an Unaccepted Asset:

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants