Skip to content

Fix alerts if user has view limit (#1531) #626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abeverley wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Fix alerts if user has view limit (#1531) #626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions lib/GADS/Alert.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) = @_;

Expand All @@ -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,
Expand Down
33 changes: 26 additions & 7 deletions lib/GADS/AlertSend.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
123 changes: 57 additions & 66 deletions lib/GADS/Records.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);

@pwlodarski-ctrlo pwlodarski-ctrlo Jun 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug statement?

@abeverley abeverley Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pawel and good spot but this is actually a "proper" way of doing debugging in the code - these trace statements will only print anything when the log level is high enough.

@pwlodarski-ctrlo pwlodarski-ctrlo Jun 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, thanks for clarifying Andy!

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;
}

Expand Down
10 changes: 10 additions & 0 deletions lib/GADS/Schema/Result/User.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) = @_;

Expand Down
61 changes: 60 additions & 1 deletion t/011_alert_limit.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Loading