Skip to content

Fix scope of project visibility checks when changing group visibility

Ruby Nealon requested to merge ruby/exclude-hidden-proj-from-visibility-project-check into master

cc @gitlab-org/tenant-scale-group @manojmj

What does this MR do and why?

Fixes issue https://gitlab.com/gitlab-org/gitlab/-/issues/433092 temporarily preventing group visibility from being changed in some instances when group-owned projects have been deleted by using correct scope.

How to set up and validate locally

This issue should only impact the GitLab.com SaaS instance, but you can confirm the fix by keeping the added test but changing the scope back to without_deleted (now not_aimed_for_deletion)

Before fix:

❯ bin/rspec spec/models/group_spec.rb:401
Run options: include {:focus=>true, :locations=>{"./spec/models/group_spec.rb"=>[401]}}

Test environment set up in 3.519824 seconds
F

Failures:

  1) Group validations #visibility_level_allowed_by_projects when group has a lower visibility is valid if higher visibility project is pending deletion via marked_for_deletion_at
     Failure/Error: expect(internal_group).to be_valid
       expected #<Group id:1269 @group2> to be valid, but got errors: Visibility level private is not allowed since this group contains projects with higher visibility.
     # ./spec/models/group_spec.rb:405:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:426:in `block (3 levels) in <top (required)>'
     # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
     # ./spec/spec_helper.rb:417:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/application_context.rb:68:in `with_raw_context'
     # ./spec/spec_helper.rb:413:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:266:in `block (2 levels) in <top (required)>'
     # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'

Finished in 7.28 seconds (files took 13.62 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/group_spec.rb:401 # Group validations #visibility_level_allowed_by_projects when group has a lower visibility is valid if higher visibility project is pending deletion via marked_for_deletion_at

[TEST PROF INFO] Time spent in factories: 00:01.129 (11.52% of total time)

After fix:

❯ bin/rspec spec/models/group_spec.rb:401
Run options: include {:focus=>true, :locations=>{"./spec/models/group_spec.rb"=>[401]}}

Test environment set up in 2.844712 seconds
.

Finished in 6.97 seconds (files took 14.3 seconds to load)
1 example, 0 failures

[TEST PROF INFO] Time spent in factories: 00:01.231 (13.49% of total time)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports