Merge lp:~iahmad/unity-greeter/duplicate-entry-bug1092425 into lp:unity-greeter

[I Ahmad]

Fixed the comments form David and Robert, Also Added a unit test case to test the defect fix as well as fixed the existing remote login unit test cases broken due to the duplicate entry fix. Please see the results below

UBUNTU_MENUPROXY=0 top_srcdir=.. . xvfb-run ./unity-greeter-test
/Simple Navigation: OK
/Remote Login: OK
/Remote Login duplicate entries: OK
/Remote Login with Servers Updated signal: OK
/Remote Login with Servers Updated signal and not in remote server: OK
/Remote Login with Login Servers Updated signal: OK
/Remote Login with Login Servers Updated signal and not in removed server: OK
/Remote Login with Remote Login Changed signal: OK
/Remote Login with Remote Login Changed signal and not in changed server: OK
/Remote Login authentication: OK
/Remote Login cancel authentication: OK
/Email Autocomplete: OK
/Greeter Communication: OK
/Unsupported server type: OK

[David Barth (dbarth)]

The code looks fine, but i'd prefer to have a unit test to verify that the bug is indeed gone.

The foreach loop could also be factorized under a new function with a name explaining that it generates a unique id for the user + url combination.

[Robert Ancell (robert-ancell)]

You have tab characters instead of four spaces in the branch.

review: Needs Fixing

[David Barth (dbarth)]

The code looks good to me now.

Approving, as much as remote login is concerned.

review: Approve

[Robert Ancell (robert-ancell)]

Pushed with some coding style fixes

review: Approve

=== modified file 'src/user-list.vala'
--- src/user-list.vala	2012-10-02 17:20:34 +0000
+++ src/user-list.vala	2013-02-11 07:47:20 +0000
@@ -172,15 +172,35 @@
         return "*remote_directory*" + remote_server.url;
     }
 
+    private string username_from_remote_server_fields(RemoteServer remote_server)
+    {
+        var username = "";
+        foreach (var f in remote_server.fields)
+        {
+	    if (f.type == "username" && f.default_value != null)
+            {
+                username = f.default_value.get_string();
+                break;
+            }
+        }
+        return username;
+     }
+	
     private string user_list_name_for_remote_login_server (RemoteServer remote_server)
     {
-        return "*remote_login*" + remote_server.url;
+        var username = username_from_remote_server_fields(remote_server);		
+        return "*remote_login*" + remote_server.url+"*"+username;
     }
 
     private string url_from_remote_loding_server_list_name (string remote_server_list_name)
     {
         return remote_server_list_name.split ("*")[2];
     }
+    
+    private string username_from_remote_loding_server_list_name (string remote_server_list_name)
+    {		
+        return remote_server_list_name.split ("*")[3];
+    }
 
     private void set_remote_directory_servers (RemoteServer[] server_list)
     {
@@ -398,6 +418,11 @@
                         login_success = true;
                         Timeout.add (5000, () => { remote_login_changed (currently_browsing_server_url, currently_browsing_server_email); return false; });
                     }
