Skip to content

Add override view option to see restricted records (#814) - #650

Open
abeverley wants to merge 1 commit into
devfrom
814_override_view
Open

Add override view option to see restricted records (#814)#650
abeverley wants to merge 1 commit into
devfrom
814_override_view

Conversation

@abeverley

Copy link
Copy Markdown
Contributor

No description provided.

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR reviewed - minor comments added.

Comment thread lib/GADS/Records.pm
my @search;
return [] if $self->no_view_limits;
foreach my $view (@{$self->_view_limits})
if (!$self->_view_limit_override)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could this be an unless?

unless ($self->_view_limit_override)

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, see previous comment on other PR :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have done, thank you. :)

Comment thread lib/GADS/Schema.pm
__PACKAGE__->load_namespaces;

our $VERSION = 111;
our $VERSION = 112;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Multiple PRs are open which have schema updates - the versioning or order of merging would need to be considered when it comes to it.

Comment thread lib/GADS.pm
}

# Setting a new view limit override
if (defined param('view_limit_override'))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could this be: if (my $override = param('view_limit_override')) ?

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.

Good question - normally, yes. However, in this case a defined but false value means to unset it (https://github.com/ctrlo/GADS/pull/650/changes#diff-88d1585c56a6b278e6f81ddada4991a8d7278f46f85a89e2af503130aaa2a3c9R4)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense, thanks for clarifying!

Comment thread t/003_search_override.t
schema => $schema,
);
try { $record->find_current_id($sensitive_record->current_id) };
like($@, qr/record not found/, "Unable to open limited record");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is $@ meant to be a variable?

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.

In this case it is the result of the try block from the previous line, and should contain the error message being matched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants