From b93762fbc5b2cf12844390430a105ed3359a686f Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Thu, 19 Oct 2023 22:34:54 +0100 Subject: [PATCH 1/6] Fix failing test --- lib/GADS/Schema/Result/User.pm | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/GADS/Schema/Result/User.pm b/lib/GADS/Schema/Result/User.pm index 41ebef520..5419ef160 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/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 @@ -907,8 +907,6 @@ 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}) @@ -1140,6 +1138,11 @@ sub for_data_table $return; } +sub validate +{ my $self = shift; + $self->value(_user_value({firstname => $self->firstname, surname => $self->surname})); +} + sub export_hash { my $self = shift; # XXX Department, organisation etc not currently exported From 3ffdd4b0cc7c3b3057504e3312be46b06e06c2f4 Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Fri, 20 Oct 2023 11:48:33 +0100 Subject: [PATCH 2/6] Tidy up username creation/deletion and add tests This commit removes username as a parameter for creating and editing users, using just email instead for consistency. It also moves validation checks to the validate() function which is always called, in case the result is updated directly. Tests also added. --- lib/GADS/API.pm | 1 - lib/GADS/Schema/Result/User.pm | 26 ++++++++++++++++++-------- lib/GADS/Schema/ResultSet/User.pm | 3 ++- t/010_permissions.t | 1 - t/024_user.t | 15 ++++++++++++++- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/lib/GADS/API.pm b/lib/GADS/API.pm index 7b9a00be4..13976b007 100644 --- a/lib/GADS/API.pm +++ b/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}, diff --git a/lib/GADS/Schema/Result/User.pm b/lib/GADS/Schema/Result/User.pm index 5419ef160..90e007cd0 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/lib/GADS/Schema/Result/User.pm @@ -877,6 +877,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}}; @@ -888,14 +890,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} = [] @@ -1140,7 +1136,21 @@ sub for_data_table sub validate { my $self = shift; + # Update value field $self->value(_user_value({firstname => $self->firstname, surname => $self->surname})); + + # 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 diff --git a/lib/GADS/Schema/ResultSet/User.pm b/lib/GADS/Schema/ResultSet/User.pm index 57d813988..ec5f9c4df 100644 --- a/lib/GADS/Schema/ResultSet/User.pm +++ b/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; @@ -207,7 +209,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, diff --git a/t/010_permissions.t b/t/010_permissions.t index ddf5ebeb3..31aa17a34 100644 --- a/t/010_permissions.t +++ b/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', diff --git a/t/024_user.t b/t/024_user.t index a4a93a20f..977108b50 100644 --- a/t/024_user.t +++ b/t/024_user.t @@ -18,7 +18,6 @@ my %template = ( surname => 'Bloggs', firstname => 'Joe', email => 'joe@example.com', - username => 'joe@example.com', ); 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}) }; +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 => 'joe1@example.com') }; like($@, qr/Please select a Organisation/, "Failed to create user missing org"); From 201ddbc96161a34bf5d673df6d7621029ce164ea Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Fri, 20 Oct 2023 12:18:40 +0100 Subject: [PATCH 3/6] Fix failing test --- lib/GADS/Schema/Result/User.pm | 5 +++++ t/014_import.t | 9 ++++++--- t/lib/Test/GADS/DataSheet.pm | 6 ++++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/GADS/Schema/Result/User.pm b/lib/GADS/Schema/Result/User.pm index 90e007cd0..73ad96998 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/lib/GADS/Schema/Result/User.pm @@ -1139,6 +1139,11 @@ sub validate # 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/) { diff --git a/t/014_import.t b/t/014_import.t index 152e94406..34fd26b6a 100644 --- a/t/014_import.t +++ b/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 => 'test@example.com', + username => 'test@example.com', password => 'test', }); my $user2 = $schema->resultset('User')->create({ - username => 'test2', + email => 'test2@example.com', + username => 'test2@example.com', password => 'test2', }); @@ -177,7 +179,8 @@ foreach my $test (@tests) $sheet->create_records; my $user = $schema->resultset('User')->create({ - username => 'test', + email => 'test@example.com', + username => 'test@example.com', password => 'test', }); diff --git a/t/lib/Test/GADS/DataSheet.pm b/t/lib/Test/GADS/DataSheet.pm index ba2bfbc11..6175b16e2 100644 --- a/t/lib/Test/GADS/DataSheet.pm +++ b/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 = 'TEMPUSER@EXAMPLE.COM'; 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", From 2810c6ec03557a74a619caec69d514428b52f1cd Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Fri, 20 Oct 2023 15:41:33 +0100 Subject: [PATCH 4/6] Fix failing test --- lib/GADS/Schema/ResultSet/User.pm | 3 ++- t/024_user.t | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/GADS/Schema/ResultSet/User.pm b/lib/GADS/Schema/ResultSet/User.pm index ec5f9c4df..f8e3bc277 100644 --- a/lib/GADS/Schema/ResultSet/User.pm +++ b/lib/GADS/Schema/ResultSet/User.pm @@ -57,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, }); diff --git a/t/024_user.t b/t/024_user.t index 977108b50..79fad197f 100644 --- a/t/024_user.t +++ b/t/024_user.t @@ -39,7 +39,7 @@ 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}) }; +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 }); From c950d0c93770cae179bc6d6a245d17ac873a79c8 Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Fri, 20 Oct 2023 18:05:09 +0100 Subject: [PATCH 5/6] Only show add record button with permission --- views/data_table.tt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/data_table.tt b/views/data_table.tt index e85ac4209..49711b2c9 100755 --- a/views/data_table.tt +++ b/views/data_table.tt @@ -4,7 +4,7 @@
- [% IF layout.show_add_record %] + [% IF layout.show_add_record AND layout.user_can('write_new') %] [% IF user.has_draft(layout.instance_id) %] Continue draft record From 865c7dbdbaa226d3a29fbe81892134d50d77fd76 Mon Sep 17 00:00:00 2001 From: Andy Beverley Date: Fri, 20 Oct 2023 18:10:42 +0100 Subject: [PATCH 6/6] Fix clearing groups input field after user submit error --- lib/GADS/Schema/Result/User.pm | 38 ++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/GADS/Schema/Result/User.pm b/lib/GADS/Schema/Result/User.pm index 73ad96998..25274214b 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/lib/GADS/Schema/Result/User.pm @@ -785,7 +785,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 @@ -854,19 +855,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; my $values = { @@ -905,8 +893,13 @@ sub update_user $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" @@ -938,6 +931,19 @@ sub update_user 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}}