+                    else if (password_field.text == "duplicate")
+                    {						
+                        test_fill_remote_login_servers_duplicate_entries (out server_list);
+                        login_success = true;                        
+                    }
                 }
                 else
                 {
@@ -589,9 +614,12 @@
     {
         current_remote_fields = new HashTable<string, Gtk.Widget> (str_hash, str_equal);
         var url = url_from_remote_loding_server_list_name (selected_entry.id);
+        var username = username_from_remote_loding_server_list_name(selected_entry.id);
+        
         foreach (var remote_server in server_list)
         {
-            if (remote_server.url == url)
+            var remote_username = username_from_remote_server_fields(remote_server);					
+            if (remote_server.url == url && (username == null || username == remote_username))
             {
                 if (selected_entry.id.has_prefix ("*remote_login"))
                 {
@@ -1236,6 +1264,50 @@
         server_list[3] = remote_server4;
     }
 
+    private void test_fill_remote_login_servers_duplicate_entries (out RemoteServer[] server_list)
+    {		
+        //Create two remote servers with same url but different username and domain.
+        server_list = {};
+
+        RemoteServer remote_server2 = RemoteServer ();
+        remote_server2.type = "rdp";
+        remote_server2.name = "RDP server with default username, and editable domain";
+        remote_server2.url = "http://rdpdefaultusername.com";
+        remote_server2.last_used_server = false;
+        remote_server2.fields = {};
+        RemoteServerField field21 = RemoteServerField ();
+        field21.type = "username";
+        field21.default_value = new Variant.string ("alowl1");
+        RemoteServerField field22 = RemoteServerField ();
+        field22.type = "password";
+        field22.default_value = new Variant.string ("duplicate1");
+        RemoteServerField field23 = RemoteServerField ();
+        field23.type = "domain";
+        field23.default_value = new Variant.string ("SCANNERS");
+        remote_server2.fields = {field21, field22, field23};
+
+        RemoteServer remote_server5 = RemoteServer ();
+        remote_server5.type = "rdp";
+        remote_server5.name = "RDP server with default username, and editable domain";
+        remote_server5.url = "http://rdpdefaultusername.com";
+        remote_server5.last_used_server = false;
+        remote_server5.fields = {};
+        RemoteServerField field51 = RemoteServerField ();
+        field51.type = "username";
+        field51.default_value = new Variant.string ("alowl2");
+        RemoteServerField field52 = RemoteServerField ();
+        field52.type = "password";
+        field52.default_value = new Variant.string ("duplicate2");
+        RemoteServerField field53 = RemoteServerField ();
+        field53.type = "domain";
+        field53.default_value = new Variant.string ("PRINTERS");
+        remote_server5.fields = {field51, field52, field53};
+
+        server_list.resize (2);
+        server_list[0] = remote_server2;
+        server_list[1] = remote_server5;
+    }
+    
     private bool test_key_press_cb (Gdk.EventKey event)
     {
         if ((event.state & Gdk.CONTROL_MASK) == 0)

=== modified file 'tests/test.vala'
--- tests/test.vala	2013-01-04 15:28:54 +0000
+++ tests/test.vala	2013-02-11 07:47:20 +0000
@@ -183,7 +183,7 @@
         pwd.text = "password";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
         wait_for_scrolling_end (list);
 
         // Go back to the remote_directory entry and write the same password but an invalid email
@@ -215,7 +215,7 @@
         pwd.text = "delay1";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
 
         bool done = false;
         // The delay1 code triggers at 5 seconds
@@ -253,7 +253,7 @@
         pwd.text = "delay1";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
         wait_for_scrolling_end (list);
 
         while (list.selected_entry.id.has_prefix("*remote_"))
@@ -299,7 +299,7 @@
         pwd.text = "delay2";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
 
         bool done = false;
         // The delay2 code triggers at 5 seconds
@@ -307,7 +307,7 @@
             {
                 // If the login server we were disappears we get moved to a different one
                 wait_for_scrolling_end (list);
-                GLib.assert (list.selected_entry.id == "*remote_login*http://megacoolrdpserver.com");
+                GLib.assert (list.selected_entry.id == "*remote_login*http://megacoolrdpserver.com*");
                 done = true;
                 return false;
             }
@@ -336,10 +336,10 @@
         pwd.text = "delay2";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
 
         // Move to a server that won't be removed
-        while (list.selected_entry.id != "*remote_login*http://coolrdpserver.com")
+        while (list.selected_entry.id != "*remote_login*http://coolrdpserver.com*")
             do_scroll (list, GreeterList.ScrollTarget.UP);
 
         bool done = false;
@@ -348,7 +348,7 @@
             {
                 // If the login server we were did not disappear we are still in the same one
                 wait_for_scrolling_end (list);
-                GLib.assert (list.selected_entry.id == "*remote_login*http://coolrdpserver.com");
+                GLib.assert (list.selected_entry.id == "*remote_login*http://coolrdpserver.com*");
                 done = true;
                 return false;
             }
@@ -377,7 +377,7 @@
         pwd.text = "delay3";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
 
         bool done = false;
         // The delay3 code triggers at 5 seconds
@@ -419,7 +419,7 @@
         pwd.text = "delay3";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
         wait_for_scrolling_end (list);
 
         while (list.selected_entry.id.has_prefix("*remote_"))
@@ -473,7 +473,7 @@
         pwd.text = "password";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
         wait_for_scrolling_end (list);
 
         UnityGreeter.singleton.session_started = false;
@@ -503,7 +503,7 @@
         pwd.text = "password";
         list.selected_entry.respond ({});
         GLib.assert (!list.selected_entry.has_errors);
-        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com");
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername2.com*lwola");
         wait_for_scrolling_end (list);
 
         UnityGreeter.singleton.session_started = false;
@@ -520,6 +520,49 @@
         mw.hide ();
     }
 
+    public static void remote_login_duplicate_entries()
+    {
+        MainWindow mw = setup ();
+        TestList list = mw.stack.top () as TestList;
+
+        scroll_to_remote_login (list); //Wait until remote login appears.
+        GLib.assert (list.selected_entry.id == "*remote_directory*http://crazyurl.com");
+        GLib.assert (!list.selected_entry.has_errors);
+
+        // If we answer without filling in any field -> error
+        list.selected_entry.respond ({});
+        GLib.assert (list.selected_entry.has_errors);
+
+        // Go to first and back to last to clear the error
+        do_scroll (list, GreeterList.ScrollTarget.START);
+        do_scroll (list, GreeterList.ScrollTarget.END);
+        GLib.assert (!list.selected_entry.has_errors);
+
+        // Fill in a valid email and password
+        // Check there is no error and we moved to the last logged in server
+        var email = remote_directory_entry_email_field (list);
+        var pwd = remote_directory_entry_password_field (list);
+        email.text = "[e-mail]";
+        pwd.text = "duplicate";
+        list.selected_entry.respond ({});
+        GLib.assert (!list.selected_entry.has_errors);        
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername.com*alowl2");        
+        
+        var username = remote_login_entry_username_field(list);
+        var domain = remote_login_entry_domain_field(list);
+        var password = remote_login_entry_password_field(list);		
+        GLib.assert (username.text == "alowl2" && domain.text == "PRINTERS" && password.text == "duplicate2");
+                        
+        do_scroll (list, GreeterList.ScrollTarget.DOWN);
+        GLib.assert (list.selected_entry.id == "*remote_login*http://rdpdefaultusername.com*alowl1");
+        username = remote_login_entry_username_field(list);
+        domain = remote_login_entry_domain_field(list);
+        password = remote_login_entry_password_field(list);
+        GLib.assert (username.text == "alowl1" && domain.text == "SCANNERS" && password.text == "duplicate1");	
+        wait_for_scrolling_end (list);
+        mw.hide ();
+    }
+
     public static void email_autocomplete ()
     {
         MainWindow mw = setup ();
@@ -587,7 +630,7 @@
         list.selected_entry.respond ({});
         wait_for_scrolling_end (list);
 
-        while (list.selected_entry.id != "*remote_login*http://coolrdpserver.com")
+        while (list.selected_entry.id != "*remote_login*http://coolrdpserver.com*")
             do_scroll (list, GreeterList.ScrollTarget.UP);
 
         var domain = remote_login_entry_domain_field (list);
@@ -626,7 +669,7 @@
         list.selected_entry.respond ({});
         wait_for_scrolling_end (list);
 
-        while (list.selected_entry.id != "*remote_login*http://notsupportedserver.com")
+        while (list.selected_entry.id != "*remote_login*http://notsupportedserver.com*")
             do_scroll (list, GreeterList.ScrollTarget.UP);
 
         GLib.assert (list.selected_entry.has_errors);
@@ -675,6 +718,7 @@
 
         GLib.Test.add_func ("/Simple Navigation", simple_navigation);
         GLib.Test.add_func ("/Remote Login", remote_login);
+        GLib.Test.add_func ("/Remote Login duplicate entries", remote_login_duplicate_entries);
         GLib.Test.add_func ("/Remote Login with Servers Updated signal", remote_login_servers_updated_signal);
         GLib.Test.add_func ("/Remote Login with Servers Updated signal and not in remote server", remote_login_servers_updated_signal_focus_not_in_remote_server);
         GLib.Test.add_func ("/Remote Login with Login Servers Updated signal", remote_login_login_servers_updated_signal);