Skip to content

Phase out project Owner method

In-depth analysis of Owner method usages:

project.owner method - it is a bit everywhere. In most specs it can probably be replaced by first_owner (whoever that is) but I'm loath to blanket replace in the code.

In the specs, many of the usages are some permutation of let(:user) { project.owner } so these can probably be changed easily to first_owner.

spec

ee/spec

Here are some places where it appears in the code that are a bit complex:

Code location Action MR
ee/app/views/projects/settings/subscriptions/_project.html.haml#L7 avatar_without_link and the project owner name. Up to UX how they'd like to display this to handle multiple owners (a list of avatars up to x width?) !78445 (merged)
app/workers/container_expiration_policy_worker.rb#L60 Called by a worker invoked by a cron job. Not sure who the target user would be if there were multiple owners 🤔 perhaps if we can't use nil, the first_owner would be ok here !78512 (merged)
app/services/projects/create_service.rb#L92 Logs the project owner name after creation - so I think this can be current_user !78511 (merged)
app/workers/auto_devops/disable_worker.rb#L35 This adds recipients for a notification so we probably want to add all the owners of the project (would this also include inherited owners from the parent group(s)?) !79078 (merged)
app/models/issue.rb#L589 this is a performance improvement to the policy framework meant to sync with IssuePolicy. Suggest replacing project.owner == user check with project.team.owner?(user) !78515 (merged)
ee/app/services/ee/notification_service.rb#L124 Returns an array of a single owner so can be changed to return project.owners instead of return [project.owner] unless group. This line could also be removed altogether if the project membership lookup does what we want anyway !79089 (merged)
lib/gitlab/utils/override.rb#L76 This is a method owner (ie. reflection) and is unrelated. n/a
lib/gitlab/hook_data/project_builder.rb#L35 This seems really tricky to change because the data structure expects a single owner 🤔 We could replace with first_owner or change the structure altogether !79285 (merged)
app/views/admin/projects/show.html.haml#L42 and app/views/admin/projects/show.html.haml#L50 change from a single owner display to an array of users, so from = link_to @project.owner_name, [:admin, @project.owner] to - @project.owners each with = link_to owner.name, [:admin, owner] or something. !78974 (merged)

Needs action?

Code location Action MR
Access matcher spec/support/matchers/access_matchers.rb Role check on membership No action needed
spec/support/matchers/access_matchers_for_controller.rb Matchers for objects that respond to owner Comment left
spec/support/helpers/access_matchers_helpers.rb Matchers for objects that respond to owner Comment left
lib/gem_extensions/active_record/disable_joins/associations/association_scope.rb Looks polymorphic, so owner looks like it refers to a reflective property No action needed

We could also do away with this method project_owner_acting_as_maintainer app/finders/projects/members/effective_access_level_finder.rb if we can backfill Project Owners that are missing them.

Edited by charlie ablett