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

SMTP Emailer #5951

Closed
pitaj opened this Issue Oct 1, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@pitaj
Copy link
Contributor

pitaj commented Oct 1, 2017

@Dravere

  • Add option for selecting security setting
  • Typo at emailer.js line 66
    serice => service
  • Allow for use without authentication
  • Move "custom" setting to top of dropdown
  • Update descriptions to tell admin that a restart is required to apply settings
    Possibly make it so restart is not required?
  • Update email-local readme to tell admins to use the core ACP settings for the same features

ref NodeBB/nodebb-plugin-emailer-local#18

@Dravere

This comment has been minimized.

Copy link
Contributor

Dravere commented Oct 1, 2017

Do you want this in separate pull-requests? I might have time tomorrow to tackle those issues.

@pitaj

This comment has been minimized.

Copy link
Contributor

pitaj commented Oct 1, 2017

Whatever is easier for you. I might work on some tonight, I'll give you the branch if I do end up working on it.

@barisusakli barisusakli added this to the 1.6.1 milestone Oct 2, 2017

@Dravere

This comment has been minimized.

Copy link
Contributor

Dravere commented Oct 2, 2017

I started working on this: https://github.com/Dravere/NodeBB/tree/smtp-emailer-issue-5951

First three points are done. Quick question though: How should the translation be done? Should I just add the strings to the en-GB/.../email.json language file? Should I also add them to en-US/.../email.json and since I know German also de/.../email.json? What is the proper procedure?

I'm now also going to start working in a different branch on point four. I'll look into if it is possible to apply those settings without a restart. Would be the nicest.

@pitaj

This comment has been minimized.

Copy link
Contributor

pitaj commented Oct 2, 2017

The proper procedure is only to add them to en-GB translation files, and then those changes get propagated through transifex to other languages automatically.

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Oct 2, 2017

@pitaj @Dravere please be sure to ping me when the PR is merged so I can manually push it up to tx. @nodebb-misty is supposed to push changes to en-GB up to Transifex, but I don't think it is working right now.

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Oct 2, 2017

@pitaj Additionally, please relocate the "custom" settings option to the top of the dropdown 😄

@Dravere

This comment has been minimized.

Copy link
Contributor

Dravere commented Oct 2, 2017

Ok, I think I have also a solution for point four. All it basically needs is to extract the creation of the fallbackTransport into its own function. Then call it from Emailer.registerApp and from the pubsub conf-update callback registered in there.

No restart and no reload needed.

I'll provide a pull-request as soon as the other one passes.

@barisusakli

This comment has been minimized.

Copy link
Member

barisusakli commented Oct 4, 2017

#5954 is merged

@Dravere

This comment has been minimized.

Copy link
Contributor

Dravere commented Oct 5, 2017

Pull request suggestion for deprecation notice on the plugin: NodeBB/nodebb-plugin-emailer-local#19

I would suggest to also release it to NPM so the readme is updated on https://npmjs.com.

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Oct 5, 2017

v1.0.1 of plugin published

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Oct 5, 2017

Translations sources pushed.

@pitaj

This comment has been minimized.

Copy link
Contributor

pitaj commented Oct 10, 2017

All done

@pitaj pitaj closed this Oct 10, 2017

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