Merge branch 'uiux' into adding-unittests

lib/GADS/API.pm

@@ -711,7 +711,6 @@ sub _post_add_user_account
         firstname             => $body->{firstname},
         surname               => $body->{surname},
         email                 => $body->{email},
-        username              => $body->{email},
         freetext1             => $body->{freetext1},
         freetext2             => $body->{freetext2},
         title                 => $body->{title},

lib/GADS/Schema/Result/User.pm

@@ -30,7 +30,7 @@ extends 'DBIx::Class::Core';
 
 =cut
 
-__PACKAGE__->load_components("InflateColumn::DateTime");
+__PACKAGE__->load_components("InflateColumn::DateTime", "+GADS::DBIC");
 
 =head1 TABLE: C<user>
 
@@ -776,7 +776,8 @@ sub graphs
 
 # Used to check if a user has a group
 has has_group => (
-    is => 'lazy',
+    is      => 'lazy',
+    clearer => 1,
 );
 
 sub _build_has_group
@@ -845,19 +846,6 @@ sub update_user
     delete $params{team_id} if !$params{team_id};
     delete $params{title} if !$params{title};
 
-    length $params{firstname} <= 128
-        or error __"Forename must be less than 128 characters";
-    length $params{surname} <= 128
-        or error __"Surname must be less than 128 characters";
-    !defined $params{organisation} || $params{organisation} =~ /^[0-9]+$/
-        or error __x"Invalid organisation {id}", id => $params{organisation};
-    !defined $params{department_id} || $params{department_id} =~ /^[0-9]+$/
-        or error __x"Invalid department {id}", id => $params{department_id};
-    !defined $params{team_id} || $params{team_id} =~ /^[0-9]+$/
-        or error __x"Invalid team {id}", id => $params{team_id};
-    GADS::Util->email_valid($params{email})
-        or error __x"The email address \"{email}\" is invalid", email => $params{email};
-
     my $site = $self->result_source->schema->resultset('Site')->next;
 
     error __x"Please select a {name} for the user", name => $site->organisation_name
@@ -877,6 +865,8 @@ sub update_user
         $values->{account_request} = $params{account_request};
     }
 
