Send Test Mail Feature

[askvortsov1]

Fixes #1655

Changes proposed in this pull request:

• Added Admin endpoint for sending a test email
• Added frontend support for sending test emails
• Added TemporarySettingsRepository to encapsulate settings for temporary use (not saved in db)

Reviewers should focus on:

• Should the endpoint be moved to API?
• Are there better ways to handle settings encapsulations than the TemporarySettingsRepository
• Code style?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[franzliedke]

Some high-level things before I dive deeper into the code:

1. Can we see a screenshot, please? 🙏
2. What about using the current user's email address? That would let us get rid of quite a bit of complexity surrounding that extra field, and frankly, it's probably the address you're gonna enter anyways.

[franzliedke]

Oh, one more thing... question for @flarum/core:
Depending on how we implement this, this test mail would either be sent via the queue (if a real queue is enabled) or ... not. I would think in this one case we actually want synchronous sending, right (as the queue would keep retrying and wouldn't give us direct feedback).

[askvortsov1]

Good idea about using the email address. And yeah I considered doing it through the queue, but we would need it to be synchronous, which is why so much logic is recreated.

Updated to use the current user's email address. This is cleaner, and also prevents possible spamming abuse (if an admin's credentials leak and we have very very lazy hackers :D).

Screenshots:

• https://i.imgur.com/a8XudbN.png
• https://i.imgur.com/q0XHYhY.png

As a side note, what's the 'mail' driver supposed to do? Looks like it always fails... I'm seeing it's the "NullDriver", but maybe we should call it something else in the frontend for clarity?

[askvortsov1]

Thanks, will update! I'm not super familiar with mithril so I appreciate the advice!

Edit: Done!

[luceos]

If you use dispatchNow just use the Bus instead of the queue. The bus has dispatchNow to fire off the job immediately.

[askvortsov1]

My point is, I don't think we should save incorrect settings, so while saving the settings reduces the amount of code, it's also un intuitive to the user and might break the system. That's why I don't want to call into the app's mailer. Right now, I'm essentially reconstructing a SwiftMailer and just using that, were y'all thinking of a different approach?

[clarkwinkelmann]

How do other platforms do it ?

I was under the impression most systems first save the email info, and then offer a way to test the currently saved configuration.

IMO Sending the test email without saving the settings first could leave people believing everything is okay and closing the window without saving the settings they just entered.

If you use the saved credentials, you're testing that the currently saved configuration is valid. Which I think is the most important.

[askvortsov1]

I modelled this off of what I remember while using Discourse. I also think that since the send test email button is before the send button, that should be clear to users. I could add additional text clarifying. From a UX perspective, I'd prefer to be able to test send until it works, then save.

IMO it's better not to save credentials unless we know theyre valid. Yes it would make the code cleaner, but if our goal is to minimize incorrect configuration. Tbh, I'd prefer to go as far as disabling the save settings button until a test email has been sent

[clarkwinkelmann]

I think saving credentials that are not yet valid is a perfectly valid use case, so requiring sending a test email would be problematic for that use case. Also from a development standpoint I might be offline while I configure email, this shouldn't prevent saving some settings.

Usually when entering new email settings you don't yet have a valid setup. So saving or not saving the potentially invalid credentials wouldn't change a thing 🤷‍♂️

The only case where I might want to test before saving is when changing from one email provider to another. Seeing this makes the code more complex, I'd recommend not including that in core. An extension could provide a way to test email credentials without saving them if someone needs that feature. The test email button could simply be grayed out until you save credentials.

My opinion of course. I would love to hear what other think.

Random thought: how often do people enter wrong credentials ? It's been years since I had email problems. I always copy-paste the credentials from my provider and it just works.

[franzliedke]

I totally agree with Clark. Also consider that the "Test Email" button can be useful to debug / confirm temporary problems with sending emails (rate limiting etc.) - no need for changing any settings there.

The test email button could simply be grayed out until you save credentials.

Exactly, I was going to suggest that as well. We already do the opposite for the "Save" button IIRC - it is grayed out until at least one value has been changed.

[franzliedke]

P.S.: My suggestion was not primarily about simplifying the code - it's how I've always envisioned that feature. Sorry for not being clearer in the ticket.

[askvortsov1]

Ok, sounds good, I'll rework this

[askvortsov1]

P.S.: My suggestion was not primarily about simplifying the code - it's how I've always envisioned that feature. Sorry for not being clearer in the ticket.

Done!

[luceos]

One nitpick. Not sure what the others think. Would it make more sense to use route test/mail instead, so that we can share this path for other logic? Or maybe even confirm-settings/mail?

[askvortsov1]

I think mail/test (what's being proposed here) only makes sense if we change mail-settings to mail/settings (which I think I have in this PR) (which on second thought I should split into a second PR, I'll do that later today). I'm not sure about confirm-settings/mail, since it's not really a confirmation (i see that more as a user action), but I'm fine with both test/mail and mail/test

[dsevillamartin]

I feel like it'd be better to have them as /mail/settings and /mail/test, not the other way around...

[franzliedke]

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

[clarkwinkelmann]

Some things I noticed.

I like how there's less and less code after each review. Less code while retaining readability for the same feature is better :)

[askvortsov1]

Thanks for the review, now simplified further!

[luceos]

Looking great, love the iterations to a clearer codebase.

[clarkwinkelmann]

Some more observations

[clarkwinkelmann]

Oops I see you've taken my suggestions to the max. I actually think it's a bit too much now 😅

• Regarding the FieldSet, I meant that one FieldSet that only had a children attribute. I think changing everything in f659234 is too much, it doesn't help readability very much for the FieldSets with many props
• Regarding the API path changes, I think it's fine to do it in this PR. I was just wondering whether the change in the serializer was intentional. The serializer type doesn't have to match with the API path exactly. I was finding it a bit odd to have mail/settings as the serializer type (we've not used a slash anywhere else yet). The URL could be mail/settings while the serializer stays as mail-settings ?

[dsevillamartin]

Some code clean up. I haven't looked at the whole thing yet.

[clarkwinkelmann]

I still haven't tested it locally, but it's looking fine now.

To still nitpick, I see there's still a diff on lines 101-now-102 and 107-now-108 of MailPage, were those edits actually required?