diff --git a/lib/GADS/Alert.pm b/lib/GADS/Alert.pm index 39792eff2..2e458d880 100644 --- a/lib/GADS/Alert.pm +++ b/lib/GADS/Alert.pm @@ -133,6 +133,10 @@ sub _build_view $views->view($self->view_id); } +# Update the stored cached values for this alert. For views that are common +# across multiple users (the view does not have a CURUSER filter and the user +# does not have a restricted view) then this cache will be shared across all +# users with the same alert. Otherwise, user-specific values are stored. sub update_cache { my ($self, %options) = @_; @@ -148,33 +152,49 @@ sub update_cache return; } - # If the view contains a CURUSER filter, we need separate - # alert caches for each user that has this alert. Only need - # to worry about this if all_users flag is set - my @users = $view->has_curuser && $options{all_users} + # If the view contains a CURUSER filter or the user of the alert has a + # limited view, then we need separate alert caches for each user that has + # this alert. + # If the all_users flag is set we need to consider this for all users. + my @users_search = $options{all_users} ? map $_->user, $self->schema->resultset('Alert')->search({ view_id => $view->id }, { prefetch => 'user' - })->all : ($self->user); + })->all + : ($self->user); - foreach my $user (@users) + my @users; + if ($view->has_curuser) { - my $u = $view->has_curuser && $user; + # View has CURUSER filter - always need user-specific cache + @users = @users_search; + } + elsif (grep $_->has_view_limit($self->layout->instance_id), @users_search) + { + # Need user specific cache if any user of the alter is view-limited + @users = @users_search; + } + else { + # Normal cache + @users = (undef); + } + foreach my $user (@users) + { my $records = GADS::Records->new( # We generally want to generate a view's alert_cache without user # defined permissions limiting it, otherwise multiple users on a # single global view will have different things to alert on. # The only exception is when the view has a CURUSER, in which # case we generate them individually. - user => $u, + user => $user, layout => $self->layout, schema => $self->schema, view => $view, ); - my $user_id = $view->has_curuser ? $u->id : undef; + my $user_id = $view->has_curuser ? $user->id : undef; my %exists; # For each item in this view, see if it exists in the cache. If it doesn't, diff --git a/lib/GADS/AlertSend.pm b/lib/GADS/AlertSend.pm index 400cdac27..9c2591a25 100644 --- a/lib/GADS/AlertSend.pm +++ b/lib/GADS/AlertSend.pm @@ -168,13 +168,32 @@ sub _process_instance my @foundin; foreach my $view (@views) { - my $records = GADS::Records->new( - columns => [], # Otherwise all columns retrieved during search construct - schema => $self->schema, - layout => $layout, - user => undef, # Alerts should not be affected by current user - ); - push @foundin, $records->search_view($current_ids, $view); + # See if we need user-specific view searches. This is if the view + # has a CURSER filter or if the alert's user has a restricted view. + my $has_unrestricted_user; + my @users; + foreach my $alert (@{$view->all_alerts}) + { + if ($alert->user->has_view_limit($layout->instance_id) || $view->has_curuser) + { + push @users, $alert->user; + } + else { + # Alerts should not be affected by current user + $has_unrestricted_user = 1; + } + } + push @users, undef if $has_unrestricted_user; + foreach my $user (@users) + { + my $records = GADS::Records->new( + columns => [], # Otherwise all columns retrieved during search construct + schema => $self->schema, + layout => $layout, + user => $user, + ); + push @foundin, $records->search_view($current_ids, $view); + } } foreach my $now_in (@foundin) { diff --git a/lib/GADS/Records.pm b/lib/GADS/Records.pm index 944ba122f..51e0cca20 100644 --- a/lib/GADS/Records.pm +++ b/lib/GADS/Records.pm @@ -621,7 +621,7 @@ sub linked_hash # A function to see if any views have a particular record within sub search_view -{ my ($self, $current_ids, $view) = @_; +{ my ($self, $current_ids, $view, $user) = @_; return unless $view && @$current_ids; @@ -634,79 +634,70 @@ sub search_view $self->columns([]); my @foundin; - # Treat each view with CURUSER as a separate view for each user - # that has it set as an alert - my @users = $view->has_curuser - ? $self->schema->resultset('User')->search({ - view_id => $view->id - }, { - join => 'alerts', - })->all : (undef); - - foreach my $user (@users) + + my $filter = $view->filter; + my $view_id = $view->id; + trace qq(About to decode filter for view ID $view_id); + my $decoded = $filter->as_hash; + if (keys %$decoded) { - my $filter = $view->filter; - my $view_id = $view->id; - trace qq(About to decode filter for view ID $view_id); - my $decoded = $filter->as_hash; - if (keys %$decoded) + my @searches = ({ + 'me.instance_id' => $self->layout->instance_id, + }); + # Perform search construct twice, to ensure all value joins are consistent numbers + $self->_search_construct($decoded, + ignore_perms => 1, + user => $user, + current_version_only => 1, + ); + push @searches, $self->_search_construct($decoded, + ignore_perms => 1, + user => $user, + current_version_only => 1, + ); + push @searches, $self->_view_limits_search(current_version_only => 1); + my $i = 0; my @ids; + while ($i < @$current_ids) { - my @searches = ({ - 'me.instance_id' => $self->layout->instance_id, - }); - # Perform search construct twice, to ensure all value joins are consistent numbers - $self->_search_construct($decoded, - ignore_perms => 1, - user => $user, - current_version_only => 1, - ); - push @searches, $self->_search_construct($decoded, - ignore_perms => 1, - user => $user, - current_version_only => 1, - ); - my $i = 0; my @ids; - while ($i < @$current_ids) - { - # See comment above about searching for all current_ids - my $search = { -and => \@searches }; - # At this point we used to see if the number of records in the - # view was equal to the number of current_ids, to try and skip - # the multiple loops if possile. However, it turned out that - # count() is expensive (especially if only one current_id), so - # this has been removed - my $max = $i + 499; - $max = @$current_ids-1 if $max >= @$current_ids; - $search->{'me.id'} = [@{$current_ids}[$i..$max]]; - push @ids, $self->schema->resultset('Current')->search($search, { - join => [ - [$self->linked_hash(search => 1, current_version_only => 1)], - { - 'current_version' => [$self->jpfetch(search => 1)], - }, - ], - })->get_column('id')->all; - last unless $search->{'me.id'}; - $i += 500; - } - foreach my $id (@ids) - { - push @foundin, { - view => $view, - id => $id, - user_id => $user && $user->id, - }; - } + # See comment above about searching for all current_ids + my $search = { -and => \@searches }; + # At this point we used to see if the number of records in the + # view was equal to the number of current_ids, to try and skip + # the multiple loops if possile. However, it turned out that + # count() is expensive (especially if only one current_id), so + # this has been removed + my $max = $i + 499; + $max = @$current_ids-1 if $max >= @$current_ids; + $search->{'me.id'} = [@{$current_ids}[$i..$max]]; + push @ids, $self->schema->resultset('Current')->search($search, { + join => [ + [$self->linked_hash(search => 1, current_version_only => 1)], + { + 'current_version' => [$self->jpfetch(search => 1)], + }, + ], + })->get_column('id')->all; + last unless $search->{'me.id'}; + $i += 500; } - else { - # No filter, definitely in view + foreach my $id (@ids) + { push @foundin, { view => $view, + id => $id, user_id => $user && $user->id, - id => $_, - } foreach @$current_ids; + }; } } + else { + # No filter, definitely in view + push @foundin, { + view => $view, + user_id => $user && $user->id, + id => $_, + } foreach @$current_ids; + } + @foundin; } diff --git a/lib/GADS/Schema/Result/User.pm b/lib/GADS/Schema/Result/User.pm index e019dc847..e5208b805 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/lib/GADS/Schema/Result/User.pm @@ -376,6 +376,24 @@ sub _build_view_limits_with_blank return [undef]; } +# Whether this user has a view limit on a particular table +sub has_view_limit +{ my ($self, $instance_id) = @_; + # Yes if they have a limited view on their user account + return 1 if $self->view_limits->search({ + 'view.instance_id' => $instance_id, + }, { + join => 'view', + })->next; + # Yes if there is an overall one on the table + return 1 if $self->result_source->schema->resultset('Layout')->search({ + instance_id => $instance_id, + view_limit_id => { '!=' => undef }, + })->next; + # Otherwise no + return 0; +} + sub set_view_limits { my ($self, $view_ids) = @_; diff --git a/t/011_alert_limit.t b/t/011_alert_limit.t index 72d0409c2..c87a77e41 100644 --- a/t/011_alert_limit.t +++ b/t/011_alert_limit.t @@ -35,93 +35,190 @@ my $data = [ }, ]; -my $sheet = Test::GADS::DataSheet->new( - data => $data, - user_permission_override => 0, -); - -my $layout = $sheet->layout; -my $columns = $sheet->columns; -my $schema = $sheet->schema; -$sheet->create_records; - -# Create view for limit user's records -my $rules = GADS::Filter->new( - as_hash => { - rules => [{ - id => $columns->{string1}->id, - type => 'string', - value => 'Foo', +# Test for the limited view being on both the overall table and directly on the +# user itself +foreach my $type (qw/table user/) +{ + my $sheet = Test::GADS::DataSheet->new( + data => $data, + user_permission_override => 0, + ); + + my $layout = $sheet->layout; + my $columns = $sheet->columns; + my $schema = $sheet->schema; + $sheet->create_records; + + # Create view for limit user's records + my $rules = GADS::Filter->new( + as_hash => { + rules => [{ + id => $columns->{string1}->id, + type => 'string', + value => 'Foo', + operator => 'equal', + }], + }, + ); + + my $view_limit = GADS::View->new( + name => 'Limit to view', + filter => $rules, + instance_id => $layout->instance_id, + layout => $layout, + schema => $schema, + user => $sheet->user, + is_admin => 1, + ); + $view_limit->write; + + my $user = $sheet->user_normal1; + if ($type eq 'table') # Overall table + { + # Table itself + $layout->view_limit_id($view_limit->id); + $layout->write; + # For the whole table test, apply an override to the limited view for + # the second user, so that they can see everything (to duplicate the + # test for just the user limited view) + my $user_rules = GADS::Filter->new( + as_hash => { + rules => [{ + id => $layout->column_id->id, + type => 'string', + value => '0', + operator => 'greater', + }], + }, + ); + my $user_view = GADS::View->new( + name => 'Limit view for all records', + filter => $user_rules, + instance_id => $layout->instance_id, + layout => $layout, + schema => $schema, + user => $sheet->user, + is_admin => 1, + ); + $user_view->write; + $sheet->user_normal2->set_view_limits([$user_view->id]); + } + else { + # Just the user + $user->set_view_limits([$view_limit->id]); + } + + # Check view limit is working + my $records = GADS::Records->new( + user => $user, + layout => $layout, + schema => $schema, + ); + + is ($records->count, 1, 'Correct number of results when limiting to a view'); + + $rules = { + rules => [{ + id => $columns->{date1}->id, + type => 'date', + value => '2014-10-10', operator => 'equal', }], - }, -); - -my $view_limit = GADS::View->new( - name => 'Limit to view', - filter => $rules, - instance_id => $layout->instance_id, - layout => $layout, - schema => $schema, - user => $sheet->user, - is_admin => 1, -); -$view_limit->write; - -my $user = $sheet->user_normal1; -$user->set_view_limits([$view_limit->id]); - -# Check view limit is working -my $records = GADS::Records->new( - user => $user, - layout => $layout, - schema => $schema, -); - -is ($records->count, 1, 'Correct number of results when limiting to a view'); - -$rules = { - rules => [{ - id => $columns->{date1}->id, - type => 'date', - value => '2014-10-10', - operator => 'equal', - }], -}; - -my $view = GADS::View->new( - name => 'View with alerts', - filter => encode_json($rules), - instance_id => $layout->instance_id, - layout => $layout, - schema => $schema, - columns => [$columns->{string1}->id], -); -$view->write; - -my $alert = GADS::Alert->new( - user => $sheet->user_normal2, - layout => $layout, - schema => $schema, - frequency => 24, - view_id => $view->id, -); -$alert->write; - -is($schema->resultset('AlertCache')->count, 2, "Correct alert cache"); -is($schema->resultset('AlertSend')->count, 0, "Correct alert send"); - -my $record = GADS::Record->new( - user => $user, - layout => $layout, - schema => $schema, -); -$record->initialise; -$record->fields->{$columns->{string1}->id}->set_value("Foo2"); -$record->fields->{$columns->{date1}->id}->set_value("2014-10-10"); -$record->write; - -is($schema->resultset('AlertCache')->count, 3, "Correct alert cache"); -is($schema->resultset('AlertSend')->count, 1, "Correct alert send"); + }; + + my $view = GADS::View->new( + name => 'View with alerts', + filter => encode_json($rules), + instance_id => $layout->instance_id, + layout => $layout, + schema => $schema, + columns => [$columns->{string1}->id], + ); + $view->write; + + my $alert = GADS::Alert->new( + user => $sheet->user_normal2, + layout => $layout, + schema => $schema, + frequency => 24, + view_id => $view->id, + ); + $alert->write; + + is($schema->resultset('AlertCache')->count, 2, "Correct alert cache"); + is($schema->resultset('AlertSend')->count, 0, "Correct alert send"); + + my $record = GADS::Record->new( + user => $user, + layout => $layout, + schema => $schema, + ); + $record->initialise; + $record->fields->{$columns->{string1}->id}->set_value("Foo2"); + $record->fields->{$columns->{date1}->id}->set_value("2014-10-10"); + $record->write; + + is($schema->resultset('AlertCache')->count, 3, "Correct alert cache"); + is($schema->resultset('AlertSend')->count, 1, "Correct alert send"); + + # Now test that the user with a limited view does not receive alerts for + # records not in the limited view + # Remove previous alert + $alert->frequency(''); + $alert->write; + is($schema->resultset('AlertCache')->count, 0, "Alert cache removed"); + is($schema->resultset('AlertSend')->count, 0, "Alert send removed"); + $view = GADS::View->new( + name => 'Limited user view with alerts', + # Same filter as before, but will result in fewer records (due to view + # limit) + filter => encode_json($rules), + instance_id => $layout->instance_id, + layout => $layout, + schema => $schema, + columns => [$columns->{string1}->id], + ); + $view->write; + + $alert = GADS::Alert->new( + user => $sheet->user_normal1, + layout => $layout, + schema => $schema, + frequency => 24, + view_id => $view->id, + ); + $alert->write; + + is($schema->resultset('AlertCache')->count, 1, "Correct alert cache for view limit user"); + is($schema->resultset('AlertSend')->count, 0, "Correct alert send"); + + # Create a record that will not appear to the view limit user + $record = GADS::Record->new( + user => $user, + layout => $layout, + schema => $schema, + ); + $record->initialise; + $record->fields->{$columns->{string1}->id}->set_value("Foo3"); + $record->fields->{$columns->{date1}->id}->set_value("2014-10-10"); + $record->write; + + is($schema->resultset('AlertCache')->count, 1, "Correct alert cache"); + is($schema->resultset('AlertSend')->count, 0, "Correct alert send"); + + # And now a record that will appear + $record = GADS::Record->new( + user => $user, + layout => $layout, + schema => $schema, + ); + $record->initialise; + $record->fields->{$columns->{string1}->id}->set_value("Foo"); + $record->fields->{$columns->{date1}->id}->set_value("2014-10-10"); + $record->write; + + is($schema->resultset('AlertCache')->count, 2, "Correct alert cache"); + is($schema->resultset('AlertSend')->count, 1, "Correct alert send"); +} done_testing();