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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of incomplete mail configuration #1921

Merged
merged 6 commits into from Feb 7, 2020

Conversation

@datitisev
Copy link
Member

datitisev commented Oct 27, 2019

Fixes #1763

Changes proposed in this pull request:

  • Create NullTransport
  • Add requiredFields method to DriverInterface
  • Warning to JS side if required fields aren't filled
  • * required indicator next to required fields

Reviewers should focus on:

  • Not sure if the code on the JS is good... the whole file is a mess 馃槚.
  • Do we want to use requiredFields for the name? And what about drivers where all fields are required? I simply made a call to the availableSettings method because it was pointless copy/pasting it.
  • How's the logic for deciding to use NullTransport? Seems to work just fine right now, the code isn't very pretty though.
  • Is the translation key ok?

Screenshot

Confirmed

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

Required changes:

  • Related core extension PRs
    • flarum/english PR required - Franz has a commit waiting on his machine
@datitisev datitisev changed the title Add required fields, incomplete configuration warning, and null transport Improve handling of incomplete mail configuration Oct 27, 2019
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 11, 2019

The whole implementation is .. scary. Makes me think of that Backup logic I've shown you @datitisev ; can't we port that? So a setting => validation mapping and parsed into the frontend for easier rendering? Very much out of scope, but worth considering before stable?

@luceos luceos requested a review from franzliedke Nov 11, 2019
@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Nov 11, 2019

@luceos Hm yeah, perhaps that'd be better. It'd allow for frontend validation rules as well, for example (whoops I did not finish reading your message). But isn't that basically more doable with the changes from #1850 then?

Yeah, I think that way it would probably be better. I could try to do so for this release, if I have the time after the items with higher priority are taken care of.

@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Nov 14, 2019

Ok, so I attempted adding validation to the mail system. I don't like how I hooked the PHP side of things together, but it works as of right now. Please do improve it if you see some glaring issues with the implementation 馃檪.

I also fixed flattenMap not working in IE, so I added a flatten function from https://www.30secondsofcode.org/snippet/deepFlatten/ (they don't require attribution, see https://www.30secondsofcode.org/about).

The weird regular expression is for the hostname, not sure if we want it.

image

Copy link
Member

franzliedke left a comment

I'm afraid this isn't ready yet. It's also unclear to me what effect this has in the frontend right now. Granted, #1169 wasn't 100% precise there. What I had in mind, I think, was a sort of alert message on the admin dashboard that warns you about an invalid / outdated email configuration - not only an error message upon saving (although I cannot quite recognize where even that would come from right now).

src/Forum/ValidateMailConfiguration.php Outdated Show resolved Hide resolved
src/Forum/ValidateMailConfiguration.php Outdated Show resolved Hide resolved
@franzliedke franzliedke force-pushed the ds/1763-handle-incomplete-email-configuration branch from df30ce9 to 8c95c98 Jan 24, 2020
datitisev and others added 5 commits Oct 27, 2019
鈥ort
鈥 shown in the frontend
@franzliedke franzliedke force-pushed the ds/1763-handle-incomplete-email-configuration branch from a0c22fd to 833ea4e Jan 24, 2020
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 24, 2020

Okay... I finished this up. It's not 100% perfect in terms of UX (e.g. server-side validation only), but a good step forward.
@datitisev Thanks for your initial implementation!

This is what it looks like now:
Screenshot of mail settings with errors

@jordanjay29 @luceos @datitisev Please let me know what you think.


@gwillem will be happy about this, I believe! 馃槂

@franzliedke franzliedke requested a review from luceos Jan 24, 2020
@gwillem

This comment has been minimized.

Copy link
Contributor

gwillem commented Jan 24, 2020

Woot! I am HAPPY! 馃槅 馃帀 馃嵕

@franzliedke franzliedke requested a review from jordanjay29 Jan 24, 2020
By commenting out the new methods on the `DriverInterface` and checking
for these methods' existence before calling them, old implementations in
extensions will not break right away.

This will be removed after beta.12 is released, giving extension authors
about two months time to update their extensions.
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 24, 2020

Now, I also added a backwards-compatibility layer, just like I did after introducing the mail driver field types in PR #1850.

Both will be removed after releasing beta.12.

@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Jan 24, 2020

@franzliedke Looks good to me, I can see that it's cleaner 馃憤.

@franzliedke franzliedke dismissed luceos鈥檚 stale review Jan 31, 2020

This was taken care of.

@franzliedke franzliedke merged commit 1d7641c into master Feb 7, 2020
19 checks passed
19 checks passed
PHP 7.1 / MySQL
Details
PHP 7.1 / MySQL
Details
PHP 7.1 / MariaDB
Details
PHP 7.1 / MariaDB
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MariaDB
Details
PHP 7.2 / MariaDB
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB (prefix)
Details
PHP 7.3 / MariaDB (prefix)
Details
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@franzliedke franzliedke deleted the ds/1763-handle-incomplete-email-configuration branch Feb 7, 2020
franzliedke added a commit to flarum/lang-english that referenced this pull request Feb 7, 2020
@franzliedke franzliedke mentioned this pull request Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can鈥檛 perform that action at this time.