diff --git a/src/client/accounts/account-manager.vala b/src/client/accounts/account-manager.vala index aa10d711..05280952 100644 --- a/src/client/accounts/account-manager.vala +++ b/src/client/accounts/account-manager.vala @@ -69,6 +69,10 @@ errordomain AccountError { public class AccountManager : GLib.Object { + private const string LOCAL_ID_PREFIX = "account_"; + private const string LOCAL_ID_FORMAT = "account_%02u"; + private const string GOA_ID_PREFIX = "goa_"; + private const string ACCOUNT_CONFIG_GROUP = "AccountInformation"; private const string ACCOUNT_MANAGER_GROUP = "AccountManager"; private const string IMAP_CONFIG_GROUP = "IMAP"; @@ -94,8 +98,6 @@ public class AccountManager : GLib.Object { private const string TRASH_FOLDER_KEY = "trash_folder"; private const string USE_EMAIL_SIGNATURE_KEY = "use_email_signature"; - private const string GOA_ID_PREFIX = "goa_"; - /** * Specifies the overall status of an account. @@ -222,10 +224,37 @@ public class AccountManager : GLib.Object { this.goa_service.account_removed.connect(on_goa_account_removed); } + /** + * Returns a new account, not yet stored on disk. + */ + public Geary.AccountInformation + new_orphan_account(Geary.ServiceProvider provider, + Geary.ServiceInformation imap, + Geary.ServiceInformation smtp) { + string? last_account = this.accounts.keys.fold((next, last) => { + string? result = last; + if (next.has_prefix(LOCAL_ID_PREFIX)) { + result = (last == null || strcmp(last, next) < 0) ? next : last; + } + return result; + }, + null); + uint next_id = 1; + if (last_account != null) { + next_id = int.parse(last_account.substring(LOCAL_ID_PREFIX.length)) + 1; + } + string id = LOCAL_ID_FORMAT.printf(next_id); + + return new Geary.AccountInformation(id, provider, imap, smtp); + } + public LocalServiceInformation new_libsecret_service(Geary.Protocol service) { return new LocalServiceInformation(service, libsecret); } + /** + * Creates new account's disk and credential storage as needed. + */ public async void create_account(Geary.AccountInformation account, GLib.Cancellable? cancellable) throws GLib.Error { @@ -757,52 +786,34 @@ public class AccountManager : GLib.Object { smtp.load_settings(smtp_config); } - Geary.AccountInformation info = new Geary.AccountInformation( - id, imap, smtp + return new Geary.AccountInformation( + id, provider, imap, smtp ); - info.service_provider = provider; - - // Known providers such as GMail will have a label specified - // by clients, but other accounts can only really be - // identified by their server names. Try to extract a 'nice' - // value for the label here. - if (provider == Geary.ServiceProvider.OTHER) { - string imap_host = imap.host; - string[] host_parts = imap_host.split("."); - if (host_parts.length > 1) { - host_parts = host_parts[1:host_parts.length]; - } - info.service_label = string.joinv(".", host_parts); - } - - return info; } private Geary.AccountInformation new_goa_account(string id, Goa.Object account) { GoaMediator mediator = new GoaMediator(account); - Geary.AccountInformation info = new Geary.AccountInformation( - id, - new GoaServiceInformation(Geary.Protocol.IMAP, mediator, account), - new GoaServiceInformation(Geary.Protocol.SMTP, mediator, account) - ); - - info.service_label = account.get_account().provider_name; + Geary.ServiceProvider provider = Geary.ServiceProvider.OTHER; switch (account.get_account().provider_type) { case "google": - info.service_provider = Geary.ServiceProvider.GMAIL; + provider = Geary.ServiceProvider.GMAIL; break; case "windows_live": - info.service_provider = Geary.ServiceProvider.OUTLOOK; - break; - - default: - info.service_provider = Geary.ServiceProvider.OTHER; + provider = Geary.ServiceProvider.OUTLOOK; break; } + Geary.AccountInformation info = new Geary.AccountInformation( + id, + provider, + new GoaServiceInformation(Geary.Protocol.IMAP, mediator, account), + new GoaServiceInformation(Geary.Protocol.SMTP, mediator, account) + ); + info.service_label = account.get_account().provider_name; + return info; } diff --git a/src/client/accounts/add-edit-page.vala b/src/client/accounts/add-edit-page.vala index 66a05a95..21a38bb4 100644 --- a/src/client/accounts/add-edit-page.vala +++ b/src/client/accounts/add-edit-page.vala @@ -697,7 +697,9 @@ public class AddEditPage : Gtk.Box { ); try { - info = this.application.engine.create_orphan_account(imap, smtp); + info = this.application.controller.account_manager.new_orphan_account( + this.get_service_provider(), imap, smtp + ); } catch (Error err) { debug("Unable to create account %s for %s: %s", this.id, this.email_address, err.message); @@ -717,7 +719,6 @@ public class AddEditPage : Gtk.Box { info.smtp.credentials = smtp_credentials; info.imap.remember_password = this.remember_password; info.smtp.remember_password = this.remember_password; - info.service_provider = this.get_service_provider(); info.save_sent_mail = this.save_sent_mail; info.imap.host = this.imap_host; info.imap.port = this.imap_port; diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index 50e6a9ff..c1b35d1b 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -92,6 +92,14 @@ public class Geary.AccountInformation : BaseObject { */ public string id { get; private set; } + /** Specifies the email provider for this account. */ + public Geary.ServiceProvider service_provider { get; private set; } + + /** A human-readable label describing the email service provider. */ + public string service_label { + get; public set; + } + /** * A unique human-readable display name for this account. * @@ -133,16 +141,6 @@ public class Geary.AccountInformation : BaseObject { */ public Gee.List? alternate_mailboxes { get; private set; default = null; } - /** Specifies the email provider for this account. */ - public Geary.ServiceProvider service_provider { - get; set; default = Geary.ServiceProvider.OTHER; - } - - /** A human-readable label describing the service. */ - public string service_label { - get; set; default = ""; - } - public int prefetch_period_days { get; set; default = DEFAULT_PREFETCH_PERIOD_DAYS; } @@ -215,18 +213,36 @@ public class Geary.AccountInformation : BaseObject { * Creates a new, empty account info file. */ public AccountInformation(string id, + ServiceProvider provider, ServiceInformation imap, ServiceInformation smtp) { this.id = id; + this.service_provider = provider; this.imap = imap; this.smtp = smtp; + + // Known providers such as Gmail will have a label specified + // by clients, but other accounts can only really be + // identified by their server names. Try to extract a 'nice' + // value for label based on service host names. + string imap_host = imap.host; + string[] host_parts = imap_host.split("."); + if (host_parts.length > 1) { + host_parts = host_parts[1:host_parts.length]; + } + this.service_label = string.joinv(".", host_parts); } /** * Creates a copy of an instance. */ public AccountInformation.temp_copy(AccountInformation from) { - this(from.id, from.imap.temp_copy(), from.smtp.temp_copy()); + this( + from.id, + from.service_provider, + from.imap.temp_copy(), + from.smtp.temp_copy() + ); copy_from(from); this.is_copy = true; } @@ -245,7 +261,6 @@ public class Geary.AccountInformation : BaseObject { foreach (RFC822.MailboxAddress alternate_mailbox in from.alternate_mailboxes) add_alternate_mailbox(alternate_mailbox); } - this.service_provider = from.service_provider; this.prefetch_period_days = from.prefetch_period_days; this.save_sent_mail = from.save_sent_mail; this.ordinal = from.ordinal; diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala index 3fce1cfa..6ccd1786 100644 --- a/src/engine/api/geary-engine.vala +++ b/src/engine/api/geary-engine.vala @@ -16,9 +16,6 @@ */ public class Geary.Engine : BaseObject { - private const string ID_PREFIX = "account_"; - private const string ID_FORMAT = "account_%02u"; - [Flags] public enum ValidationOption { NONE = 0, @@ -203,42 +200,6 @@ public class Geary.Engine : BaseObject { return accounts.read_only_view; } - /** - * Returns a new account, not yet stored on disk. - * - * Throws an error if the engine has not been opened or if an - * invalid account id is generated. - */ - public AccountInformation create_orphan_account(ServiceInformation imap, - ServiceInformation smtp) - throws GLib.Error { - check_opened(); - - // We might want to allow the client to specify the id, but - // just generate one here for now: Use a common prefix and a - // numeric suffix, starting at 1. To generate the next id, - // find the last account and increment its suffix. - - string? last_account = this.accounts.keys.fold((next, last) => { - string? result = last; - if (next.has_prefix(ID_PREFIX)) { - result = (last == null || strcmp(last, next) < 0) ? next : last; - } - return result; - }, - null); - uint next_id = 1; - if (last_account != null) { - next_id = int.parse(last_account.substring(ID_PREFIX.length)) + 1; - } - string id = ID_FORMAT.printf(next_id); - - if (this.accounts.has_key(id)) - throw new EngineError.ALREADY_EXISTS("Account %s already exists", id); - - return new AccountInformation(id, imap, smtp); - } - /** * Returns whether the account information "validates." If validate_connection is true, * we check if we can connect to the endpoints and authenticate using the supplied credentials. diff --git a/test/client/accounts/account-manager-test.vala b/test/client/accounts/account-manager-test.vala new file mode 100644 index 00000000..eee44635 --- /dev/null +++ b/test/client/accounts/account-manager-test.vala @@ -0,0 +1,151 @@ +/* + * Copyright 2018 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +class AccountManagerTest : TestCase { + + + private AccountManager? test = null; + private File? tmp = null; + + + public AccountManagerTest() { + base("AccountManagerTest"); + add_test("create_account", create_account); + add_test("create_orphan_account", create_orphan_account); + add_test("create_orphan_account_with_legacy", create_orphan_account_with_legacy); + } + + public override void set_up() throws GLib.Error { + // XXX this whole thing stinks. We need to be able to test the + // engine without creating all of these dirs. + + this.tmp = GLib.File.new_for_path( + GLib.DirUtils.make_tmp("geary-engine-test-XXXXXX") + ); + + GLib.File config = this.tmp.get_child("config"); + config.make_directory(); + + GLib.File data = this.tmp.get_child("data"); + data.make_directory(); + + this.test = new AccountManager(new GearyApplication(), config, data); + } + + public override void tear_down() throws GLib.Error { + this.test = null; + @delete(this.tmp); + } + + public void create_account() throws GLib.Error { + const string ID = "test"; + Geary.AccountInformation account = new Geary.AccountInformation( + ID, + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + bool was_added = false; + bool was_enabled = false; + + this.test.account_added.connect((added, status) => { + was_added = (added == account); + was_enabled = (status == AccountManager.Status.ENABLED); + }); + + this.test.create_account.begin( + account, new GLib.Cancellable(), + (obj, res) => { async_complete(res); } + ); + this.test.create_account.end(async_result()); + + assert_int(1, this.test.size, "Account manager size"); + assert_equal(account, this.test.get_account(ID), "Is not contained"); + assert_true(was_added, "Was not added"); + assert_true(was_enabled, "Was not enabled"); + } + + public void create_orphan_account() throws GLib.Error { + Geary.AccountInformation account1 = this.test.new_orphan_account( + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + assert(account1.id == "account_01"); + + this.test.create_account.begin( + account1, new GLib.Cancellable(), + (obj, res) => { async_complete(res); } + ); + this.test.create_account.end(async_result()); + + Geary.AccountInformation account2 = this.test.new_orphan_account( + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + assert(account2.id == "account_02"); + } + + public void create_orphan_account_with_legacy() throws GLib.Error { + const string ID = "test"; + Geary.AccountInformation account = new Geary.AccountInformation( + ID, + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + + this.test.create_account.begin( + account, new GLib.Cancellable(), + (obj, res) => { async_complete(res); } + ); + this.test.create_account.end(async_result()); + + Geary.AccountInformation account1 = this.test.new_orphan_account( + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + assert(account1.id == "account_01"); + + this.test.create_account.begin( + account1, new GLib.Cancellable(), + (obj, res) => { async_complete(res); } + ); + this.test.create_account.end(async_result()); + + Geary.AccountInformation account2 = this.test.new_orphan_account( + Geary.ServiceProvider.OTHER, + new Geary.MockServiceInformation(), + new Geary.MockServiceInformation() + ); + assert(account2.id == "account_02"); + } + + private void delete(File parent) throws Error { + FileInfo info = parent.query_info( + "standard::*", + FileQueryInfoFlags.NOFOLLOW_SYMLINKS + ); + + if (info.get_file_type () == FileType.DIRECTORY) { + FileEnumerator enumerator = parent.enumerate_children( + "standard::*", + FileQueryInfoFlags.NOFOLLOW_SYMLINKS + ); + + info = null; + while (((info = enumerator.next_file()) != null)) { + @delete(parent.get_child(info.get_name())); + } + } + + parent.delete(); + } + +} diff --git a/test/engine/api/geary-account-information-test.vala b/test/engine/api/geary-account-information-test.vala index 7cab7bd4..5215b8de 100644 --- a/test/engine/api/geary-account-information-test.vala +++ b/test/engine/api/geary-account-information-test.vala @@ -15,7 +15,10 @@ class Geary.AccountInformationTest : TestCase { public void has_email_address() throws GLib.Error { AccountInformation test = new AccountInformation( - "test", new MockServiceInformation(), new MockServiceInformation() + "test", + ServiceProvider.OTHER, + new MockServiceInformation(), + new MockServiceInformation() ); test.primary_mailbox = (new RFC822.MailboxAddress(null, "test1@example.com")); diff --git a/test/engine/api/geary-engine-test.vala b/test/engine/api/geary-engine-test.vala index ae2c9c35..8a6f4bbd 100644 --- a/test/engine/api/geary-engine-test.vala +++ b/test/engine/api/geary-engine-test.vala @@ -18,8 +18,6 @@ class Geary.EngineTest : TestCase { add_test("add_account", add_account); add_test("remove_account", remove_account); add_test("re_add_account", re_add_account); - add_test("create_orphan_account_with_legacy", create_orphan_account_with_legacy); - add_test("create_orphan_account", create_orphan_account); } ~EngineTest() { @@ -65,7 +63,9 @@ class Geary.EngineTest : TestCase { } public void add_account() throws GLib.Error { - AccountInformation info = this.engine.create_orphan_account( + AccountInformation info = new AccountInformation( + "test", + ServiceProvider.OTHER, new MockServiceInformation(), new MockServiceInformation() ); @@ -83,7 +83,9 @@ class Geary.EngineTest : TestCase { } public void remove_account() throws GLib.Error { - AccountInformation info = this.engine.create_orphan_account( + AccountInformation info = new AccountInformation( + "test", + ServiceProvider.OTHER, new MockServiceInformation(), new MockServiceInformation() ); @@ -98,7 +100,9 @@ class Geary.EngineTest : TestCase { } public void re_add_account() throws GLib.Error { - AccountInformation info = this.engine.create_orphan_account( + AccountInformation info = new AccountInformation( + "test", + ServiceProvider.OTHER, new MockServiceInformation(), new MockServiceInformation() ); @@ -111,78 +115,7 @@ class Geary.EngineTest : TestCase { assert_true(this.engine.has_account(info.id)); } - public void create_orphan_account() throws Error { - try { - AccountInformation info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_01"); - this.engine.add_account(info); - - info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_02"); - this.engine.add_account(info); - - info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_03"); - this.engine.add_account(info); - - info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_04"); - } catch (Error err) { - print("\nerr: %s\n", err.message); - assert_not_reached(); - } - } - - public void create_orphan_account_with_legacy() throws Error { - this.engine.add_account( - new AccountInformation( - "foo", - new MockServiceInformation(), - new MockServiceInformation() - ) - ); - - AccountInformation info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_01"); - this.engine.add_account(info); - - info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_02"); - - this.engine.add_account( - new AccountInformation( - "bar", - new MockServiceInformation(), - new MockServiceInformation() - ) - ); - - info = this.engine.create_orphan_account( - new MockServiceInformation(), - new MockServiceInformation() - ); - assert(info.id == "account_02"); - } - - private void delete(File parent) throws Error { + private void delete(File parent) throws Error { FileInfo info = parent.query_info( "standard::*", FileQueryInfoFlags.NOFOLLOW_SYMLINKS diff --git a/test/engine/app/app-conversation-monitor-test.vala b/test/engine/app/app-conversation-monitor-test.vala index 285a62b4..a641a8fb 100644 --- a/test/engine/app/app-conversation-monitor-test.vala +++ b/test/engine/app/app-conversation-monitor-test.vala @@ -30,6 +30,7 @@ class Geary.App.ConversationMonitorTest : TestCase { public override void set_up() { this.account_info = new AccountInformation( "account_01", + ServiceProvider.OTHER, new MockServiceInformation(), new MockServiceInformation() ); diff --git a/test/engine/imap-engine/account-processor-test.vala b/test/engine/imap-engine/account-processor-test.vala index ebe53b79..d89483de 100644 --- a/test/engine/imap-engine/account-processor-test.vala +++ b/test/engine/imap-engine/account-processor-test.vala @@ -71,6 +71,7 @@ public class Geary.ImapEngine.AccountProcessorTest : TestCase { public override void set_up() { this.info = new Geary.AccountInformation( "test-info", + ServiceProvider.OTHER, new MockServiceInformation(), new MockServiceInformation() ); diff --git a/test/meson.build b/test/meson.build index 18f91d9e..bea158a1 100644 --- a/test/meson.build +++ b/test/meson.build @@ -61,6 +61,15 @@ geary_test_engine_sources = [ geary_test_client_sources = [ 'test-client.vala', + # These should be included in the test lib sources, but we can't + # since that would make the test lib depend on geary-engine.vapi, + # and the engine test sute needs to depend + # geary-engine_internal.vapi, which leads to duplicate symbols when + # linking + 'engine/api/geary-credentials-mediator-mock.vala', + 'engine/api/geary-service-information-mock.vala', + + 'client/accounts/account-manager-test.vala', 'client/application/geary-configuration-test.vala', 'client/components/client-web-view-test.vala', 'client/components/client-web-view-test-case.vala', diff --git a/test/test-client.vala b/test/test-client.vala index 06352e89..31aa1974 100644 --- a/test/test-client.vala +++ b/test/test-client.vala @@ -39,6 +39,7 @@ int main(string[] args) { // Keep this before other ClientWebView based tests since it tests // WebContext init + client.add_suite(new AccountManagerTest().get_suite()); client.add_suite(new ClientWebViewTest().get_suite()); client.add_suite(new ComposerWebViewTest().get_suite()); client.add_suite(new ConfigurationTest().get_suite());