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..602933685 100644 --- a/lib/GADS/Schema/Result/User.pm +++ b/lib/GADS/Schema/Result/User.pm @@ -376,6 +376,16 @@ 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) = @_; + !! $self->view_limits->search({ + 'view.instance_id' => $instance_id, + }, { + join => 'view', + })->next; +} + sub set_view_limits { my ($self, $view_ids) = @_; diff --git a/t/011_alert_limit.t b/t/011_alert_limit.t index 72d0409c2..ac846aabd 100644 --- a/t/011_alert_limit.t +++ b/t/011_alert_limit.t @@ -95,7 +95,7 @@ my $view = GADS::View->new( instance_id => $layout->instance_id, layout => $layout, schema => $schema, - columns => [$columns->{string1}->id], + columns => [$columns->{string1}->id], ); $view->write; @@ -124,4 +124,63 @@ $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();