+    my $original_username = $self->username;
+
     foreach my $field ($site->user_fields)
     {
         next if !exists $params{$field->{name}};
@@ -887,14 +877,8 @@ sub update_user
 
     my $audit = GADS::Audit->new(schema => $self->result_source->schema, user => $current_user);
 
-    if (lc $values->{username} ne lc $self->username)
-    {
-        $self->result_source->schema->resultset('User')->active->search({
-            username => $values->{username},
-        })->count
-            and error __x"Email address {username} already exists as an active user", username => $values->{username};
-        $audit->login_change("Username ".$self->username." (id ".$self->id.") being changed to $values->{username}");
-    }
+    $audit->login_change("Username $original_username (id ".$self->id.") being changed to ".$self->username)
+        if $original_username && $self->is_column_changed('username');
 
     # Coerce view_limits to value expected, ensure all removed if exists
     $params{view_limits} = []
@@ -906,12 +890,15 @@ sub update_user
     $params{permissions} = []
         if exists $params{permissions} && !$params{permissions};
 
-    $params{value} = _user_value(\%params);
-    $values->{value} = _user_value($values);
     $self->update($values);
 
-    $self->groups($current_user, $params{groups})
-        if $params{groups};
+    if ($params{groups})
+    {
+        $self->groups($current_user, $params{groups});
+        $self->clear_has_group;
+        $self->has_group;
+    }
+
     if ($params{permissions} && ref $params{permissions} eq 'ARRAY')
     {
         error __"You do not have permission to set global user permissions"
@@ -921,6 +908,36 @@ sub update_user
     $self->set_view_limits($params{view_limits})
         if $params{view_limits};
 
+    my $empty = 1;
+    $empty = 0 if($params{organisation});
+
+    my $required = 0;
+    $required = 1 if $site->register_organisation_mandatory;
+    $required = 0 if $params{edit_own_user};
+    $required = 1 if $params{$site->user_field_is_editable('organisation')};
+
+    error __x"Please select a {name} for the user", name => $site->organisation_name
+        if $empty && $required;
+
+    error __x"Please select a {name} for the user", name => $site->team_name
+        if !$params{team_id} && $site->register_team_mandatory;
+
+    error __x"Please select a {name} for the user", name => $site->department_name
+        if !$params{department_id} && $site->register_department_mandatory;
+
+    length $params{firstname} <= 128
+        or error __"Forename must be less than 128 characters";
+    length $params{surname} <= 128
+        or error __"Surname must be less than 128 characters";
+    !defined $params{organisation} || $params{organisation} =~ /^[0-9]+$/
+        or error __x"Invalid organisation {id}", id => $params{organisation};
+    !defined $params{department_id} || $params{department_id} =~ /^[0-9]+$/
+        or error __x"Invalid department {id}", id => $params{department_id};
+    !defined $params{team_id} || $params{team_id} =~ /^[0-9]+$/
+        or error __x"Invalid team {id}", id => $params{team_id};
+    GADS::Util->email_valid($params{email})
+        or error __x"The email address \"{email}\" is invalid", email => $params{email};
+
     my $msg = __x"User updated: ID {id}, username: {username}",
         id => $self->id, username => $params{username};
     $msg .= __x", groups: {groups}", groups => join ', ', @{$params{groups}}
@@ -1117,6 +1134,30 @@ sub for_data_table
     $return;
 }
 
+sub validate
+{   my $self = shift;
+    # Update value field
+    $self->value(_user_value({firstname => $self->firstname, surname => $self->surname}));
+
+    $self->username
+        or error "Username required";
+    $self->email
+        or error "Email required";
+
+    # Check existing user rename, check both email address and username
+    foreach my $f (qw/username email/)
+    {
+        if ($self->is_column_changed($f) || !$self->id)
+        {
+            my $search = { $f => $self->$f };
+            $search->{id} = { '!=' => $self->id }
+                if $self->id;
+            $self->result_source->resultset->active->search($search)->next
+                and error __x"{username} already exists as an active user", username => $self->$f;
+        }
+    }
+}
+
 sub export_hash
 {   my $self = shift;
     # XXX Department, organisation etc not currently exported

lib/GADS/Schema/ResultSet/User.pm

@@ -47,6 +47,8 @@ sub create_user
 
     error __"An email address must be specified for the user"
         if !$params{email};
+    panic "username is no longer accepted for create_user - use email instead"
+        if $params{username};
 
     error __x"User {email} already exists", email => $params{email}
         if $self->active(email => $params{email})->count;
@@ -55,7 +57,8 @@ sub create_user
     my $request_base = $params{request_base};
 
     my $user = $self->create({
-        username => $params{username},
+        email    => $params{email},
+        username => $params{email},
         resetpw  => $code,
         created  => DateTime->now,
     });
@@ -207,7 +210,6 @@ sub upload
             firstname             => defined $user_mapping{forename} ? $row->[$user_mapping{forename}] : '',
             surname               => defined $user_mapping{surname} ? $row->[$user_mapping{surname}] : '',
             email                 => defined $user_mapping{email} ? $row->[$user_mapping{email}] : '',
-            username              => defined $user_mapping{email} ? $row->[$user_mapping{email}] : '',
             freetext1             => defined $user_mapping{$freetext1} ? $row->[$user_mapping{$freetext1}] : '',
             freetext2             => defined $user_mapping{$freetext2} ? $row->[$user_mapping{$freetext2}] : '',
             title                 => $title_id,

t/010_permissions.t

@@ -358,7 +358,6 @@ foreach my $test (qw/single all/)
         try {
             $user = $schema->resultset('User')->create_user(
                 current_user     => $sheet->$usertype,
-                username         => "$usertype\@example.com",
                 email            => "$usertype\@example.com",
                 firstname        => 'Joe',
                 surname          => 'Bloggs',

t/014_import.t

@@ -21,12 +21,14 @@ $ENV{GADS_NO_FORK} = 1; # Prevent forking during import process
     $sheet->create_records;
 
     my $user1 = $schema->resultset('User')->create({
-        username => 'test',
+        email    => '[email protected]',
+        username => '[email protected]',
         password => 'test',
     });
 
     my $user2 = $schema->resultset('User')->create({
-        username => 'test2',
+        email    => '[email protected]',
+        username => '[email protected]',
         password => 'test2',
     });
 
@@ -177,7 +179,8 @@ foreach my $test (@tests)
         $sheet->create_records;
 
         my $user = $schema->resultset('User')->create({
-            username => 'test',
+            email    => '[email protected]',
+            username => '[email protected]',
             password => 'test',
         });
 

t/024_user.t

@@ -18,7 +18,6 @@ my %template = (
     surname          => 'Bloggs',
     firstname        => 'Joe',
     email            => '[email protected]',
-    username         => '[email protected]',
 );
 
 my $user = $schema->resultset('User')->create_user(%template);
@@ -29,6 +28,20 @@ my $u = $schema->resultset('User')->find($user_id);
 
 is($u->value, "Bloggs, Joe", "User created successfully");
 
+# Check cannot rename to existing user
+my $existing = $schema->resultset('User')->next->username;
+ok($existing ne $u->username, "Testing username different to that of test");
+try { $u->update({ email => $existing }) };
+like($@, qr/already exists/, "Unable to rename user to existing username");
+
+# Check cannot create same username as existing
+try { $schema->resultset('User')->create_user(%template, email => $existing) };
+like($@, qr/already exists/, "Unable to create user with existing username");
+
+# Same directly in resultset
+try { $schema->resultset('User')->create({email => $existing, username => $existing}) };
+like($@, qr/already exists/, "Unable to create user with existing username");
+
 $site->update({ register_organisation_mandatory => 1 });
 try { $schema->resultset('User')->create_user(%template, email => '[email protected]') };
 like($@, qr/Please select a Organisation/, "Failed to create user missing org");

t/lib/Test/GADS/DataSheet.pm

@@ -261,9 +261,11 @@ sub create_user
     my $instance_id = $options{instance_id} || $self->instance_id;
     my $user_id     = $options{user_id};
 
+    # messy - username and email are madatory when creating user object
+    my $temp = '[email protected]';
     my $user = $user_id
-        ? $self->schema->resultset('User')->find_or_create({ id => $user_id })
-        : $self->schema->resultset('User')->create({});
+        ? $self->schema->resultset('User')->find_or_create({ id => $user_id, username => $temp, email => $temp })
+        : $self->schema->resultset('User')->create({ username => $temp, email => $temp });
     $user_id ||= $user->id;
     $user->update({
         username      => "user$user_id\@example.com",

views/data_table.tt

@@ -4,7 +4,7 @@
   <div class="content-block__info col-lg-6"></div>
 
   <div class="content-block__aside col-lg-6">
-    [% IF layout.show_add_record %]
+    [% IF layout.show_add_record AND layout.user_can('write_new') %]
       <a href="[% url.page %]/[% layout.identifier %]/record/" class="btn btn-add">
         [% IF user.has_draft(layout.instance_id) %]
           Continue draft record