staging into main

[rsmoke]

This pull request updates the entry authorization logic to more precisely determine which users with administrator or manager roles can access entries, and adds corresponding test coverage to ensure correct behavior. The main focus is on improving how container-level roles are handled in both the policy and controller tests.

Authorization logic improvements:

• Refactored the EntryPolicy::Scope#resolve method to determine access for "Collection Administrator" and "Collection Manager" roles based on explicit container assignments, rather than relying solely on user role methods. This ensures that only users assigned to specific containers with the appropriate roles can view entries from those containers.

Test coverage enhancements:

• Added a controller spec to verify that a user assigned as a "Collection Administrator" for a specific container can successfully access the entry's modal details, ensuring the new policy logic is enforced.

[Copilot]

Pull request overview

This pull request refactors the entry authorization logic in the EntryPolicy::Scope#resolve method to enforce container-specific role checks rather than relying on global user role methods. The change moves role queries outside the conditional branches and modifies the condition from checking user.administrator? || user.manager? to checking admin_container_ids.any?. A corresponding controller test is added to verify that users with Collection Administrator roles assigned to specific containers can access entries from those containers.

Changes:

• Modified EntryPolicy::Scope#resolve to determine admin access based on explicit container assignments rather than global role methods
• Added controller test to verify Collection Administrator container-level access to entry modal details

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
app/policies/entry_policy.rb	Refactored Scope#resolve method to check container-specific role assignments instead of global administrator/manager role methods
spec/controllers/entries_controller_spec.rb	Added test context verifying Collection Administrator with container assignment can access entry modal details

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The queries to fetch role IDs and container IDs are now executed unconditionally for all users at the beginning of the resolve method, even when the user is nil or axis_mundi. This could cause unnecessary database queries.

Consider moving these queries inside the elsif admin_container_ids.any? block or restructuring to only execute when needed. For example:

if user.nil?
  scope.none
elsif user.axis_mundi?
  base_scope
elsif user.present?
  admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id)
  admin_container_ids = user.assignments.where(role_id: admin_role_ids).pluck(:container_id).uniq
  
  if admin_container_ids.any?
    # Collection administrators and managers logic
  elsif user.judge?
    # Judge logic
  # ... rest of conditions
end

This would avoid running the queries for nil users and axis_mundi users.

[Copilot]

While the controller test verifies that Collection Administrators with container assignments can access entries, there is no corresponding test coverage for the EntryPolicy::Scope#resolve method itself. The policy spec file (spec/policies/entry_policy_spec.rb) only tests the show? method but does not test the Scope class.

Given that this PR introduces significant changes to the Scope#resolve logic, consider adding comprehensive tests for the Scope class similar to what exists for ContainerPolicy::Scope (spec/policies/container_policy_spec.rb lines 55-86). This would verify that:

• Users with container-specific admin/manager assignments see only entries from their containers
• Users without such assignments don't see entries they shouldn't
• Edge cases like users with assignments to multiple containers are handled correctly

[Copilot]

The condition change from user.administrator? || user.manager? to admin_container_ids.any? creates inconsistency with other methods in the same policy class.

For example, the show? method (line 27) uses user&.has_container_role?(container) which correctly checks for container-specific role assignments. However, other methods like view_applicant_profile? (line 17) also use has_container_role?.

Consider using the existing has_container_role? helper method for consistency:

containers_with_admin_access = user.present? ? user.assignments.joins(:role).where(roles: { kind: ['Collection Administrator', 'Collection Manager'] }).pluck(:container_id).uniq : []

if containers_with_admin_access.any?
  base_scope.joins(contest_instance: { contest_description: :container })
           .where(containers: { id: containers_with_admin_access })

This maintains consistency with the existing authorization patterns in the policy.

[Copilot]

The test context description uses "Container Administrator" but the role kind is 'Collection Administrator'. This inconsistency exists in the codebase (see line 66 which has the same pattern), but it could cause confusion.

Throughout the codebase, the role kind in the database is 'Collection Administrator', but test descriptions sometimes refer to them as "Container Administrator". Consider standardizing on one term - preferably "Collection Administrator" to match the database value and Role model. This would make tests clearer and reduce potential confusion about whether these are different role types.

Suggested change

	context "when user is a Container Administrator for the entry's container" do
	context "when user is a Collection Administrator for the entry's container" do