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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Improve group email settings UI #13083

Merged
merged 25 commits into from May 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5827a5d
WIP commit
martin-brennan May 13, 2021
9cd0d84
WIP commit for group controller
martin-brennan May 14, 2021
b16199e
Merge branch 'master' into feature/group-email-settings-ui-part-1
martin-brennan May 18, 2021
cd79a79
More progress. Group SMTP settings interface now done.
martin-brennan May 18, 2021
fcc986c
IMAP and SMTP UI interactions and clearing now working well
martin-brennan May 20, 2021
0f3b35f
Fix scss lint
martin-brennan May 20, 2021
67f9069
Fix specs
martin-brennan May 20, 2021
630751e
IMAP mailbox selection and filtering
martin-brennan May 20, 2021
785380d
Hide IMAP settings if it is not enabled for the site
martin-brennan May 20, 2021
433a838
Fix spec and add filter_mailboxes spec
martin-brennan May 20, 2021
cb6ac05
Start group email acceptance tests
martin-brennan May 20, 2021
7a18f4e
Merge branch 'master' into feature/group-email-settings-ui-part-1
martin-brennan May 24, 2021
e4305b7
Acceptance test for group email settings
martin-brennan May 24, 2021
da9051e
Merge branch 'master' into feature/group-email-settings-ui-part-1
martin-brennan May 25, 2021
f5c4f42
Review fixes round 1, minor fixes
martin-brennan May 25, 2021
8998fda
Move provider settings for prefill into shared lib
martin-brennan May 25, 2021
95def36
Add hijack for test_email_settings
martin-brennan May 25, 2021
0396a19
Move imap/smtpSettingsValid off the group
martin-brennan May 25, 2021
2abf99a
Change clearing of email settings to be server side
martin-brennan May 25, 2021
bb2b4c1
Only show clear fields warning if any values have been entered
martin-brennan May 25, 2021
26819fc
Merge branch 'master' into feature/group-email-settings-ui-part-1
martin-brennan May 25, 2021
3201f8c
Move gmail related code into provider specific areas
martin-brennan May 26, 2021
e18cab5
Make form usable on mobile and with keyboard
martin-brennan May 26, 2021
8e14aa0
SCSS lint
martin-brennan May 26, 2021
cfffcef
Keep testing button disabled until all fields are filled
martin-brennan May 26, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/javascripts/application.js
Expand Up @@ -22,6 +22,7 @@
//= require ./discourse/app/lib/offset-calculator
//= require ./discourse/app/lib/lock-on
//= require ./discourse/app/lib/url
//= require ./discourse/app/lib/email-provider-default-settings
//= require ./discourse/app/lib/debounce
//= require ./discourse/app/lib/quote
//= require ./discourse/app/lib/key-value-store
Expand Down
@@ -0,0 +1,91 @@
import Component from "@ember/component";
import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings";
import { isEmpty } from "@ember/utils";
import { popupAjaxError } from "discourse/lib/ajax-error";
import discourseComputed, { on } from "discourse-common/utils/decorators";
import EmberObject, { action } from "@ember/object";
import { ajax } from "discourse/lib/ajax";

export default Component.extend({
tagName: "",
udan11 marked this conversation as resolved.
Show resolved Hide resolved
form: null,

@discourseComputed(
"group.email_username",
"group.email_password",
"form.imap_server",
"form.imap_port"
)
missingSettings(email_username, email_password, imap_server, imap_port) {
return [
email_username,
email_password,
imap_server,
imap_port,
].some((value) => isEmpty(value));
},

@discourseComputed("group.imap_mailboxes")
mailboxes(imapMailboxes) {
if (!imapMailboxes) {
return [];
}
return imapMailboxes.map((mailbox) => ({ name: mailbox, value: mailbox }));
},

@discourseComputed("group.imap_mailbox_name", "mailboxes.length")
mailboxSelected(mailboxName, mailboxesSize) {
return mailboxesSize === 0 || !isEmpty(mailboxName);
},

@action
resetSettingsValid() {
this.set("imapSettingsValid", false);
},

@on("init")
_fillForm() {
this.set(
"form",
EmberObject.create({
imap_server: this.group.imap_server,
imap_port: (this.group.imap_port || "").toString(),
imap_ssl: this.group.imap_ssl,
})
);
},

@action
prefillSettings(provider) {
this.form.setProperties(emailProviderDefaultSettings(provider, "imap"));
},

@action
testImapSettings() {
const settings = {
host: this.form.imap_server,
port: this.form.imap_port,
ssl: this.form.imap_ssl,
username: this.group.email_username,
password: this.group.email_password,
};

this.set("testingSettings", true);
this.set("imapSettingsValid", false);

return ajax(`/groups/${this.group.id}/test_email_settings`, {
type: "POST",
data: Object.assign(settings, { protocol: "imap" }),
})
.then(() => {
this.set("imapSettingsValid", true);
this.group.setProperties({
imap_server: this.form.imap_server,
imap_port: this.form.imap_port,
imap_ssl: this.form.imap_ssl,
});
})
.catch(popupAjaxError)
.finally(() => this.set("testingSettings", false));
},
});
@@ -0,0 +1,116 @@
import Component from "@ember/component";
import { isEmpty } from "@ember/utils";
import discourseComputed, { on } from "discourse-common/utils/decorators";
import I18n from "I18n";
import bootbox from "bootbox";
import { action } from "@ember/object";

export default Component.extend({
tagName: "",

imapSettingsValid: false,
smtpSettingsValid: false,

@on("init")
_determineSettingsValid() {
this.set(
"imapSettingsValid",
this.group.imap_enabled && this.group.imap_server
);
this.set(
"smtpSettingsValid",
this.group.smtp_enabled && this.group.smtp_server
);
},

@discourseComputed(
"emailSettingsValid",
"group.smtp_enabled",
"group.imap_enabled"
)
enableImapSettings(emailSettingsValid, smtpEnabled, imapEnabled) {
return smtpEnabled && (emailSettingsValid || imapEnabled);
},

@discourseComputed(
"smtpSettingsValid",
"imapSettingsValid",
"group.smtp_enabled",
"group.imap_enabled"
)
emailSettingsValid(
smtpSettingsValid,
imapSettingsValid,
smtpEnabled,
imapEnabled
) {
return (
(!smtpEnabled || smtpSettingsValid) && (!imapEnabled || imapSettingsValid)
);
},

_anySmtpFieldsFilled() {
return [
this.group.smtp_server,
this.group.smtp_port,
this.group.email_username,
this.group.email_password,
].some((value) => !isEmpty(value));
},

_anyImapFieldsFilled() {
return [this.group.imap_server, this.group.imap_port].some(
(value) => !isEmpty(value)
);
},

@action
smtpEnabledChange(event) {
if (
!event.target.checked &&
this.group.smtp_enabled &&
this._anySmtpFieldsFilled()
) {
bootbox.confirm(
I18n.t("groups.manage.email.smtp_disable_confirm"),
(result) => {
if (!result) {
this.group.set("smtp_enabled", true);
} else {
this.group.set("imap_enabled", false);
}
}
);
}

this.group.set("smtp_enabled", event.target.checked);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about using a buffered proxy can be applied to this component too. Simply use a buffered proxy to hold the "dirty" state of the object and commit the changes to the object after the request to the server succeeded (or replace object entirely with server's response and reset the buffer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about this with the buffered proxy? Seems like we only use it in one place, and I am not sure what the benefit is over using just a simple object to shadow these properties?

@discourseComputed("site.categories.[]")
categoriesBuffered(categories) {
const bufProxy = EmberObjectProxy.extend(BufferedMixin || BufferedProxy);
return (categories || []).map((c) => bufProxy.create({ content: c }));
},

Copy link
Contributor

Choose a reason for hiding this comment

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

We use it in many places via bufferedProperty. One recent place I used is in the create-invite modal where the use case is similar.

The biggest benefit I find is that you can achieve more with less code, but I guess there are other benefits such as the initial object never being "dirty" - in most cases it will be an exact copy of what is on the server and that you do not have to copy the value of the properties and keep them up-to-date when they are updated in the original object.

},

@action
imapEnabledChange(event) {
martin-brennan marked this conversation as resolved.
Show resolved Hide resolved
if (
!event.target.checked &&
this.group.imap_enabled &&
this._anyImapFieldsFilled()
) {
bootbox.confirm(
I18n.t("groups.manage.email.imap_disable_confirm"),
(result) => {
if (!result) {
this.group.set("imap_enabled", true);
}
}
);
}

this.group.set("imap_enabled", event.target.checked);
},

@action
afterSave() {
// reload the group to get the updated imap_mailboxes
this.store.find("group", this.group.name).then(() => {
this._determineSettingsValid();
});
},
});
Expand Up @@ -7,6 +7,7 @@ import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new"

export default Component.extend({
saving: null,
disabled: false,

@discourseComputed("saving")
savingText(saving) {
Expand All @@ -15,6 +16,10 @@ export default Component.extend({

actions: {
save() {
if (this.beforeSave) {
this.beforeSave();
}

this.set("saving", true);
const group = this.model;

Expand All @@ -31,6 +36,10 @@ export default Component.extend({
}

this.set("saved", true);

if (this.afterSave) {
this.afterSave();
}
})
.catch(popupAjaxError)
.finally(() => this.set("saving", false));
Expand Down
@@ -0,0 +1,82 @@
import Component from "@ember/component";
import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings";
import { isEmpty } from "@ember/utils";
import { popupAjaxError } from "discourse/lib/ajax-error";
import discourseComputed, { on } from "discourse-common/utils/decorators";
import EmberObject, { action } from "@ember/object";
import { ajax } from "discourse/lib/ajax";

export default Component.extend({
martin-brennan marked this conversation as resolved.
Show resolved Hide resolved
tagName: "",
form: null,

@discourseComputed(
"form.email_username",
"form.email_password",
"form.smtp_server",
"form.smtp_port"
)
missingSettings(email_username, email_password, smtp_server, smtp_port) {
return [
email_username,
email_password,
smtp_server,
smtp_port,
].some((value) => isEmpty(value));
},

@action
resetSettingsValid() {
this.set("smtpSettingsValid", false);
},

@on("init")
_fillForm() {
this.set(
"form",
EmberObject.create({
email_username: this.group.email_username,
email_password: this.group.email_password,
smtp_server: this.group.smtp_server,
smtp_port: (this.group.smtp_port || "").toString(),
smtp_ssl: this.group.smtp_ssl,
})
);
},

@action
prefillSettings(provider) {
this.form.setProperties(emailProviderDefaultSettings(provider, "smtp"));
},

@action
testSmtpSettings() {
const settings = {
host: this.form.smtp_server,
port: this.form.smtp_port,
ssl: this.form.smtp_ssl,
username: this.form.email_username,
password: this.form.email_password,
};

this.set("testingSettings", true);
this.set("smtpSettingsValid", false);

return ajax(`/groups/${this.group.id}/test_email_settings`, {
type: "POST",
data: Object.assign(settings, { protocol: "smtp" }),
})
.then(() => {
this.set("smtpSettingsValid", true);
this.group.setProperties({
smtp_server: this.form.smtp_server,
smtp_port: this.form.smtp_port,
smtp_ssl: this.form.smtp_ssl,
email_username: this.form.email_username,
email_password: this.form.email_password,
});
})
.catch(popupAjaxError)
.finally(() => this.set("testingSettings", false));
},
});
6 changes: 6 additions & 0 deletions app/assets/javascripts/discourse/app/lib/ajax-error.js
@@ -1,4 +1,5 @@
import I18n from "I18n";
import { isTesting } from "discourse-common/config/environment";
import bootbox from "bootbox";

export function extractError(error, defaultMessage) {
Expand Down Expand Up @@ -69,6 +70,11 @@ export function popupAjaxError(error) {
}
bootbox.alert(extractError(error));

// in testing mode we want to be able to test these ajax popup messages
if (isTesting()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if this will come back to bite me...I think if all the acceptance tests pass it should be OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well they ran okay! As a little more context I wanted to test a bad response here:

https://github.com/discourse/discourse/pull/13083/files#diff-785a91ce827d2667a7e90342377d3f89c3e8c6715f8f75a5efc9894f41fb479dR201-R206

But if I just did that the actual error was thrown and stopped the tests, even though the popup shows the message :/

return;
}

error._discourse_displayed = true;

// We re-throw in a catch to not swallow the exception
Expand Down
@@ -0,0 +1,22 @@
const GMAIL = {
imap: {
imap_server: "imap.gmail.com",
imap_port: "993",
imap_ssl: true,
},
smtp: {
smtp_server: "smtp.gmail.com",
smtp_port: "587",
smtp_ssl: true,
},
};

export default function emailProviderDefaultSettings(provider, protocol) {
provider = provider.toLowerCase();
protocol = protocol.toLowerCase();

switch (provider) {
case "gmail":
return GMAIL[protocol];
}
}
2 changes: 2 additions & 0 deletions app/assets/javascripts/discourse/app/models/group.js
Expand Up @@ -217,10 +217,12 @@ const Group = RestModel.extend({
smtp_server: this.smtp_server,
smtp_port: this.smtp_port,
smtp_ssl: this.smtp_ssl,
smtp_enabled: this.smtp_enabled,
imap_server: this.imap_server,
imap_port: this.imap_port,
imap_ssl: this.imap_ssl,
imap_mailbox_name: this.imap_mailbox_name,
imap_enabled: this.imap_enabled,
email_username: this.email_username,
email_password: this.email_password,
flair_icon: null,
Expand Down
Expand Up @@ -5,7 +5,8 @@ export default DiscourseRoute.extend({
showFooter: true,

beforeModel() {
if (!this.siteSettings.enable_imap && !this.siteSettings.enable_smtp) {
// cannot configure IMAP without SMTP being enabled
if (!this.siteSettings.enable_smtp) {
udan11 marked this conversation as resolved.
Show resolved Hide resolved
return this.transitionTo("group.manage.profile");
}
},
Expand Down