Skip to content

Conversation

@KilianTrunk
Copy link
Contributor

No description provided.

@KilianTrunk KilianTrunk marked this pull request as draft July 11, 2025 12:29
@KilianTrunk KilianTrunk marked this pull request as ready for review July 11, 2025 12:49
Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

  1. For the message, allow rich text / WYS editor
  2. Sender (user) email should be set as the "reply to" address
  3. Refactor actions: a) move stuff from the trait to SendEmailActionclass and delete the trait, b) SendEmailTableAction should extend SendEmailAction and overload the make method

@SlimDeluxe
Copy link
Member

SlimDeluxe commented Jul 19, 2025

  1. In the context of this PR, can you check why the tests are failing? This happens for all PRs that you guys submit, but not for my PR that I tried recently.
image See the workflow run for details.
  1. Please resolve the conflict (merge from main I guess).

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2025

Codecov Report

❌ Patch coverage is 69.00212% with 146 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (2ae5449) to head (e991e3b).

Files with missing lines Patch % Lines
src/Filament/Actions/SendEmailAction.php 62.29% 46 Missing ⚠️
src/Filament/Resources/MailLogResource.php 77.38% 45 Missing ⚠️
src/Listeners/SendEmailSuccessNotification.php 0.00% 23 Missing ⚠️
src/Mail/SendEmailToUser.php 62.26% 20 Missing ⚠️
src/Listeners/LogEmailToDatabase.php 75.55% 11 Missing ⚠️
...Filament/Resources/UserResource/Pages/ViewUser.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #18      +/-   ##
============================================
- Coverage     74.50%   73.10%   -1.41%     
- Complexity      225      289      +64     
============================================
  Files            45       54       +9     
  Lines          1377     1848     +471     
============================================
+ Hits           1026     1351     +325     
- Misses          351      497     +146     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KilianTrunk KilianTrunk requested a review from SlimDeluxe July 25, 2025 22:37
Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

It works!

You can now work on the details

  • Move the nav link to Tools, after "Application Health"
  • When using the log as mailer, the listener does not appear to work. Is it tricky to set up also for log?
  • The mail log table should be full width (see catalogue products table)
  • Some columns are not visible in the UI
  • Remove unnecessary permissions for mail log resource

See below for code comments

@KilianTrunk
Copy link
Contributor Author

  • When using the log as mailer, the listener does not appear to work. Is it tricky to set up also for log?

what do you mean with this?

if you mean MAIL_MAILER=log configuration in the .env file, then this is what I'm using as well - so what you're saying is that it's not working on your side? what exactly doesn't work? not sure how the listener wouldn't pick it up 🤔

@SlimDeluxe
Copy link
Member

Yes that's what I mean, but it wasn't working for me 😐 Maybe I forgot to restart horizon, I'll try again.

@SlimDeluxe
Copy link
Member

It works now, must have been horizon's fault, since emails are sent using the queue and any code changes require a manual restart (horizon:terminate).

However, it seems this last item is still unresolved:
"Remove unnecessary permissions for mail log resource"
I guess only these two are relevant:

image

@KilianTrunk
Copy link
Contributor Author

KilianTrunk commented Jul 30, 2025

"Remove unnecessary permissions for mail log resource"
I guess only these two are relevant:

should be fixed now

@SlimDeluxe SlimDeluxe merged commit bc76271 into DataLinx:main Jul 30, 2025
1 check passed
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.

3 participants